| Message ID | 20200114132115.21694-1-elibr@mellanox.com |
|---|---|
| State | Accepted |
| Delegated to: | Ben Pfaff |
| Headers | show |
| Series | [ovs-dev,1/1] flow: Fix parsing l3_ofs with partial offloading | expand |
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. */
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.
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.
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.
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!
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. */
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(-)