[Qemu-devel] [PATCH 0/5] A few VLAN-related bugfixes for RX packet abstraction

[Qemu-devel] [PATCH 0/5] A few VLAN-related bugfixes for RX packet abstraction

[Dmitry Fleytman]

This series fix a few issues related
to processing of RX packets with VLAN headers.

See commit messages of specific patches
for information regarding affected devices.

Dmitry Fleytman (5):
  eth: Extend vlan stripping functions
  NetRxPkt: Fix memory corruption on VLAN header stripping
  NetRxPkt: Do not try to pull more data than present
  NetRxPkt: Account buffer with ETH header in IOV length
  NetRxPkt: Remove code duplication in net_rx_pkt_pull_data()

 hw/net/net_rx_pkt.c | 40 +++++++++++++++++++++-------------------
 include/net/eth.h   |  4 ++--
 net/eth.c           | 25 ++++++++++++++-----------
 3 files changed, 37 insertions(+), 32 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/5] eth: Extend vlan stripping functions

[Dmitry Fleytman]

Make VLAN stripping functions return number of bytes
copied to given Ethernet header buffer.

This information should be used to re-compose
packet IOV after VLAN stripping.

Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
---
 include/net/eth.h |  4 ++--
 net/eth.c         | 25 ++++++++++++++-----------
 2 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/include/net/eth.h b/include/net/eth.h
index 2013175..afeb45b 100644
--- a/include/net/eth.h
+++ b/include/net/eth.h
@@ -331,12 +331,12 @@ eth_get_pkt_tci(const void *p)
     }
 }
 
-bool
+size_t
 eth_strip_vlan(const struct iovec *iov, int iovcnt, size_t iovoff,
                uint8_t *new_ehdr_buf,
                uint16_t *payload_offset, uint16_t *tci);
 
-bool
+size_t
 eth_strip_vlan_ex(const struct iovec *iov, int iovcnt, size_t iovoff,
                   uint16_t vet, uint8_t *new_ehdr_buf,
                   uint16_t *payload_offset, uint16_t *tci);
diff --git a/net/eth.c b/net/eth.c
index df81efb..5b9ba26 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -232,7 +232,7 @@ void eth_get_protocols(const struct iovec *iov, int iovcnt,
     }
 }
 
-bool
+size_t
 eth_strip_vlan(const struct iovec *iov, int iovcnt, size_t iovoff,
                uint8_t *new_ehdr_buf,
                uint16_t *payload_offset, uint16_t *tci)
@@ -244,7 +244,7 @@ eth_strip_vlan(const struct iovec *iov, int iovcnt, size_t iovoff,
                                new_ehdr, sizeof(*new_ehdr));
 
     if (copied < sizeof(*new_ehdr)) {
-        return false;
+        return 0;
     }
 
     switch (be16_to_cpu(new_ehdr->h_proto)) {
@@ -254,7 +254,7 @@ eth_strip_vlan(const struct iovec *iov, int iovcnt, size_t iovoff,
                             &vlan_hdr, sizeof(vlan_hdr));
 
         if (copied < sizeof(vlan_hdr)) {
-            return false;
+            return 0;
         }
 
         new_ehdr->h_proto = vlan_hdr.h_proto;
@@ -268,18 +268,21 @@ eth_strip_vlan(const struct iovec *iov, int iovcnt, size_t iovoff,
                                 PKT_GET_VLAN_HDR(new_ehdr), sizeof(vlan_hdr));
 
             if (copied < sizeof(vlan_hdr)) {
-                return false;
+                return 0;
             }
 
             *payload_offset += sizeof(vlan_hdr);
+
+            return sizeof(struct eth_header) + sizeof(struct vlan_header);
+        } else {
+            return sizeof(struct eth_header);
         }
-        return true;
     default:
-        return false;
+        return 0;
     }
 }
 
