Re: [ovs-dev] [PATCH net 1/1] openvswitch: Fix setting ipv6 fields causing hw csum failure
On Thu, 10 Feb 2022, Jakub Kicinski wrote: > On Thu, 10 Feb 2022 10:53:24 +0200 Paul Blakey wrote: > > > The calls seem a little heavy for single byte replacements. > > > Can you instead add a helper based on csum_replace4() maybe? > > > > > > BTW doesn't pedit have the same problem? > > > > I don't think they are heavier then csum_replace4, > > csum_replace4 is a handful of instructions all of which will be inlined. > csum_partial() is a function call and handles variable lengths. > > > but they are more bulletproof in my opinion, since they handle both > > the COMPLETE and PARTIAL csum cases (in __skb_postpull_rcsum()) > > Yes, that's why I said "add a helper based on", a skb helper which > checks the csum type of the packet but instead of calling csum_partial > for no reason does the adjustment directly. Then sure, I will do that and send v2. > > > and resemble what editing of the packet should have done - pull the > > header, edit, and then push it back. > > That's not what this code is doing, so the argument does not stand IMO. > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net 1/1] openvswitch: Fix setting ipv6 fields causing hw csum failure
On Thu, 10 Feb 2022 10:53:24 +0200 Paul Blakey wrote: > > The calls seem a little heavy for single byte replacements. > > Can you instead add a helper based on csum_replace4() maybe? > > > > BTW doesn't pedit have the same problem? > > I don't think they are heavier then csum_replace4, csum_replace4 is a handful of instructions all of which will be inlined. csum_partial() is a function call and handles variable lengths. > but they are more bulletproof in my opinion, since they handle both > the COMPLETE and PARTIAL csum cases (in __skb_postpull_rcsum()) Yes, that's why I said "add a helper based on", a skb helper which checks the csum type of the packet but instead of calling csum_partial for no reason does the adjustment directly. > and resemble what editing of the packet should have done - pull the > header, edit, and then push it back. That's not what this code is doing, so the argument does not stand IMO. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net 1/1] openvswitch: Fix setting ipv6 fields causing hw csum failure
On Wed, 9 Feb 2022, Jakub Kicinski wrote: > On Mon, 7 Feb 2022 16:41:01 +0200 Paul Blakey wrote: > > Ipv6 ttl, label and tos fields are modified without first > > pulling/pushing the ipv6 header, which would have updated > > the hw csum (if available). This might cause csum validation > > when sending the packet to the stack, as can be seen in > > the trace below. > > > > Fix this by calling postpush/postpull checksum calculation > > which will update the hw csum if needed. > > > -static void set_ipv6_fl(struct ipv6hdr *nh, u32 fl, u32 mask) > > +static void set_ipv6_dsfield(struct sk_buff *skb, struct ipv6hdr *nh, __u8 > > ipv6_tclass, __u8 mask) > > { > > + skb_postpull_rcsum(skb, nh, 4); > > + > > + ipv6_change_dsfield(nh, ~mask, ipv6_tclass); > > + > > + skb_postpush_rcsum(skb, nh, 4); > > +} > > The calls seem a little heavy for single byte replacements. > Can you instead add a helper based on csum_replace4() maybe? > > BTW doesn't pedit have the same problem? > I don't think they are heavier then csum_replace4, but they are more bulletproof in my opinion, since they handle both the COMPLETE and PARTIAL csum cases (in __skb_postpull_rcsum()) and resemble what editing of the packet should have done - pull the header, edit, and then push it back. RE pedit, not really the issue here, but i guess pedit should be followed by act_csum with the relevant checksum fields (as we do when offloading ovs set() action to tc pedit + csum actions). ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net 1/1] openvswitch: Fix setting ipv6 fields causing hw csum failure
On Mon, 7 Feb 2022 16:41:01 +0200 Paul Blakey wrote: > Ipv6 ttl, label and tos fields are modified without first > pulling/pushing the ipv6 header, which would have updated > the hw csum (if available). This might cause csum validation > when sending the packet to the stack, as can be seen in > the trace below. > > Fix this by calling postpush/postpull checksum calculation > which will update the hw csum if needed. > -static void set_ipv6_fl(struct ipv6hdr *nh, u32 fl, u32 mask) > +static void set_ipv6_dsfield(struct sk_buff *skb, struct ipv6hdr *nh, __u8 > ipv6_tclass, __u8 mask) > { > + skb_postpull_rcsum(skb, nh, 4); > + > + ipv6_change_dsfield(nh, ~mask, ipv6_tclass); > + > + skb_postpush_rcsum(skb, nh, 4); > +} The calls seem a little heavy for single byte replacements. Can you instead add a helper based on csum_replace4() maybe? BTW doesn't pedit have the same problem? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH net 1/1] openvswitch: Fix setting ipv6 fields causing hw csum failure
Ipv6 ttl, label and tos fields are modified without first pulling/pushing the ipv6 header, which would have updated the hw csum (if available). This might cause csum validation when sending the packet to the stack, as can be seen in the trace below. Fix this by calling postpush/postpull checksum calculation which will update the hw csum if needed. Trace resulted by ipv6 ttl dec and then sending packet to conntrack [actions: set(ipv6(hlimit=63)),ct(zone=99)]: [295241.900063] s_pf0vf2: hw csum failure [295241.923191] Call Trace: [295241.925728] [295241.927836] dump_stack+0x5c/0x80 [295241.931240] __skb_checksum_complete+0xac/0xc0 [295241.935778] nf_conntrack_tcp_packet+0x398/0xba0 [nf_conntrack] [295241.953030] nf_conntrack_in+0x498/0x5e0 [nf_conntrack] [295241.958344] __ovs_ct_lookup+0xac/0x860 [openvswitch] [295241.968532] ovs_ct_execute+0x4a7/0x7c0 [openvswitch] [295241.979167] do_execute_actions+0x54a/0xaa0 [openvswitch] [295242.001482] ovs_execute_actions+0x48/0x100 [openvswitch] [295242.006966] ovs_dp_process_packet+0x96/0x1d0 [openvswitch] [295242.012626] ovs_vport_receive+0x6c/0xc0 [openvswitch] [295242.028763] netdev_frame_hook+0xc0/0x180 [openvswitch] [295242.034074] __netif_receive_skb_core+0x2ca/0xcb0 [295242.047498] netif_receive_skb_internal+0x3e/0xc0 [295242.052291] napi_gro_receive+0xba/0xe0 [295242.056231] mlx5e_handle_rx_cqe_mpwrq_rep+0x12b/0x250 [mlx5_core] [295242.062513] mlx5e_poll_rx_cq+0xa0f/0xa30 [mlx5_core] [295242.067669] mlx5e_napi_poll+0xe1/0x6b0 [mlx5_core] [295242.077958] net_rx_action+0x149/0x3b0 [295242.086762] __do_softirq+0xd7/0x2d6 [295242.090427] irq_exit+0xf7/0x100 [295242.093748] do_IRQ+0x7f/0xd0 [295242.096806] common_interrupt+0xf/0xf [295242.100559] [295242.102750] RIP: 0033:0x7f9022e88cbd [295242.125246] RSP: 002b:7f9022282b20 EFLAGS: 0246 ORIG_RAX: ffda [295242.132900] RAX: 0005 RBX: 0010 RCX: [295242.140120] RDX: 7f9022282ba8 RSI: 7f9022282a30 RDI: 7f9014005c30 [295242.147337] RBP: 7f9014014d60 R08: 0020 R09: 7f90254a8340 [295242.154557] R10: 7f9022282a28 R11: 0246 R12: [295242.161775] R13: 7f902308c000 R14: 002b R15: 7f9022b71f40 Fixes: 3fdbd1ce11e5 ("openvswitch: add ipv6 'set' action") Signed-off-by: Paul Blakey --- net/openvswitch/actions.c | 29 - 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c index 076774034bb9..77b99dc82f95 100644 --- a/net/openvswitch/actions.c +++ b/net/openvswitch/actions.c @@ -423,12 +423,32 @@ static void set_ipv6_addr(struct sk_buff *skb, u8 l4_proto, memcpy(addr, new_addr, sizeof(__be32[4])); } -static void set_ipv6_fl(struct ipv6hdr *nh, u32 fl, u32 mask) +static void set_ipv6_dsfield(struct sk_buff *skb, struct ipv6hdr *nh, __u8 ipv6_tclass, __u8 mask) { + skb_postpull_rcsum(skb, nh, 4); + + ipv6_change_dsfield(nh, ~mask, ipv6_tclass); + + skb_postpush_rcsum(skb, nh, 4); +} + +static void set_ipv6_fl(struct sk_buff *skb, struct ipv6hdr *nh, u32 fl, u32 mask) +{ + skb_postpull_rcsum(skb, nh, 4); + /* Bits 21-24 are always unmasked, so this retains their values. */ OVS_SET_MASKED(nh->flow_lbl[0], (u8)(fl >> 16), (u8)(mask >> 16)); OVS_SET_MASKED(nh->flow_lbl[1], (u8)(fl >> 8), (u8)(mask >> 8)); OVS_SET_MASKED(nh->flow_lbl[2], (u8)fl, (u8)mask); + + skb_postpush_rcsum(skb, nh, 4); +} + +static void set_ipv6_ttl(struct sk_buff *skb, struct ipv6hdr *nh, u8 new_ttl, u8 mask) +{ + __skb_postpull_rcsum(skb, >hop_limit, sizeof(nh->hop_limit), 1); + OVS_SET_MASKED(nh->hop_limit, new_ttl, mask); + __skb_postpush_rcsum(skb, >hop_limit, sizeof(nh->hop_limit), 1); } static void set_ip_ttl(struct sk_buff *skb, struct iphdr *nh, u8 new_ttl, @@ -546,18 +566,17 @@ static int set_ipv6(struct sk_buff *skb, struct sw_flow_key *flow_key, } } if (mask->ipv6_tclass) { - ipv6_change_dsfield(nh, ~mask->ipv6_tclass, key->ipv6_tclass); + set_ipv6_dsfield(skb, nh, key->ipv6_tclass, mask->ipv6_tclass); flow_key->ip.tos = ipv6_get_dsfield(nh); } if (mask->ipv6_label) { - set_ipv6_fl(nh, ntohl(key->ipv6_label), + set_ipv6_fl(skb, nh, ntohl(key->ipv6_label), ntohl(mask->ipv6_label)); flow_key->ipv6.label = *(__be32 *)nh & htonl(IPV6_FLOWINFO_FLOWLABEL); } if (mask->ipv6_hlimit) { - OVS_SET_MASKED(nh->hop_limit, key->ipv6_hlimit, - mask->ipv6_hlimit); + set_ipv6_ttl(skb, nh, key->ipv6_hlimit, mask->ipv6_hlimit); flow_key->ip.ttl = nh->hop_limit; } return 0; -- 2.30.1