Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Data race and TOCTOU flaw on payload->allocated_pbn. - [Medium] Stale allocated_pbn constraint leads to spurious -ENOSPC errors when reducing bandwidth. --
commit 51837e7219143c6b9b0e48d938f61a3258bf06cd Author: Xiao Lu <[email protected]> drm/dp_mst: Track allocated_pbn from ALLOCATE_PAYLOAD down-reply The DP MST ALLOCATE_PAYLOAD sideband message returns the payload bandwidth actually allocated by the branch in the allocated_pbn field of the down-reply. That value may differ from the requested PBN when an intermediate branch applies its own constraints, so the source should keep track of it instead of only relying on the local request. > 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 write to payload->allocated_pbn cause a data race with concurrent atomic checks? When a non-blocking mode commit runs atomic_commit_tail in a workqueue, it writes to payload->allocated_pbn here. Concurrently, if userspace initiates a new atomic check, it calls drm_dp_mst_duplicate_state(), which synchronously copies the active payload object: drivers/gpu/drm/display/drm_dp_mst_topology.c:drm_dp_mst_duplicate_state() { ... list_for_each_entry(pos, &old_state->payloads, next) { ... payload = kmemdup(pos, sizeof(*payload), GFP_KERNEL); ... } This unsynchronized kmemdup() could result in a torn read of payload->allocated_pbn, which is later evaluated during the atomic check in drm_dp_mst_atomic_check_port_bw_limit(). > ret = 0; > + } > } > kfree(txmsg); > fail_put: [ ... ] > @@ -5352,7 +5359,7 @@ drm_dp_mst_atomic_check_port_bw_limit(struct > drm_dp_mst_port *port, > return -EINVAL; > } > > - pbn_used = payload->pbn; > + pbn_used = max(payload->pbn, payload->allocated_pbn); [Severity: Medium] Will this stale payload->allocated_pbn constraint lead to spurious -ENOSPC errors when reducing bandwidth? If userspace initially sets a high resolution, payload->allocated_pbn is populated with a high bandwidth value. Later, if userspace requests a lower resolution, drm_dp_atomic_find_time_slots() updates payload->pbn but leaves payload->allocated_pbn unchanged: drivers/gpu/drm/display/drm_dp_mst_topology.c:drm_dp_atomic_find_time_slots() { ... payload->time_slots = req_slots; payload->pbn = pbn; return req_slots; } Because payload->allocated_pbn is not cleared or updated, max(payload->pbn, payload->allocated_pbn) will resolve to the old, larger bandwidth. This could falsely fail the bandwidth limit check and prevent userspace from reclaiming bandwidth for other displays. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