-bool
+size_t
 eth_strip_vlan_ex(const struct iovec *iov, int iovcnt, size_t iovoff,
                   uint16_t vet, uint8_t *new_ehdr_buf,
                   uint16_t *payload_offset, uint16_t *tci)
@@ -291,7 +294,7 @@ eth_strip_vlan_ex(const struct iovec *iov, int iovcnt, size_t iovoff,
                                new_ehdr, sizeof(*new_ehdr));
 
     if (copied < sizeof(*new_ehdr)) {
-        return false;
+        return 0;
     }
 
     if (be16_to_cpu(new_ehdr->h_proto) == vet) {
@@ -299,17 +302,17 @@ eth_strip_vlan_ex(const struct iovec *iov, int iovcnt, size_t iovoff,
                             &vlan_hdr, sizeof(vlan_hdr));
 
         if (copied < sizeof(vlan_hdr)) {
-            return false;
+            return 0;
         }
 
         new_ehdr->h_proto = vlan_hdr.h_proto;
 
         *tci = be16_to_cpu(vlan_hdr.h_tci);
         *payload_offset = iovoff + sizeof(*new_ehdr) + sizeof(vlan_hdr);
-        return true;
+        return sizeof(struct eth_header);
     }
 
-    return false;
+    return 0;
 }
 
 void
-- 
2.7.4

[Qemu-devel] [PATCH 2/5] NetRxPkt: Fix memory corruption on VLAN header stripping

[Dmitry Fleytman]

This patch fixed a problem that was introduced in commit eb700029.

When net_rx_pkt_attach_iovec() calls eth_strip_vlan()
this can result in pkt->ehdr_buf being overflowed, because
ehdr_buf is only sizeof(struct eth_header) bytes large
but eth_strip_vlan() can write
sizeof(struct eth_header) + sizeof(struct vlan_header)
bytes into it.

Devices affected by this problem: vmxnet3.

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
---
 hw/net/net_rx_pkt.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/hw/net/net_rx_pkt.c b/hw/net/net_rx_pkt.c
