Re: [PATCH 2/3] packet: Don't have struct pool specify its buffer

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 13 Dec 2024 23:01:55 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> struct pool, which represents a batch of packets includes values giving
> the buffer in which all the packets lie - or for vhost_user a link to the
> vu_dev_region array in which the packets sit.  Originally that made sense
> because we stored each packet as an offset and length within that buffer.
> 
> However dd143e389 ("packet: replace struct desc by struct iovec") replaced
> the offset and length with a struct iovec which can directly reference a
> packet anywhere in memory.  This means we no longer need the buffer
> reference to interpret packets from the pool.  So there's really no need
> to check where the packet sits.  We can remove the buf reference and all
> checks associated with it.  As a bonus this removes the special case for
> vhost-user.
> 
> Similarly the old representation used a 16-bit length, so there were some
> checks that packets didn't exceed that.  That's also no longer necessary
> with the struct iovec which uses a size_t length.
> 
> I think under an unlikely set of circumstances it might have been possible
> to hit that 16-bit limit for a legitimate packet: other parts of the code
> place a limit of 65535 bytes on the L2 frame, however that doesn't include
> the length tag used by the qemu socket protocol.  That tag *is* included in
> the packet as stored in the pool, however, meaning we could get a 65539
> byte packet at this level.

As I mentioned in the call on Monday: sure, we need to fix this, but at
the same time I'm not quite convinced that it's a good idea to drop all
these sanity checks.

Even if they're not based on offsets anymore, I think it's still
valuable to ensure that the packets are not exactly _anywhere_ in
memory, but only where we expect them to be.

If it's doable, I would rather keep these checks, and change the ones
on the length to allow a maximum value of 65539 bytes. I mean, there's
a big difference between 65539 and, say, 4294967296.

By the way, I haven't checked what happens with MTUs slightly bigger
than 65520 bytes: virtio-net (at least with QEMU) doesn't budge if I
set more than 65520, but I didn't actually send big packets. I'll try
to have a look (also with muvm) unless you already checked.

-- 
Stefano