Re: [ovs-dev] [PATCH net v2 1/1] openvswitch: Fix setting ipv6 fields causing hw csum failure

2022-02-14 Thread Jakub Kicinski
On Mon, 14 Feb 2022 14:48:51 +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 updating skb->csum if available.

> Fixes: 3fdbd1ce11e5 ("openvswitch: add ipv6 'set' action")
> Signed-off-by: Paul Blakey 
> ---
>  Changelog:
> v1->v2:
>   Replaced push pull rcsum checksum calc with actual checksum calc

Well, what I had in mind was an skb helper which would take skb, u32
new_val, u32 old_val, but this works, too.

Please fix the new sparse warnings tho, and..

> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 076774034bb9..3725801cb040 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -423,12 +423,44 @@ 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)
>  {
> + __u8 old_ipv6_tclass = ipv6_get_dsfield(nh);

use normal u8, __u8 is for uAPI.

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


[ovs-dev] [PATCH net v2 1/1] openvswitch: Fix setting ipv6 fields causing hw csum failure

2022-02-14 Thread Paul Blakey via dev
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 updating skb->csum if available.

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 
---
 Changelog:
v1->v2:
  Replaced push pull rcsum checksum calc with actual checksum calc
 
 net/openvswitch/actions.c | 47 ---
 1 file changed, 39 insertions(+), 8 deletions(-)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 076774034bb9..3725801cb040 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -423,12 +423,44 @@ 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)
 {
+   __u8 old_ipv6_tclass = ipv6_get_dsfield(nh);
+
+   ipv6_tclass = OVS_MASKED(old_ipv6_tclass, ipv6_tclass, mask);
+
+   if (skb->ip_summed == CHECKSUM_COMPLETE)
+   skb->csum = ~csum_block_add(csum_block_sub(~skb->csum, 
ipv6_tclass << 4, 1),
+   old_ipv6_tclass << 4, 1);
+
+   ipv6_change_dsfield(nh, ~mask, ipv6_tclass);
+}
+
+static void set_ipv6_fl(struct sk_buff *skb, struct ipv6hdr *nh, u32 fl, u32 
mask)
+{
+   u32 old_fl;
+
+   old_fl = nh->flow_lbl[0] << 16 |  nh->flow_lbl[1] << 8 |  
nh->flow_lbl[2];
+   fl = OVS_MASKED(old_fl, fl, mask);
+
/* 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);
+   nh->flow_lbl[0] = (u8)(fl >> 16);
+   nh->flow_lbl[1] = (u8)(fl >> 8);
+   nh->flow_lbl[2] = (u8)fl;
+
+   if (skb->ip_summed == CHECKSUM_COMPLETE)
+   skb->csum = ~csum_block_add(csum_block_sub(~skb->csum, 
htonl(fl), 0),
+   htonl(old_fl), 0);
+}
+
+static void set_ipv6_ttl(struct sk_buff *skb, struct ipv6hdr *nh, u8 new_ttl, 
u8 mask)
+{
+   new_ttl = OVS_MASKED(nh->hop_limit, new_ttl, mask);
+
+   if (skb->ip_summed == CHECKSUM_COMPLETE)
+   skb->csum = ~csum_block_add(csum_block_sub(~skb->csum, new_ttl, 
1),
+   nh->hop_limit, 1);
+   nh->hop_limit = new_ttl;
 }
 
 static void set_ip_ttl(struct sk_buff *skb, struct iphdr *nh, u8 new_ttl,
@@ -546,18 +578,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);
+