index 1019b50..7c0beac 100644
--- a/hw/net/net_rx_pkt.c
+++ b/hw/net/net_rx_pkt.c
@@ -23,13 +23,13 @@
 
 struct NetRxPkt {
     struct virtio_net_hdr virt_hdr;
-    uint8_t ehdr_buf[sizeof(struct eth_header)];
+    uint8_t ehdr_buf[sizeof(struct eth_header) + sizeof(struct vlan_header)];
     struct iovec *vec;
     uint16_t vec_len_total;
     uint16_t vec_len;
     uint32_t tot_len;
     uint16_t tci;
-    bool vlan_stripped;
+    size_t ehdr_buf_len;
     bool has_virt_hdr;
     eth_pkt_types_e packet_type;
 
@@ -88,15 +88,13 @@ net_rx_pkt_pull_data(struct NetRxPkt *pkt,
                         const struct iovec *iov, int iovcnt,
                         size_t ploff)
 {
-    if (pkt->vlan_stripped) {
+    if (pkt->ehdr_buf_len) {
         net_rx_pkt_iovec_realloc(pkt, iovcnt + 1);
 
         pkt->vec[0].iov_base = pkt->ehdr_buf;
-        pkt->vec[0].iov_len = sizeof(pkt->ehdr_buf);
-
-        pkt->tot_len =
-            iov_size(iov, iovcnt) - ploff + sizeof(struct eth_header);
+        pkt->vec[0].iov_len = pkt->ehdr_buf_len;
 
+        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);
     } else {
@@ -123,11 +121,12 @@ void net_rx_pkt_attach_iovec(struct NetRxPkt *pkt,
     uint16_t tci = 0;
     uint16_t ploff = iovoff;
     assert(pkt);
-    pkt->vlan_stripped = false;
 
     if (strip_vlan) {
-        pkt->vlan_stripped = eth_strip_vlan(iov, iovcnt, iovoff, pkt->ehdr_buf,
-                                            &ploff, &tci);
+        pkt->ehdr_buf_len = eth_strip_vlan(iov, iovcnt, iovoff, pkt->ehdr_buf,
+                                           &ploff, &tci);
+    } else {
+        pkt->ehdr_buf_len = 0;
     }
 
     pkt->tci = tci;
@@ -143,12 +142,13 @@ void net_rx_pkt_attach_iovec_ex(struct NetRxPkt *pkt,
     uint16_t tci = 0;
     uint16_t ploff = iovoff;
     assert(pkt);
-    pkt->vlan_stripped = false;
 
     if (strip_vlan) {
-        pkt->vlan_stripped = eth_strip_vlan_ex(iov, iovcnt, iovoff, vet,
-                                               pkt->ehdr_buf,
-                                               &ploff, &tci);
+        pkt->ehdr_buf_len = eth_strip_vlan_ex(iov, iovcnt, iovoff, vet,
+                                              pkt->ehdr_buf,
+                                              &ploff, &tci);
+    } else {
+        pkt->ehdr_buf_len = 0;
     }
 
     pkt->tci = tci;
@@ -162,8 +162,8 @@ void net_rx_pkt_dump(struct NetRxPkt *pkt)
     NetRxPkt *pkt = (NetRxPkt *)pkt;
     assert(pkt);
 
-    printf("RX PKT: tot_len: %d, vlan_stripped: %d, vlan_tag: %d\n",
-              pkt->tot_len, pkt->vlan_stripped, pkt->tci);
+    printf("RX PKT: tot_len: %d, ehdr_buf_len: %lu, vlan_tag: %d\n",
+              pkt->tot_len, pkt->ehdr_buf_len, pkt->tci);
 #endif
 }
 
@@ -426,7 +426,7 @@ bool net_rx_pkt_is_vlan_stripped(struct NetRxPkt *pkt)
 {
     assert(pkt);
 
-    return pkt->vlan_stripped;
+    return pkt->ehdr_buf_len ? true : false;
 }
 
 bool net_rx_pkt_has_virt_hdr(struct NetRxPkt *pkt)
-- 
2.7.4

[Qemu-devel] [PATCH 3/5] NetRxPkt: Do not try to pull more data than present

[Dmitry Fleytman]

In case of VLAN stripping, ETH header put into a
separate buffer, therefore amont of data copied
from original IOV should be smaller.

Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
---
 hw/net/net_rx_pkt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/net/net_rx_pkt.c b/hw/net/net_rx_pkt.c
index 7c0beac..d38babe 100644
--- a/hw/net/net_rx_pkt.c
+++ b/hw/net/net_rx_pkt.c
@@ -96,7 +96,8 @@ 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);
+                                iov, iovcnt, ploff,
+                                pkt->tot_len - pkt->ehdr_buf_len);
     } else {
         net_rx_pkt_iovec_realloc(pkt, iovcnt);
 
-- 
2.7.4

[Qemu-devel] [PATCH 4/5] NetRxPkt: Account buffer with ETH header in IOV length

[Dmitry Fleytman]

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;
     } else {
         net_rx_pkt_iovec_realloc(pkt, iovcnt);
 
-- 
2.7.4

[Qemu-devel] [PATCH 5/5] NetRxPkt: Remove code duplication in net_rx_pkt_pull_data()

[Dmitry Fleytman]

This is a refactoring commit that does not change behavior.

Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
---
 hw/net/net_rx_pkt.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/net/net_rx_pkt.c b/hw/net/net_rx_pkt.c
index c7ae33d..3899211 100644
--- a/hw/net/net_rx_pkt.c
+++ b/hw/net/net_rx_pkt.c
@@ -88,20 +88,21 @@ net_rx_pkt_pull_data(struct NetRxPkt *pkt,
                         const struct iovec *iov, int iovcnt,
                         size_t ploff)
 {
+    uint32_t pllen = iov_size(iov, iovcnt) - ploff;
+
     if (pkt->ehdr_buf_len) {
         net_rx_pkt_iovec_realloc(pkt, iovcnt + 1);
 
         pkt->vec[0].iov_base = pkt->ehdr_buf;
         pkt->vec[0].iov_len = pkt->ehdr_buf_len;
 
-        pkt->tot_len = iov_size(iov, iovcnt) - ploff + pkt->ehdr_buf_len;
+        pkt->tot_len = pllen + 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) + 1;
+                                iov, iovcnt, ploff, pllen) + 1;
     } else {
         net_rx_pkt_iovec_realloc(pkt, iovcnt);
 
-        pkt->tot_len = iov_size(iov, iovcnt) - ploff;
+        pkt->tot_len = pllen;
         pkt->vec_len = iov_copy(pkt->vec, pkt->vec_len_total,
                                 iov, iovcnt, ploff, pkt->tot_len);
     }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 0/5] A few VLAN-related bugfixes for RX packet abstraction

