Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [Medium] Structural memory under-allocation for subclassed plane state. The virtio-gpu driver subclasses `drm_plane_state` into `virtio_gpu_plane_state` (adding a `fence` member), but uses the generic `drm_atomic_helper_plane_create_state` helper for its `.atomic_create_state` hook. This helper allocates only `sizeof(struct drm_plane_state)`. Consequently, the initial plane state is under-allocated. Although current subclass field accesses (like `fence`) are protected by `if (state->fb)` which prevents an immediate out-of-bounds access on the initial state, this is a strict violation of DRM atomic API contracts which mandate custom allocation hooks when subclassing states. -- commit 021c0051fe694d9f32c7c2b9fb493bbd17a34379 Author: Maxime Ripard <[email protected]> drm/virtio: Convert to atomic_create_state The plane only initializes a pristine state in its reset hook using drm_atomic_helper_plane_reset(), which is equivalent to what atomic_create_state expects. Convert to it. > diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c > b/drivers/gpu/drm/virtio/virtgpu_plane.c > index 1d1b27ece62a7..52971864db50b 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_plane.c > +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c > @@ -91,7 +91,7 @@ drm_plane_state *virtio_gpu_plane_duplicate_state(struct > drm_plane *plane) > static const struct drm_plane_funcs virtio_gpu_plane_funcs = { > .update_plane = drm_atomic_helper_update_plane, > .disable_plane = drm_atomic_helper_disable_plane, > - .reset = drm_atomic_helper_plane_reset, > + .atomic_create_state = drm_atomic_helper_plane_create_state, [Severity: Medium] This is a pre-existing issue, but does assigning the generic helper here result in structural memory under-allocation for the subclassed plane state? The virtio-gpu driver subclasses drm_plane_state into virtio_gpu_plane_state which adds a fence member: drivers/gpu/drm/virtio/virtgpu_drv.h: struct virtio_gpu_plane_state { struct drm_plane_state base; struct virtio_gpu_fence *fence; }; However, the generic helper drm_atomic_helper_plane_create_state in drivers/gpu/drm/drm_atomic_state_helper.c only allocates the size of the base struct drm_plane_state: drm_atomic_helper_plane_create_state() { ... state = kzalloc_obj(*state); ... } Any subsequent downcast on this initial state treats it as a larger object. Currently, no out-of-bounds access occurs because accesses to the subclassed fields (e.g., fence) are guarded by if (state->fb), and the initial state has a NULL framebuffer. Should a custom allocation hook be used here to allocate the full virtio_gpu_plane_state and adhere to the DRM atomic API contracts? > .atomic_duplicate_state = virtio_gpu_plane_duplicate_state, > .atomic_destroy_state = drm_atomic_helper_plane_destroy_state, > }; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=22
