Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] `SlotManager::activate` is a safe function but can cause a Use-After-Free and hardware state corruption if called with mismatched `slot_data` on an already active seat. --
--- Patch [2]: [PATCH v8 2/7] drm/tyr: add a generic slot manager --- commit c0b62f1ee506ab72e8c6ddd5dd7d561783c86a38 Author: Boris Brezillon <[email protected]> drm/tyr: add a generic slot manager Introduce a generic slot manager to dynamically allocate limited hardware slots to software "seats". It handles address space and command stream group slots, lazily evicting idle slots when contention arises. > --- /dev/null > +++ b/drivers/gpu/drm/tyr/slot.rs [ ... ] > + /// Records a slot as active for the given seat. > + fn record_active_slot( > + &mut self, > + slot_idx: usize, > + locked_seat: &LockedSeat<T, MAX_SLOTS>, > + slot_data: T::SlotData, > + ) { [ ... ] > + self.slots[slot_idx] = Slot::Active(SlotInfo { > + slot_data, > + seqno: cur_seqno, > + }); > + > + self.use_seqno += 1; > + } [ ... ] > + /// Activates a resource on any available/reclaimable slot. > + /// > + /// TODO: add a mechanism to ensure that `locked_seat` and `slot_data` > belong to the > + /// same seat owner since they may be reused. For now the caller has to > be careful > + /// to do this correctly. > + pub(crate) fn activate( > + &mut self, > + locked_seat: &LockedSeat<T, MAX_SLOTS>, > + slot_data: T::SlotData, > + ) -> Result { > + self.check_seat(locked_seat); > + match locked_seat.access(self) { > + // If a seat still has a valid slot, just reuse the slot and > refresh the bookkeeping. > + Seat::Active(seat_info) | Seat::Idle(seat_info) => { > + self.record_active_slot(seat_info.slot as usize, > locked_seat, slot_data); [Severity: High] When a seat is already active or idle, this branch refreshes the slot by calling record_active_slot() with the newly provided slot_data, but without calling self.manager.activate() to update the hardware. In record_active_slot(), the assignment to self.slots[slot_idx] overwrites the old SlotInfo and drops the previous slot_data. If the old slot_data held the last reference to resources mapped to the hardware (such as DMA buffers or pagetables), could they be freed while the hardware is still actively using them, causing a use-after-free? Furthermore, when the slot is later evicted, self.manager.evict() will be called with the newly provided slot_data instead of the old one. Could this confuse the eviction logic and corrupt hardware state? Since activate() is a safe function, relying on callers to be careful about mismatched slot_data without marking the function unsafe or enforcing the invariant at runtime seems to violate Rust's soundness guarantees. Is there a plan to implement the mechanism mentioned in the TODO before this is merged? > + Ok(()) > + } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