[Peter Maydell]

On 16 February 2017 at 12:29, Dmitry Fleytman <dmitry@daynix.com> wrote:
> This series fix a few issues related
> to processing of RX packets with VLAN headers.
>
> See commit messages of specific patches
> for information regarding affected devices.

I think at least the "fix buffer overrun" parts of this
patchset should be cc:stable ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/5] A few VLAN-related bugfixes for RX packet abstraction

[Jason Wang]

On 2017年02月16日 20:29, Dmitry Fleytman wrote:
> This series fix a few issues related
> to processing of RX packets with VLAN headers.
>
> See commit messages of specific patches
> for information regarding affected devices.
>
> Dmitry Fleytman (5):
>    eth: Extend vlan stripping functions
>    NetRxPkt: Fix memory corruption on VLAN header stripping
>    NetRxPkt: Do not try to pull more data than present
>    NetRxPkt: Account buffer with ETH header in IOV length
>    NetRxPkt: Remove code duplication in net_rx_pkt_pull_data()
>
>   hw/net/net_rx_pkt.c | 40 +++++++++++++++++++++-------------------
>   include/net/eth.h   |  4 ++--
>   net/eth.c           | 25 ++++++++++++++-----------
>   3 files changed, 37 insertions(+), 32 deletions(-)
>

Applied. Thanks

Re: [Qemu-devel] [PATCH 0/5] A few VLAN-related bugfixes for RX packet abstraction

[Jason Wang]

On 2017年02月16日 20:39, Peter Maydell wrote:
> On 16 February 2017 at 12:29, Dmitry Fleytman <dmitry@daynix.com> wrote:
>> This series fix a few issues related
>> to processing of RX packets with VLAN headers.
>>
>> See commit messages of specific patches
>> for information regarding affected devices.
> I think at least the "fix buffer overrun" parts of this
> patchset should be cc:stable ?
>
> thanks
> -- PMM

Yes, I add cc:stable to patch 1 - 4.

Thanks

Re: [Qemu-devel] [PATCH 0/5] A few VLAN-related bugfixes for RX packet abstraction

[Dmitry Fleytman]

> On 17 Feb 2017, at 5:05, Jason Wang <jasowang@redhat.com> wrote:
> 
> 
> 
>> On 2017年02月16日 20:39, Peter Maydell wrote:
>>> On 16 February 2017 at 12:29, Dmitry Fleytman <dmitry@daynix.com> wrote:
>>> This series fix a few issues related
>>> to processing of RX packets with VLAN headers.
>>> 
>>> See commit messages of specific patches
>>> for information regarding affected devices.
>> I think at least the "fix buffer overrun" parts of this
>> patchset should be cc:stable ?
>> 
>> thanks
>> -- PMM
> 
> Yes, I add cc:stable to patch 1 - 4.

Thanks, Jason!

> 
> Thanks

Re: [Qemu-devel] [PATCH 5/5] NetRxPkt: Remove code duplication in net_rx_pkt_pull_data()

[Philippe Mathieu-Daudé]

