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

[Alyssa Ross <hi@alyssa.is>]

[-- Attachment #1: Type: text/plain, Size: 2616 bytes --]

"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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]