Re: [RFC PATCH v2 09/10] drm/vino: two heads, 90/270 rotation, damage clips and connector probe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] The frame-damage clips overflow fallback logic permanently drops previously accumulated damage clips instead of properly merging them.
- [High] The CRTC gamma LUT state is incorrectly shared globally rather than being per-head, causing independent display heads to overwrite or clear each other's gamma ramps.

Pre-existing issues:
- [High] Kernel out-of-bounds read occurs when userspace requests plane scaling.
- [High] The plane `atomic_update` and CRTC callbacks run in atomic context but acquire sleepable Mutexes and perform GFP_KERNEL allocations.
- [Medium] The cursor plane update logic uses the raw virtual address of the framebuffer as a cache key, risking an ABA problem where a re-uploaded cursor is silently ignored.
--

commit 02346681a0e083ee04b818868a94582e0d21e04e
Author: Mike Lothian <mike@fireburn.co.uk>

    drm/vino: two heads, 90/270 rotation, damage clips and connector probe
    
    Consume the new connector detect()/mode_valid() and plane damage-clip
    bindings, and finish the KMS feature set the driver previously stubbed:

> diff --git a/drivers/gpu/drm/vino/drm_sink.rs b/drivers/gpu/drm/vino/drm_sink.rs
> index 898cfae8fd4fb..ee04a5af9f7d5 100644
> --- a/drivers/gpu/drm/vino/drm_sink.rs
> +++ b/drivers/gpu/drm/vino/drm_sink.rs
> @@ -435,10 +474,12 @@ fn atomic_enable(commit: CrtcAtomicCommit<'_, Self>) {
>      /// against a shadow the dock may have dropped, and blanks the monitor via DDC/CI -- a
>      /// no-op until CP engages.
>      fn atomic_disable(commit: CrtcAtomicCommit<'_, Self>) {
> -        let data: &VinoDrmData = commit.crtc().drm_dev();
> +        let crtc = commit.crtc();
> +        let head = crtc.head;
> +        let data: &VinoDrmData = crtc.drm_dev();
>          data.update_gamma(None);

[Severity: High]
Since the driver now supports multiple independent display heads, is it safe to
share a single global gamma state?

`data.update_gamma` modifies the shared `gamma` in `VinoDrmData`. This could
inadvertently overwrite or clear the gamma ramps of other connected displays
when one head is updated or disabled.

Should `gamma` be made per-head, similar to the `connectors` array?

Also, this is a pre-existing issue, but doesn't `data.update_gamma` lock a
sleepable Mutex? CRTC callbacks like `atomic_disable` and `atomic_enable` can
run in atomic context during non-blocking atomic commits where sleeping is
forbidden.

[ ... ]

> @@ -498,14 +554,14 @@ fn atomic_update(commit: PlaneAtomicCommit<'_, Self>) {
>              return;
>          }
>          let plane = commit.plane();
> +        let head = plane.head;
>          let data: &VinoDrmData = plane.drm_dev();
> -        let new = commit.take_new_state();
>  
>          // Cursor plane: forward the cursor bitmap/position to the dock over CP (id=0x1b create,
>          // 0x401c image, 0x1a move -- see `cp::cursor_*`). A no-op until CP engages, like scanout.
>          if plane.is_cursor {
>              use core::sync::atomic::Ordering::Relaxed;
> -            const HEAD: u8 = 0;
> +            let new = commit.take_new_state();
>              match new.framebuffer::<VinoDrmDriver>() {
>                  Some(fb) => {
>                      let w = fb.width() as u16;
> @@ -514,15 +570,17 @@ fn atomic_update(commit: PlaneAtomicCommit<'_, Self>) {
>                      // position (a bare cursor move must not re-send the whole image).
>                      let key = fb as *const _ as usize;
>                      if plane.cursor_last.swap(key, Relaxed) != key {

[Severity: Medium]
This is a pre-existing issue, but does using the raw virtual address of the
framebuffer as a cache key risk an ABA problem?

If userspace frees a cursor framebuffer and immediately allocates a new one,
the memory allocator may place it at the same virtual address. This could
cause the new cursor image to be silently ignored because the cache key
hasn't changed.

[ ... ]

> @@ -537,6 +597,37 @@ fn atomic_update(commit: PlaneAtomicCommit<'_, Self>) {
>          let (w, h) = (new.crtc_w() as usize, new.crtc_h() as usize);

[Severity: High]
This is a pre-existing issue, but can this lead to an out-of-bounds read if
userspace requests plane scaling?

The destination dimensions `crtc_w` and `crtc_h` are used to iterate and
calculate source memory offsets. If a compositor provides a small framebuffer
and requests scaling, the offsets could far exceed the bounds of the source
framebuffer.

>          // Plane rotation/reflection (identity unless the compositor set the rotation property).
>          let rotation = new.rotation();
> +        // Collect the client's individual frame-damage clips (the rectangles that
> +        // `damage_merged()` would collapse into one bounding box), each clamped to the output, so
> +        // only the genuinely changed rectangles are re-converted from the source rather than their
> +        // whole enclosing box. Only for identity rotation (the clips are in un-rotated source
> +        // space; mapping them through 90/270 is not worth it for the throttled fallback path), and
> +        // never on the WHT keyframe path -- see `encode_and_send`. A fixed stack array keeps the
> +        // atomic-commit path allocation-free; on overflow the clips collapse into one bounding box.
> +        // An empty list means "convert the whole output" (used for the rotated/reflected case).
> +        let mut clips = [(0usize, 0usize, 0usize, 0usize); MAX_DAMAGE_CLIPS];
> +        let mut nclips = 0usize;
> +        if rotation & bindings::DRM_MODE_ROTATE_MASK == bindings::DRM_MODE_ROTATE_0
> +            && rotation & (bindings::DRM_MODE_REFLECT_X | bindings::DRM_MODE_REFLECT_Y) == 0
> +        {
> +            new.for_each_damage_clip(old, |r| {
> +                let c = (
> +                    (r.x1.max(0) as usize).min(w),
> +                    (r.y1.max(0) as usize).min(h),
> +                    (r.x2.max(0) as usize).min(w),
> +                    (r.y2.max(0) as usize).min(h),
> +                );
> +                if nclips < MAX_DAMAGE_CLIPS {
> +                    clips[nclips] = c;
> +                    nclips += 1;
> +                } else {
> +                    // Overflow: collapse everything so far (and `c`) into clips[0]'s bounding box.
> +                    let b = clips[0];
> +                    clips[0] = (b.0.min(c.0), b.1.min(c.1), b.2.max(c.2), b.3.max(c.3));
> +                    nclips = 1;

[Severity: High]
Does this overflow fallback logic correctly merge the clips?

When `nclips` reaches `MAX_DAMAGE_CLIPS`, it updates `clips[0]` to be the
bounding box of `clips[0]` and `c`. However, this completely discards the
previously accumulated clips from `clips[1]` through `clips[15]`, dropping
them permanently rather than expanding the bounding box to include them.

> +                }
> +            });
> +        }
>  
>          use core::sync::atomic::Ordering::Relaxed;

[ ... ]

> @@ -693,6 +803,8 @@ fn encode_and_send(
>          return encode_and_send_wht(data, plane, vaddr, pitch, rotation, w, h);
>      }
>      let gamma = data.gamma_snapshot();
> +    // Source dimensions (swapped from the output for 90/270 rotation).
> +    let (sw, sh) = src_dims(rotation, w, h);
>      let frame = {
>          let mut st = plane.scanout.lock();

[Severity: High]
This isn't a bug introduced by this patch, but doesn't `atomic_update` run
in atomic context during non-blocking atomic commits? 

Locking a sleepable Mutex like `plane.scanout.lock()` here can cause kernel
warnings and system instability.

[ ... ]

> @@ -746,7 +868,7 @@ fn encode_and_send(
>      // DRM device, so it is bound for the duration of this push.
>      let dev = unsafe { data.intf.as_bound() };
>      prime_video_eps(dev);
> -    dev.bulk_send(VIDEO_EP, &frame, super::timeout(), GFP_KERNEL)?;
> +    dev.bulk_send(VIDEO_EPS[plane.head as usize], &frame, super::timeout(), GFP_KERNEL)?;

[Severity: High]
Similarly, this is a pre-existing issue, but does `dev.bulk_send` perform
allocations using the `GFP_KERNEL` flag? Using `GFP_KERNEL` in atomic context
is unsafe and can sleep.

>      Ok(())
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703030217.2886-1-mike@fireburn.co.uk?part=9