On 02/16/2017 09:29 AM, Dmitry Fleytman wrote:
> This is a refactoring commit that does not change behavior.
>
> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/net/net_rx_pkt.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/hw/net/net_rx_pkt.c b/hw/net/net_rx_pkt.c
> index c7ae33d..3899211 100644
> --- a/hw/net/net_rx_pkt.c
> +++ b/hw/net/net_rx_pkt.c
> @@ -88,20 +88,21 @@ net_rx_pkt_pull_data(struct NetRxPkt *pkt,
>                          const struct iovec *iov, int iovcnt,
>                          size_t ploff)
>  {
> +    uint32_t pllen = iov_size(iov, iovcnt) - ploff;
> +
>      if (pkt->ehdr_buf_len) {
>          net_rx_pkt_iovec_realloc(pkt, iovcnt + 1);
>
>          pkt->vec[0].iov_base = pkt->ehdr_buf;
>          pkt->vec[0].iov_len = pkt->ehdr_buf_len;
>
> -        pkt->tot_len = iov_size(iov, iovcnt) - ploff + pkt->ehdr_buf_len;
> +        pkt->tot_len = pllen + 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) + 1;
> +                                iov, iovcnt, ploff, pllen) + 1;
>      } else {
>          net_rx_pkt_iovec_realloc(pkt, iovcnt);
>
> -        pkt->tot_len = iov_size(iov, iovcnt) - ploff;
> +        pkt->tot_len = pllen;
>          pkt->vec_len = iov_copy(pkt->vec, pkt->vec_len_total,
>                                  iov, iovcnt, ploff, pkt->tot_len);
>      }
>

Re: [Qemu-devel] [PATCH 0/5] A few VLAN-related bugfixes for RX packet abstraction

[Peter Maydell]

On 17 February 2017 at 03:04, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2017年02月16日 20:29, Dmitry Fleytman wrote:
>>
>> This series fix a few issues related
>> to processing of RX packets with VLAN headers.
>>
>> See commit messages of specific patches
>> for information regarding affected devices.
>>
>> Dmitry Fleytman (5):
>>    eth: Extend vlan stripping functions
>>    NetRxPkt: Fix memory corruption on VLAN header stripping
>>    NetRxPkt: Do not try to pull more data than present
>>    NetRxPkt: Account buffer with ETH header in IOV length
>>    NetRxPkt: Remove code duplication in net_rx_pkt_pull_data()
>>
>>   hw/net/net_rx_pkt.c | 40 +++++++++++++++++++++-------------------
>>   include/net/eth.h   |  4 ++--
>>   net/eth.c           | 25 ++++++++++++++-----------
>>   3 files changed, 37 insertions(+), 32 deletions(-)
>>
>
> Applied. Thanks

Hi Jason -- what's the status of these patches? I don't
see them in master and I don't think you have them in a pull
request yet (though I may have missed it in the deluge of
freeze-deadline pulls...)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 4/5] NetRxPkt: Account buffer with ETH header in IOV length

[Philippe Mathieu-Daudé]

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);
>
>

Re: [Qemu-devel] [PATCH 3/5] NetRxPkt: Do not try to pull more data than present

[Philippe Mathieu-Daudé]

this seems correct but unit-test would confirm any doubt.

