[ovs-dev,1/1] flow: Fix parsing l3_ofs with partial offloading
diff mbox series

Message ID 20200114132115.21694-1-elibr@mellanox.com
State New
Headers show
Series
  • [ovs-dev,1/1] flow: Fix parsing l3_ofs with partial offloading
Related show

Commit Message

Eli Britstein Jan. 14, 2020, 1:21 p.m. UTC
l3_ofs should be set all Ethernet packets, not just IPv4/IPv6 ones.
For example for ARP over VLAN tagged packets, it may cause wrong
processing like in changing the VLAN ID action. Fix it.

Fixes: aab96ec4d81e ("dpif-netdev: retrieve flow directly from the flow mark")
Signed-off-by: Eli Britstein <elibr@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
---
 lib/flow.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Eli Britstein Jan. 29, 2020, 7:59 a.m. UTC | #1
ping

On 1/14/2020 3:21 PM, Eli Britstein wrote:
> l3_ofs should be set all Ethernet packets, not just IPv4/IPv6 ones.
> For example for ARP over VLAN tagged packets, it may cause wrong
> processing like in changing the VLAN ID action. Fix it.
>
> Fixes: aab96ec4d81e ("dpif-netdev: retrieve flow directly from the flow mark")
> Signed-off-by: Eli Britstein <elibr@mellanox.com>
> Reviewed-by: Roi Dayan <roid@mellanox.com>
> ---
>   lib/flow.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/lib/flow.c b/lib/flow.c
> index 45bb96b54..5c32b4a01 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -1107,6 +1107,7 @@ parse_tcp_flags(struct dp_packet *packet)
>       if (OVS_UNLIKELY(eth_type_mpls(dl_type))) {
>           packet->l2_5_ofs = (char *)data - frame;
>       }
> +    packet->l3_ofs = (char *)data - frame;
>       if (OVS_LIKELY(dl_type == htons(ETH_TYPE_IP))) {
>           const struct ip_header *nh = data;
>           int ip_len;
> @@ -1116,7 +1117,6 @@ parse_tcp_flags(struct dp_packet *packet)
>               return 0;
>           }
>           dp_packet_set_l2_pad_size(packet, size - tot_len);
> -        packet->l3_ofs = (uint16_t)((char *)nh - frame);
>           nw_proto = nh->ip_proto;
>           nw_frag = ipv4_get_nw_frag(nh);
>   
> @@ -1129,7 +1129,6 @@ parse_tcp_flags(struct dp_packet *packet)
>           if (OVS_UNLIKELY(!ipv6_sanity_check(nh, size))) {
>               return 0;
>           }
> -        packet->l3_ofs = (uint16_t)((char *)nh - frame);
>           data_pull(&data, &size, sizeof *nh);
>   
>           plen = ntohs(nh->ip6_plen); /* Never pull padding. */
Ilya Maximets Jan. 30, 2020, 11:28 a.m. UTC | #2
On 14.01.2020 14:21, Eli Britstein wrote:
> l3_ofs should be set all Ethernet packets, not just IPv4/IPv6 ones.
> For example for ARP over VLAN tagged packets, it may cause wrong
> processing like in changing the VLAN ID action. Fix it.
> 
> Fixes: aab96ec4d81e ("dpif-netdev: retrieve flow directly from the flow mark")
> Signed-off-by: Eli Britstein <elibr@mellanox.com>
> Reviewed-by: Roi Dayan <roid@mellanox.com>
> ---

Thanks, Eli.  This seems correct to me.  I'm traveling now, will be able
to check on next week if Ben will not get to this earlier.

BTW, it might be good to have a unit test for this.  We already have
a couple of such partial offloading tests.

