| Message ID | 20241107163210.3620697-2-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | net: Make ip_header struct QEMU_PACKED | expand |
On Thu, 7 Nov 2024 at 16:32, Peter Maydell <peter.maydell@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.maydell@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. -- PMM
On Fri, Nov 8, 2024 at 4:48 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Thu, 7 Nov 2024 at 16:32, Peter Maydell <peter.maydell@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.maydell@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 >
On Sun, Nov 10, 2024 at 1:10 PM Yan Vugenfirer <yvugenfi@redhat.com> wrote: > Please take a look is this is host\guest common struct > > ---------- Forwarded message --------- > From: Peter Maydell <peter.maydell@linaro.org> > Date: Thu, Nov 7, 2024 at 6:33 PM > Subject: [PATCH 1/2] hw/net/virtio-net.c: Don't assume IP length field is > aligned > To: <qemu-devel@nongnu.org> > Cc: Michael S. Tsirkin <mst@redhat.com>, Jason Wang <jasowang@redhat.com>, > Dmitry Fleytman <dmitry.fleytman@gmail.com>, Akihiko Odaki < > akihiko.odaki@daynix.com> > > > 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.maydell@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); > shouldn't it be lduw_be_p? > +} > + > +static void write_unit_ip_len(VirtioNetRscUnit *unit, uint16_t l) > +{ > + stl_be_p(unit->ip_plen, l); > shouldn't it be stw_be_p? > +} > + > static void virtio_net_rsc_extract_unit4(VirtioNetRscChain *chain, > const uint8_t *buf, > VirtioNetRscUnit *unit) > @@ -2063,7 +2078,7 @@ static void > virtio_net_rsc_extract_unit4(VirtioNetRscChain *chain, > unit->ip_plen = &ip->ip_len; > unit->tcp = (struct tcp_header *)(((uint8_t *)unit->ip) + ip_hdrlen); > unit->tcp_hdrlen = (htons(unit->tcp->th_offset_flags) & 0xF000) >> 10; > - unit->payload = htons(*unit->ip_plen) - ip_hdrlen - unit->tcp_hdrlen; > + unit->payload = read_unit_ip_len(unit) - ip_hdrlen - unit->tcp_hdrlen; > } > > static void virtio_net_rsc_extract_unit6(VirtioNetRscChain *chain, > @@ -2082,7 +2097,7 @@ static void > virtio_net_rsc_extract_unit6(VirtioNetRscChain *chain, > > /* There is a difference between payload length in ipv4 and v6, > ip header is excluded in ipv6 */ > - unit->payload = htons(*unit->ip_plen) - unit->tcp_hdrlen; > + unit->payload = read_unit_ip_len(unit) - unit->tcp_hdrlen; > } > > static size_t virtio_net_rsc_drain_seg(VirtioNetRscChain *chain, > @@ -2231,7 +2246,7 @@ static int32_t > virtio_net_rsc_coalesce_data(VirtioNetRscChain *chain, > VirtioNetRscUnit *o_unit; > > o_unit = &seg->unit; > - o_ip_len = htons(*o_unit->ip_plen); > + o_ip_len = read_unit_ip_len(o_unit); > nseq = htonl(n_unit->tcp->th_seq); > oseq = htonl(o_unit->tcp->th_seq); > > @@ -2267,7 +2282,7 @@ coalesce: > o_unit->payload += n_unit->payload; /* update new data len */ > > /* update field in ip header */ > - *o_unit->ip_plen = htons(o_ip_len + n_unit->payload); > + write_unit_ip_len(o_unit, o_ip_len + n_unit->payload); > > /* Bring 'PUSH' big, the whql test guide says 'PUSH' can be > coalesced > for windows guest, while this may change the behavior for linux > -- > 2.34.1 > > >
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); +} + static void virtio_net_rsc_extract_unit4(VirtioNetRscChain *chain, const uint8_t *buf, VirtioNetRscUnit *unit) @@ -2063,7 +2078,7 @@ static void virtio_net_rsc_extract_unit4(VirtioNetRscChain *chain, unit->ip_plen = &ip->ip_len; unit->tcp = (struct tcp_header *)(((uint8_t *)unit->ip) + ip_hdrlen); unit->tcp_hdrlen = (htons(unit->tcp->th_offset_flags) & 0xF000) >> 10; - unit->payload = htons(*unit->ip_plen) - ip_hdrlen - unit->tcp_hdrlen; + unit->payload = read_unit_ip_len(unit) - ip_hdrlen - unit->tcp_hdrlen; } static void virtio_net_rsc_extract_unit6(VirtioNetRscChain *chain, @@ -2082,7 +2097,7 @@ static void virtio_net_rsc_extract_unit6(VirtioNetRscChain *chain, /* There is a difference between payload length in ipv4 and v6, ip header is excluded in ipv6 */ - unit->payload = htons(*unit->ip_plen) - unit->tcp_hdrlen; + unit->payload = read_unit_ip_len(unit) - unit->tcp_hdrlen; } static size_t virtio_net_rsc_drain_seg(VirtioNetRscChain *chain, @@ -2231,7 +2246,7 @@ static int32_t virtio_net_rsc_coalesce_data(VirtioNetRscChain *chain, VirtioNetRscUnit *o_unit; o_unit = &seg->unit; - o_ip_len = htons(*o_unit->ip_plen); + o_ip_len = read_unit_ip_len(o_unit); nseq = htonl(n_unit->tcp->th_seq); oseq = htonl(o_unit->tcp->th_seq); @@ -2267,7 +2282,7 @@ coalesce: o_unit->payload += n_unit->payload; /* update new data len */ /* update field in ip header */ - *o_unit->ip_plen = htons(o_ip_len + n_unit->payload); + write_unit_ip_len(o_unit, o_ip_len + n_unit->payload); /* Bring 'PUSH' big, the whql test guide says 'PUSH' can be coalesced for windows guest, while this may change the behavior for linux
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.maydell@linaro.org> --- include/hw/virtio/virtio-net.h | 2 +- hw/net/virtio-net.c | 23 +++++++++++++++++++---- 2 files changed, 20 insertions(+), 5 deletions(-)