"Michael S. Tsirkin" [off-list ref] writes:

On Fri, Aug 15, 2025 at 09:19:34PM +0200, Alyssa Ross wrote:

quoted

Alyssa Ross [off-list ref] writes:

quoted

On Mon, May 12, 2025 at 07:51:53AM +0930, SamiUddinsami.md.ko@gmail.com wrote:

quoted

From: Sami Uddin <redacted>

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 <redacted>
---
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/ (local)

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?