| Message ID | 1487248176-29602-5-git-send-email-dmitry@daynix.com |
|---|---|
| State | New |
| Headers | show |
Hi Dmitry, On 02/16/2017 09:29 AM, Dmitry Fleytman wrote: > In case of VLAN stripping ETH header is stored in a > separate chunk and length of IOV should take this into > account. > > This patch fixes checksum validation for RX packets > with VLAN header. > > Devices affected by this problem: e1000e and vmxnet3. > > Signed-off-by: Dmitry Fleytman <dmitry@daynix.com> > --- > hw/net/net_rx_pkt.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/net/net_rx_pkt.c b/hw/net/net_rx_pkt.c > index d38babe..c7ae33d 100644 > --- a/hw/net/net_rx_pkt.c > +++ b/hw/net/net_rx_pkt.c > @@ -97,7 +97,7 @@ net_rx_pkt_pull_data(struct NetRxPkt *pkt, > pkt->tot_len = iov_size(iov, iovcnt) - ploff + pkt->ehdr_buf_len; > pkt->vec_len = iov_copy(pkt->vec + 1, pkt->vec_len_total - 1, > iov, iovcnt, ploff, > - pkt->tot_len - pkt->ehdr_buf_len); > + pkt->tot_len - pkt->ehdr_buf_len) + 1; I can not understand the trailing '1', shouldn't it be 'sizeof(struct vlan_header)' instead? (=4) This should be easily unit-testable. > } else { > net_rx_pkt_iovec_realloc(pkt, iovcnt); > >
> On 3 Mar 2017, at 18:39 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > Hi Dmitry, > > On 02/16/2017 09:29 AM, Dmitry Fleytman wrote: >> In case of VLAN stripping ETH header is stored in a >> separate chunk and length of IOV should take this into >> account. >> >> This patch fixes checksum validation for RX packets >> with VLAN header. >> >> Devices affected by this problem: e1000e and vmxnet3. >> >> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com> >> --- >> hw/net/net_rx_pkt.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hw/net/net_rx_pkt.c b/hw/net/net_rx_pkt.c >> index d38babe..c7ae33d 100644 >> --- a/hw/net/net_rx_pkt.c >> +++ b/hw/net/net_rx_pkt.c >> @@ -97,7 +97,7 @@ net_rx_pkt_pull_data(struct NetRxPkt *pkt, >> pkt->tot_len = iov_size(iov, iovcnt) - ploff + pkt->ehdr_buf_len; >> pkt->vec_len = iov_copy(pkt->vec + 1, pkt->vec_len_total - 1, >> iov, iovcnt, ploff, >> - pkt->tot_len - pkt->ehdr_buf_len); >> + pkt->tot_len - pkt->ehdr_buf_len) + 1; > > I can not understand the trailing '1', shouldn't it be 'sizeof(struct vlan_header)' instead? (=4) > This should be easily unit-testable. Hi Philippe, Please pay attention that iov_copy returns number of items in target IOV, pkt->vec_len is number of items as well, so +1 is perfectly fine here. ~Dmitry > >> } else { >> net_rx_pkt_iovec_realloc(pkt, iovcnt);
diff --git a/hw/net/net_rx_pkt.c b/hw/net/net_rx_pkt.c index d38babe..c7ae33d 100644 --- a/hw/net/net_rx_pkt.c +++ b/hw/net/net_rx_pkt.c @@ -97,7 +97,7 @@ net_rx_pkt_pull_data(struct NetRxPkt *pkt, pkt->tot_len = iov_size(iov, iovcnt) - ploff + pkt->ehdr_buf_len; pkt->vec_len = iov_copy(pkt->vec + 1, pkt->vec_len_total - 1, iov, iovcnt, ploff, - pkt->tot_len - pkt->ehdr_buf_len); + pkt->tot_len - pkt->ehdr_buf_len) + 1; } else { net_rx_pkt_iovec_realloc(pkt, iovcnt);
In case of VLAN stripping ETH header is stored in a separate chunk and length of IOV should take this into account. This patch fixes checksum validation for RX packets with VLAN header. Devices affected by this problem: e1000e and vmxnet3. Signed-off-by: Dmitry Fleytman <dmitry@daynix.com> --- hw/net/net_rx_pkt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)