On 02/16/2017 09:29 AM, Dmitry Fleytman wrote:
> In case of VLAN stripping, ETH header put into a
> separate buffer, therefore amont of data copied
> from original IOV should be smaller.
>
> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/net/net_rx_pkt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/net_rx_pkt.c b/hw/net/net_rx_pkt.c
> index 7c0beac..d38babe 100644
> --- a/hw/net/net_rx_pkt.c
> +++ b/hw/net/net_rx_pkt.c
> @@ -96,7 +96,8 @@ 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);
> +                                iov, iovcnt, ploff,
> +                                pkt->tot_len - pkt->ehdr_buf_len);
>      } else {
>          net_rx_pkt_iovec_realloc(pkt, iovcnt);
>
>

Re: [Qemu-devel] [PATCH 1/5] eth: Extend vlan stripping functions

[Philippe Mathieu-Daudé]

On 02/16/2017 09:29 AM, Dmitry Fleytman wrote:
> Make VLAN stripping functions return number of bytes
> copied to given Ethernet header buffer.
>
> This information should be used to re-compose
> packet IOV after VLAN stripping.
>
> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
> ---
>  include/net/eth.h |  4 ++--
>  net/eth.c         | 25 ++++++++++++++-----------
>  2 files changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/include/net/eth.h b/include/net/eth.h
> index 2013175..afeb45b 100644
> --- a/include/net/eth.h
> +++ b/include/net/eth.h
> @@ -331,12 +331,12 @@ eth_get_pkt_tci(const void *p)
>      }
>  }
>
> -bool
> +size_t
>  eth_strip_vlan(const struct iovec *iov, int iovcnt, size_t iovoff,
>                 uint8_t *new_ehdr_buf,
>                 uint16_t *payload_offset, uint16_t *tci);
>
> -bool
> +size_t
>  eth_strip_vlan_ex(const struct iovec *iov, int iovcnt, size_t iovoff,
>                    uint16_t vet, uint8_t *new_ehdr_buf,
>                    uint16_t *payload_offset, uint16_t *tci);
> diff --git a/net/eth.c b/net/eth.c
> index df81efb..5b9ba26 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -232,7 +232,7 @@ void eth_get_protocols(const struct iovec *iov, int iovcnt,
>      }
>  }
>
> -bool
> +size_t
>  eth_strip_vlan(const struct iovec *iov, int iovcnt, size_t iovoff,
>                 uint8_t *new_ehdr_buf,
>                 uint16_t *payload_offset, uint16_t *tci)
> @@ -244,7 +244,7 @@ eth_strip_vlan(const struct iovec *iov, int iovcnt, size_t iovoff,
>                                 new_ehdr, sizeof(*new_ehdr));
>
>      if (copied < sizeof(*new_ehdr)) {
> -        return false;
> +        return 0;
>      }
>
>      switch (be16_to_cpu(new_ehdr->h_proto)) {
> @@ -254,7 +254,7 @@ eth_strip_vlan(const struct iovec *iov, int iovcnt, size_t iovoff,
>                              &vlan_hdr, sizeof(vlan_hdr));
>
>          if (copied < sizeof(vlan_hdr)) {
> -            return false;
> +            return 0;
>          }
>
>          new_ehdr->h_proto = vlan_hdr.h_proto;
> @@ -268,18 +268,21 @@ eth_strip_vlan(const struct iovec *iov, int iovcnt, size_t iovoff,
>                                  PKT_GET_VLAN_HDR(new_ehdr), sizeof(vlan_hdr));
>
>              if (copied < sizeof(vlan_hdr)) {
> -                return false;
> +                return 0;
>              }
>
>              *payload_offset += sizeof(vlan_hdr);
> +
> +            return sizeof(struct eth_header) + sizeof(struct vlan_header);
> +        } else {
> +            return sizeof(struct eth_header);

remove this "else return ..."

>          }
> -        return true;

here avoid fall-through with 'return sizeof(struct eth_header);'

>      default:
> -        return false;
> +        return 0;
>      }
>  }
>
> -bool
> +size_t
>  eth_strip_vlan_ex(const struct iovec *iov, int iovcnt, size_t iovoff,
>                    uint16_t vet, uint8_t *new_ehdr_buf,
>                    uint16_t *payload_offset, uint16_t *tci)
> @@ -291,7 +294,7 @@ eth_strip_vlan_ex(const struct iovec *iov, int iovcnt, size_t iovoff,
>                                 new_ehdr, sizeof(*new_ehdr));
>
>      if (copied < sizeof(*new_ehdr)) {
> -        return false;
> +        return 0;
>      }
>
>      if (be16_to_cpu(new_ehdr->h_proto) == vet) {
> @@ -299,17 +302,17 @@ eth_strip_vlan_ex(const struct iovec *iov, int iovcnt, size_t iovoff,
>                              &vlan_hdr, sizeof(vlan_hdr));
>
>          if (copied < sizeof(vlan_hdr)) {
> -            return false;
> +            return 0;
>          }
>
>          new_ehdr->h_proto = vlan_hdr.h_proto;
>
>          *tci = be16_to_cpu(vlan_hdr.h_tci);
>          *payload_offset = iovoff + sizeof(*new_ehdr) + sizeof(vlan_hdr);
> -        return true;
> +        return sizeof(struct eth_header);
>      }
>
> -    return false;
> +    return 0;
>  }
>
>  void
>