Best regards, Ilya Maximets.
Eli Britstein Jan. 30, 2020, 3:26 p.m. UTC | #3
On 1/30/2020 1:28 PM, Ilya Maximets wrote:
> On 14.01.2020 14:21, Eli Britstein wrote:
>> l3_ofs should be set all Ethernet packets, not just IPv4/IPv6 ones.
>> For example for ARP over VLAN tagged packets, it may cause wrong
>> processing like in changing the VLAN ID action. Fix it.
>>
>> Fixes: aab96ec4d81e ("dpif-netdev: retrieve flow directly from the flow mark")
>> Signed-off-by: Eli Britstein <elibr@mellanox.com>
>> Reviewed-by: Roi Dayan <roid@mellanox.com>
>> ---
> Thanks, Eli.  This seems correct to me.  I'm traveling now, will be able
> to check on next week if Ben will not get to this earlier.
>
> BTW, it might be good to have a unit test for this.  We already have
> a couple of such partial offloading tests.
Could you please point me to where? Note that in order to hit this issue 
we really need a supporting HW.
>
> Best regards, Ilya Maximets.
Ilya Maximets Jan. 30, 2020, 9:12 p.m. UTC | #4
On Thu, Jan 30, 2020 at 4:26 PM Eli Britstein <elibr@mellanox.com> wrote:
> On 1/30/2020 1:28 PM, Ilya Maximets wrote:
> > On 14.01.2020 14:21, Eli Britstein wrote:
> >> l3_ofs should be set all Ethernet packets, not just IPv4/IPv6 ones.
> >> For example for ARP over VLAN tagged packets, it may cause wrong
> >> processing like in changing the VLAN ID action. Fix it.
> >>
> >> Fixes: aab96ec4d81e ("dpif-netdev: retrieve flow directly from the flow mark")
> >> Signed-off-by: Eli Britstein <elibr@mellanox.com>
> >> Reviewed-by: Roi Dayan <roid@mellanox.com>
> >> ---
> > Thanks, Eli.  This seems correct to me.  I'm traveling now, will be able
> > to check on next week if Ben will not get to this earlier.
> >
> > BTW, it might be good to have a unit test for this.  We already have
> > a couple of such partial offloading tests.
> Could you please point me to where? Note that in order to hit this issue
> we really need a supporting HW.

Take a look at this patch:
 54e2baec09d2 ("flow: Fix crash on vlan packets with partial offloading.")

And you don't need to have any hardware to test partial offloading since we
have  dummy offload provider.
Ben Pfaff Jan. 31, 2020, 5:04 p.m. UTC | #5
On Thu, Jan 30, 2020 at 12:28:39PM +0100, Ilya Maximets wrote:
> On 14.01.2020 14:21, Eli Britstein wrote:
> > l3_ofs should be set all Ethernet packets, not just IPv4/IPv6 ones.
> > For example for ARP over VLAN tagged packets, it may cause wrong
> > processing like in changing the VLAN ID action. Fix it.
> > 
> > Fixes: aab96ec4d81e ("dpif-netdev: retrieve flow directly from the flow mark")
> > Signed-off-by: Eli Britstein <elibr@mellanox.com>
> > Reviewed-by: Roi Dayan <roid@mellanox.com>
> > ---
> 
> Thanks, Eli.  This seems correct to me.  I'm traveling now, will be able
> to check on next week if Ben will not get to this earlier.
> 
> BTW, it might be good to have a unit test for this.  We already have
> a couple of such partial offloading tests.

I applied this to master and backported it.

A test would be nice!

Patch
diff mbox series

diff --git a/lib/flow.c b/lib/flow.c
index 45bb96b54..5c32b4a01 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -1107,6 +1107,7 @@  parse_tcp_flags(struct dp_packet *packet)
     if (OVS_UNLIKELY(eth_type_mpls(dl_type))) {
         packet->l2_5_ofs = (char *)data - frame;
     }
+    packet->l3_ofs = (char *)data - frame;
     if (OVS_LIKELY(dl_type == htons(ETH_TYPE_IP))) {
         const struct ip_header *nh = data;
         int ip_len;
@@ -1116,7 +1117,6 @@  parse_tcp_flags(struct dp_packet *packet)
             return 0;
         }
         dp_packet_set_l2_pad_size(packet, size - tot_len);
-        packet->l3_ofs = (uint16_t)((char *)nh - frame);
         nw_proto = nh->ip_proto;
         nw_frag = ipv4_get_nw_frag(nh);
 
@@ -1129,7 +1129,6 @@  parse_tcp_flags(struct dp_packet *packet)
         if (OVS_UNLIKELY(!ipv6_sanity_check(nh, size))) {
             return 0;
         }
-        packet->l3_ofs = (uint16_t)((char *)nh - frame);
         data_pull(&data, &size, sizeof *nh);
 
         plen = ntohs(nh->ip6_plen); /* Never pull padding. */