Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/dp_mst: data race on allocated_pbn can bypass bandwidth limits - [Medium] drm/dp_mst: bandwidth check fails on shrinking payloads due to stale allocated_pbn --
commit d52065aed1fa389dfd394c9d3cae48a882973e04 Author: Xiao Lu <[email protected]> drm/dp_mst: Track allocated_pbn from ALLOCATE_PAYLOAD down-reply This commit stores the reported PBN from an ALLOCATE_PAYLOAD reply. It uses the larger of the requested and allocated PBN for bandwidth checks to account for constraints applied by intermediate branches along the path. > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c > b/drivers/gpu/drm/display/drm_dp_mst_topology.c > index 7ce9e212770ad..86a42c5e2debb 100644 > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c [ ... ] > @@ -3166,10 +3167,14 @@ static int drm_dp_payload_send_msg(struct > drm_dp_mst_topology_mgr *mgr, > */ > ret = drm_dp_mst_wait_tx_reply(mstb, txmsg); > if (ret > 0) { > - if (txmsg->reply.reply_type == DP_SIDEBAND_REPLY_NAK) > + if (txmsg->reply.reply_type == DP_SIDEBAND_REPLY_NAK) { > ret = -EINVAL; > - else > + } else { > + if (payload) > + payload->allocated_pbn = > + > txmsg->reply.u.allocate_payload.allocated_pbn; [Severity: High] Can this lockless write to payload->allocated_pbn race with an atomic check? If a non-blocking atomic commit is executing asynchronously, it receives the ALLOCATE_PAYLOAD reply and drm_dp_payload_send_msg() updates allocated_pbn without locks. Concurrently, if userspace initiates a new atomic check, drm_dp_mst_duplicate_state() will copy the payload struct via kmemdup(). Could this result in the atomic check reading a torn or stale value when evaluating max(payload->pbn, payload->allocated_pbn) in drm_dp_mst_atomic_check_port_bw_limit(), potentially bypassing bandwidth constraints? > ret = 0; > + } > } > kfree(txmsg); > fail_put: [ ... ] > @@ -4575,6 +4580,7 @@ int drm_dp_atomic_release_time_slots(struct > drm_atomic_commit *state, > drm_dbg_atomic(mgr->dev, "[MST PORT:%p] TU %d -> 0\n", port, > payload->time_slots); > if (!payload->delete) { > payload->pbn = 0; > + payload->allocated_pbn = 0; [Severity: Medium] Does this properly handle payloads that shrink in size? Since payload->allocated_pbn is only cleared here when releasing the payload entirely (during a full deletion), what happens if a commit simply shrinks an existing payload (for example, switching to a smaller display mode on the same CRTC)? In that scenario, drm_dp_atomic_release_time_slots() skips active CRTCs, meaning allocated_pbn remains at its old, larger value. When the new bandwidth is checked, max(payload->pbn, payload->allocated_pbn) in drm_dp_mst_atomic_check_port_bw_limit() would yield the old larger value, which could falsely prevent the freed bandwidth from being reallocated to other displays. > payload->delete = true; > if (payload->vcpi > 0) > topology_state->payload_mask &= ~BIT(payload->vcpi - 1); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