with changes:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Phil.

Re: [Qemu-devel] [PATCH 0/5] A few VLAN-related bugfixes for RX packet abstraction

[Jason Wang]

On 2017年03月03日 20:06, Peter Maydell wrote:
> On 17 February 2017 at 03:04, Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2017年02月16日 20:29, Dmitry Fleytman wrote:
>>> This series fix a few issues related
>>> to processing of RX packets with VLAN headers.
>>>
>>> See commit messages of specific patches
>>> for information regarding affected devices.
>>>
>>> Dmitry Fleytman (5):
>>>     eth: Extend vlan stripping functions
>>>     NetRxPkt: Fix memory corruption on VLAN header stripping
>>>     NetRxPkt: Do not try to pull more data than present
>>>     NetRxPkt: Account buffer with ETH header in IOV length
>>>     NetRxPkt: Remove code duplication in net_rx_pkt_pull_data()
>>>
>>>    hw/net/net_rx_pkt.c | 40 +++++++++++++++++++++-------------------
>>>    include/net/eth.h   |  4 ++--
>>>    net/eth.c           | 25 ++++++++++++++-----------
>>>    3 files changed, 37 insertions(+), 32 deletions(-)
>>>
>> Applied. Thanks
> Hi Jason -- what's the status of these patches? I don't
> see them in master and I don't think you have them in a pull
> request yet (though I may have missed it in the deluge of
> freeze-deadline pulls...)
>
> thanks
> -- PMM
>

Already queued but haven't sent the pull request.

Plan to send it next Monday.

Thanks

Re: [Qemu-devel] [PATCH 4/5] NetRxPkt: Account buffer with ETH header in IOV length

[Dmitry Fleytman]

> 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);

Re: [Qemu-devel] [PATCH 1/5] eth: Extend vlan stripping functions

[Dmitry Fleytman]

