On Fri, Nov 8, 2024 at 4:48 AM Peter Maydell <peter.mayd...@linaro.org> wrote:
>
> On Thu, 7 Nov 2024 at 16:32, Peter Maydell <peter.mayd...@linaro.org> wrote:
> >
> > In virtio-net.c we assume that the IP length field in the packet is
> > aligned, and we copy its address into a uint16_t* in the
> > VirtioNetRscUnit struct which we then dereference later. This isn't
> > a safe assumption; it will also result in compilation failures if we
> > mark the ip_header struct as QEMU_PACKED because the compiler will
> > not let you take the address of an unaligned struct field.
> >
> > Make the ip_plen field in VirtioNetRscUnit a void*, and make all the
> > places where we read or write through that pointer instead use some
> > new accessor functions read_unit_ip_len() and write_unit_ip_len()
> > which account for the pointer being potentially unaligned and also do
> > the network-byte-order conversion we were previously using htons() to
> > perform.
> >
> > Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
> > ---
> > include/hw/virtio/virtio-net.h | 2 +-
> > hw/net/virtio-net.c | 23 +++++++++++++++++++----
> > 2 files changed, 20 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> > index 060c23c04d2..b9ea9e824e3 100644
> > --- a/include/hw/virtio/virtio-net.h
> > +++ b/include/hw/virtio/virtio-net.h
> > @@ -102,7 +102,7 @@ typedef struct VirtioNetRscStat {
> > /* Rsc unit general info used to checking if can coalescing */
> > typedef struct VirtioNetRscUnit {
> > void *ip; /* ip header */
> > - uint16_t *ip_plen; /* data len pointer in ip header field */
> > + void *ip_plen; /* pointer to unaligned uint16_t data len in ip header
> > */
> > struct tcp_header *tcp; /* tcp header */
> > uint16_t tcp_hdrlen; /* tcp header len */
> > uint16_t payload; /* pure payload without virtio/eth/ip/tcp */
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index f2104ed364a..11cf462180d 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -2049,6 +2049,21 @@ static ssize_t virtio_net_do_receive(NetClientState
> > *nc, const uint8_t *buf,
> > return virtio_net_receive_rcu(nc, buf, size, false);
> > }
> >
> > +/*
> > + * Accessors to read and write the IP packet data length field. This
> > + * is a potentially unaligned network-byte-order 16 bit unsigned integer
> > + * pointed to by unit->ip_len.
> > + */
> > +static uint16_t read_unit_ip_len(VirtioNetRscUnit *unit)
> > +{
> > + return ldl_be_p(unit->ip_plen);
> > +}
> > +
> > +static void write_unit_ip_len(VirtioNetRscUnit *unit, uint16_t l)
> > +{
> > + stl_be_p(unit->ip_plen, l);
> > +}
>
> These should of course be lduw_be_p() and stw_be_p(), since
> it's a 16 bit field.
>
> Interestingly nothing fails in "make check" or "make check-functional"
> if this breaks, suggesting we aren't exercising virtio-net very hard.
If I was not wrong, we don't test RSC in those tests. And RSC is only
enabled for Windows guests for certification purposes.
Thanks
>
> -- PMM
>
