Re: [PATCH v8 2/7] drm/tyr: add a generic slot manager

[sashiko-bot@kernel.org]

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 <boris.brezillon@collabora.com>

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/20260721-fw-boot-b4-v8-0-b28f744581c9@collabora.com?part=2