> On 3 Mar 2017, at 18:52 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 
> On 02/16/2017 09:29 AM, Dmitry Fleytman wrote:
>> Make VLAN stripping functions return number of bytes
>> copied to given Ethernet header buffer.
>> 
>> This information should be used to re-compose
>> packet IOV after VLAN stripping.
>> 
>> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
>> ---
>> include/net/eth.h |  4 ++--
>> net/eth.c         | 25 ++++++++++++++-----------
>> 2 files changed, 16 insertions(+), 13 deletions(-)
>> 
>> diff --git a/include/net/eth.h b/include/net/eth.h
>> index 2013175..afeb45b 100644
>> --- a/include/net/eth.h
>> +++ b/include/net/eth.h
>> @@ -331,12 +331,12 @@ eth_get_pkt_tci(const void *p)
>>     }
>> }
>> 
>> -bool
>> +size_t
>> eth_strip_vlan(const struct iovec *iov, int iovcnt, size_t iovoff,
>>                uint8_t *new_ehdr_buf,
>>                uint16_t *payload_offset, uint16_t *tci);
>> 
>> -bool
>> +size_t
>> eth_strip_vlan_ex(const struct iovec *iov, int iovcnt, size_t iovoff,
>>                   uint16_t vet, uint8_t *new_ehdr_buf,
>>                   uint16_t *payload_offset, uint16_t *tci);
>> diff --git a/net/eth.c b/net/eth.c
>> index df81efb..5b9ba26 100644
>> --- a/net/eth.c
>> +++ b/net/eth.c
>> @@ -232,7 +232,7 @@ void eth_get_protocols(const struct iovec *iov, int iovcnt,
>>     }
>> }
>> 
>> -bool
>> +size_t
>> eth_strip_vlan(const struct iovec *iov, int iovcnt, size_t iovoff,
>>                uint8_t *new_ehdr_buf,
>>                uint16_t *payload_offset, uint16_t *tci)
>> @@ -244,7 +244,7 @@ eth_strip_vlan(const struct iovec *iov, int iovcnt, size_t iovoff,
>>                                new_ehdr, sizeof(*new_ehdr));
>> 
>>     if (copied < sizeof(*new_ehdr)) {
>> -        return false;
>> +        return 0;
>>     }
>> 
>>     switch (be16_to_cpu(new_ehdr->h_proto)) {
>> @@ -254,7 +254,7 @@ eth_strip_vlan(const struct iovec *iov, int iovcnt, size_t iovoff,
>>                             &vlan_hdr, sizeof(vlan_hdr));
>> 
>>         if (copied < sizeof(vlan_hdr)) {
>> -            return false;
>> +            return 0;
>>         }
>> 
>>         new_ehdr->h_proto = vlan_hdr.h_proto;
>> @@ -268,18 +268,21 @@ eth_strip_vlan(const struct iovec *iov, int iovcnt, size_t iovoff,
>>                                 PKT_GET_VLAN_HDR(new_ehdr), sizeof(vlan_hdr));
>> 
>>             if (copied < sizeof(vlan_hdr)) {
>> -                return false;
>> +                return 0;
>>             }
>> 
>>             *payload_offset += sizeof(vlan_hdr);
>> +
>> +            return sizeof(struct eth_header) + sizeof(struct vlan_header);
>> +        } else {
>> +            return sizeof(struct eth_header);
> 
> remove this "else return ..."
> 
>>         }
>> -        return true;
> 
> here avoid fall-through with 'return sizeof(struct eth_header);’


Hi Philippe,

IIUYC there is no functional difference, just a matter of style. Am I right?

As for me current code is more explicit so I would leave it as is unless
there is a specific project coding style requirement.

~Dmitry

> 
>>     default:
>> -        return false;
>> +        return 0;
>>     }
>> }
>> 
>> -bool
>> +size_t
>> eth_strip_vlan_ex(const struct iovec *iov, int iovcnt, size_t iovoff,
>>                   uint16_t vet, uint8_t *new_ehdr_buf,
>>                   uint16_t *payload_offset, uint16_t *tci)
>> @@ -291,7 +294,7 @@ eth_strip_vlan_ex(const struct iovec *iov, int iovcnt, size_t iovoff,
>>                                new_ehdr, sizeof(*new_ehdr));
>> 
>>     if (copied < sizeof(*new_ehdr)) {
>> -        return false;
>> +        return 0;
>>     }
>> 
>>     if (be16_to_cpu(new_ehdr->h_proto) == vet) {
>> @@ -299,17 +302,17 @@ eth_strip_vlan_ex(const struct iovec *iov, int iovcnt, size_t iovoff,
>>                             &vlan_hdr, sizeof(vlan_hdr));
>> 
>>         if (copied < sizeof(vlan_hdr)) {
>> -            return false;
>> +            return 0;
>>         }
>> 
>>         new_ehdr->h_proto = vlan_hdr.h_proto;
>> 
>>         *tci = be16_to_cpu(vlan_hdr.h_tci);
>>         *payload_offset = iovoff + sizeof(*new_ehdr) + sizeof(vlan_hdr);
>> -        return true;
>> +        return sizeof(struct eth_header);
>>     }
>> 
>> -    return false;
>> +    return 0;
>> }
>> 
>> void
>> 
> 
> with changes:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org <mailto:f4bug@amsat.org>>
> 
> Phil.