Re: [REGRESSION] virtio: reject shm region if length is zero

["Michael S. Tsirkin" <mst@redhat.com>]

On Sat, Aug 16, 2025 at 01:05:21PM +0200, Alyssa Ross wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Fri, Aug 15, 2025 at 09:19:34PM +0200, Alyssa Ross wrote:
> >> Alyssa Ross <hi@alyssa.is> writes:
> >> 
> >> > On Mon, May 12, 2025 at 07:51:53AM +0930, SamiUddinsami.md.ko@gmail.com wrote:
> >> >> From: Sami Uddin <sami.md.ko@gmail.com>
> >> >>
> >> >> Prevent usage of shared memory regions where the length is zero,
> >> >> as such configurations are not valid and may lead to unexpected behavior.
> >> >>
> >> >> Signed-off-by: Sami Uddin <sami.md.ko@gmail.com>
> >> >> ---
> >> >> v3:
> >> >> - Use idiomatic 'if (!region->len)' as suggested by reviewer
> >> >> v2:
> >> >> - Fixed coding style issue: added space after 'if' statement
> >> >>
> >> >>  include/linux/virtio_config.h | 2 ++
> >> >>  1 file changed, 2 insertions(+)
> >> >
> >> > Hi, I'm sorry to be the bearer of bad news, but since this patch my VM
> >> > no longer works.  The system is running wayland-proxy-virtwl[1] inside
> >> > a crosvm[2] VM, using crosvm's virtio-gpu device to do cross-domain
> >> > Wayland forwarding.
> >> >
> >> > Since this change, wayland-proxy-virtwl crashes with the following log
> >> > message:
> >> >
> >> > 	wl-proxy [WARNING]: Error handling client: Unix.Unix_error(Unix.EINVAL, "DRM_IOCTL_VIRTGPU_RESOURCE_CREATE_BLOB", "")
> >> >
> >> > I'm pretty confused by what this change was supposed to do in the first
> >> > place…  Looking at how virtio_get_shm_region() is used in
> >> > virtio_gpu_init(), it's called with a pointer to zeroed memory, and then
> >> > the get_shm_region() implementation is supposed to write to the region,
> >> > without ever reading from it as far as I can tell.  Why is the initial
> >> > value of an out parameter being checked at all?  How does this prevent
> >> > using zero-length shared memory regions?
> >> >
> >> > [1]: https://crosvm.dev/
> >> > [2]: https://github.com/talex5/wayland-proxy-virtwl
> >> >
> >> > #regzbot introduced: 206cc44588f72b49ad4d7e21a7472ab2a72a83df
> >> 
> >> Okay, just found that it's already been reverted:
> >> https://lore.kernel.org/all/20250808072533-mutt-send-email-mst@kernel.org/
> >> 
> >> Still, I'm confused how this was supposed to fix anything…
> >> 
> >> #regzbot fix: Revert "virtio: reject shm region if length is zero"
> >
> >
> >
> > Are you asking why was the patch applied in the 1st place?
> > It seemed like an invalid behaviour to me, and I thought it's
> > not too late to block it so we don't need to support it
> > down the road.
> 
> So you just weren't aware during the review that it's an output
> parameter rather than an input?  Should the parameter maybe be renamed
> or something to make that more obvious?

Sounds like a good idea.

-- 
MST