All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net] xfrm: calculate L4 checksums also for GSO case before encrypting packets
@ 2017-04-10 18:42 Ansis Atteka
  2017-04-11  7:07 ` Steffen Klassert
  0 siblings, 1 reply; 8+ messages in thread
From: Ansis Atteka @ 2017-04-10 18:42 UTC (permalink / raw)
  To: netdev; +Cc: Ansis Atteka

Otherwise, if L4 checksum calculation is done after encryption,
then all ESP packets end up being corrupted at the location
where pre-encryption L4 checksum field resides.

One of the ways to reproduce this bug is to have a VM with virtio_net
driver (UFO set to ON in the guest VM); and then encapsulate all guest's
Ethernet frames in GENEVE; and then further encrypt GENEVE with IPsec.
In this case following symptoms are observed:
1. If using ixgbe NIC, then the driver will also emit following
   warning message:
   ixgbe 0000:01:00.1: partial checksum but l4 proto=32!
2. Receiving VM will drop all the corrupted ESP packets, hence UDP iperf test
   with large packets will fail completely or TCP iperf will get ridiculously
   low performance because TCP window will never grow above MTU.

Signed-off-by: Ansis Atteka <aatteka@ovn.org>
---
 net/xfrm/xfrm_output.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
index 8ba29fe..7ad7e5f 100644
--- a/net/xfrm/xfrm_output.c
+++ b/net/xfrm/xfrm_output.c
@@ -168,7 +168,8 @@ static int xfrm_output2(struct net *net, struct sock *sk, struct sk_buff *skb)
 
 static int xfrm_output_gso(struct net *net, struct sock *sk, struct sk_buff *skb)
 {
-	struct sk_buff *segs;
+	struct sk_buff *segs, *nskb;
+	int err;
 
 	BUILD_BUG_ON(sizeof(*IPCB(skb)) > SKB_SGO_CB_OFFSET);
 	BUILD_BUG_ON(sizeof(*IP6CB(skb)) > SKB_SGO_CB_OFFSET);
@@ -180,21 +181,27 @@ static int xfrm_output_gso(struct net *net, struct sock *sk, struct sk_buff *skb
 		return -EINVAL;
 
 	do {
-		struct sk_buff *nskb = segs->next;
-		int err;
+		nskb = segs->next;
 
 		segs->next = NULL;
-		err = xfrm_output2(net, sk, segs);
+		err = skb_checksum_help(segs);
+		if (unlikely(err)) {
+			XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
+			goto error;
+		}
 
+		err = xfrm_output2(net, sk, segs);
 		if (unlikely(err)) {
-			kfree_skb_list(nskb);
-			return err;
+			goto error;
 		}
 
 		segs = nskb;
 	} while (segs);
 
 	return 0;
+error:
+	kfree_skb_list(nskb);
+	return err;
 }
 
 int xfrm_output(struct sock *sk, struct sk_buff *skb)
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH net] xfrm: calculate L4 checksums also for GSO case before encrypting packets
  2017-04-10 18:42 [PATCH net] xfrm: calculate L4 checksums also for GSO case before encrypting packets Ansis Atteka
@ 2017-04-11  7:07 ` Steffen Klassert
       [not found]   ` <CAMQa7Biajree5Kc1fOWQN42R1UDDGp7ZevZZRtUMZOKDWTk-Vw@mail.gmail.com>
  0 siblings, 1 reply; 8+ messages in thread
From: Steffen Klassert @ 2017-04-11  7:07 UTC (permalink / raw)
  To: Ansis Atteka; +Cc: netdev

On Mon, Apr 10, 2017 at 11:42:07AM -0700, Ansis Atteka wrote:
> Otherwise, if L4 checksum calculation is done after encryption,
> then all ESP packets end up being corrupted at the location
> where pre-encryption L4 checksum field resides.
> 
> One of the ways to reproduce this bug is to have a VM with virtio_net
> driver (UFO set to ON in the guest VM); and then encapsulate all guest's
> Ethernet frames in GENEVE; and then further encrypt GENEVE with IPsec.
> In this case following symptoms are observed:
> 1. If using ixgbe NIC, then the driver will also emit following
>    warning message:
>    ixgbe 0000:01:00.1: partial checksum but l4 proto=32!
> 2. Receiving VM will drop all the corrupted ESP packets, hence UDP iperf test
>    with large packets will fail completely or TCP iperf will get ridiculously
>    low performance because TCP window will never grow above MTU.
> 
> Signed-off-by: Ansis Atteka <aatteka@ovn.org>
> ---
>  net/xfrm/xfrm_output.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
> index 8ba29fe..7ad7e5f 100644
> --- a/net/xfrm/xfrm_output.c
> +++ b/net/xfrm/xfrm_output.c
> @@ -168,7 +168,8 @@ static int xfrm_output2(struct net *net, struct sock *sk, struct sk_buff *skb)
>  
>  static int xfrm_output_gso(struct net *net, struct sock *sk, struct sk_buff *skb)
>  {
> -	struct sk_buff *segs;
> +	struct sk_buff *segs, *nskb;
> +	int err;
>  
>  	BUILD_BUG_ON(sizeof(*IPCB(skb)) > SKB_SGO_CB_OFFSET);
>  	BUILD_BUG_ON(sizeof(*IP6CB(skb)) > SKB_SGO_CB_OFFSET);
> @@ -180,21 +181,27 @@ static int xfrm_output_gso(struct net *net, struct sock *sk, struct sk_buff *skb
>  		return -EINVAL;
>  
>  	do {
> -		struct sk_buff *nskb = segs->next;
> -		int err;
> +		nskb = segs->next;
>  
>  		segs->next = NULL;
> -		err = xfrm_output2(net, sk, segs);
> +		err = skb_checksum_help(segs);

What's wrong with the checksum provided by the GSO layer and
why we have to do this unconditionally here?

We don't announce any checksum capabilities, so the GSO
layer should provide the checksum. If this is not the case,
something along the path is taking wrong assumptions.

Btw. all GSO packets on a standard IPv4 xfrm tunnel are getting
dropped with your patch applied.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net] xfrm: calculate L4 checksums also for GSO case before encrypting packets
       [not found]   ` <CAMQa7Biajree5Kc1fOWQN42R1UDDGp7ZevZZRtUMZOKDWTk-Vw@mail.gmail.com>
@ 2017-04-14  2:54     ` Ansis Atteka
  2017-04-18  9:09     ` Steffen Klassert
  1 sibling, 0 replies; 8+ messages in thread
From: Ansis Atteka @ 2017-04-14  2:54 UTC (permalink / raw)
  To: Steffen Klassert, netdev, Ansis Atteka

On 13 April 2017 at 19:45, Ansis Atteka <ansisatteka@gmail.com> wrote:
>
>
>
> On 11 April 2017 at 00:07, Steffen Klassert <steffen.klassert@secunet.com> wrote:
>>
>> On Mon, Apr 10, 2017 at 11:42:07AM -0700, Ansis Atteka wrote:
>> > Otherwise, if L4 checksum calculation is done after encryption,
>> > then all ESP packets end up being corrupted at the location
>> > where pre-encryption L4 checksum field resides.
>> >
>> > One of the ways to reproduce this bug is to have a VM with virtio_net
>> > driver (UFO set to ON in the guest VM); and then encapsulate all guest's
>> > Ethernet frames in GENEVE; and then further encrypt GENEVE with IPsec.
>> > In this case following symptoms are observed:
>> > 1. If using ixgbe NIC, then the driver will also emit following
>> >    warning message:
>> >    ixgbe 0000:01:00.1: partial checksum but l4 proto=32!
>> > 2. Receiving VM will drop all the corrupted ESP packets, hence UDP iperf test
>> >    with large packets will fail completely or TCP iperf will get ridiculously
>> >    low performance because TCP window will never grow above MTU.
>> >
>> > Signed-off-by: Ansis Atteka <aatteka@ovn.org>
>> > ---
>> >  net/xfrm/xfrm_output.c | 19 +++++++++++++------
>> >  1 file changed, 13 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
>> > index 8ba29fe..7ad7e5f 100644
>> > --- a/net/xfrm/xfrm_output.c
>> > +++ b/net/xfrm/xfrm_output.c
>> > @@ -168,7 +168,8 @@ static int xfrm_output2(struct net *net, struct sock *sk, struct sk_buff *skb)
>> >
>> >  static int xfrm_output_gso(struct net *net, struct sock *sk, struct sk_buff *skb)
>> >  {
>> > -     struct sk_buff *segs;
>> > +     struct sk_buff *segs, *nskb;
>> > +     int err;
>> >
>> >       BUILD_BUG_ON(sizeof(*IPCB(skb)) > SKB_SGO_CB_OFFSET);
>> >       BUILD_BUG_ON(sizeof(*IP6CB(skb)) > SKB_SGO_CB_OFFSET);
>> > @@ -180,21 +181,27 @@ static int xfrm_output_gso(struct net *net, struct sock *sk, struct sk_buff *skb
>> >               return -EINVAL;
>> >
>> >       do {
>> > -             struct sk_buff *nskb = segs->next;
>> > -             int err;
>> > +             nskb = segs->next;
>> >
>> >               segs->next = NULL;
>> > -             err = xfrm_output2(net, sk, segs);
>> > +             err = skb_checksum_help(segs);
>>
>> What's wrong with the checksum provided by the GSO layer and
>> why we have to do this unconditionally here?

I believe with "GSO layer" you meant the skb_gso_segment() function
invocation from xfrm_output_gso()?

If so, then the problem with that is that the list of the skb's
returned by that function could be in CHECKSUM_PARTIAL state, if skbs
came from a UDP tunnel such as Geneve:

   xfrm_output() {
     __skb_gso_segment() {
       skb_mac_gso_segment() {
         skb_network_protocol();
         inet_gso_segment() {
           udp4_ufo_fragment() {
             skb_udp_tunnel_segment() {
               skb_mac_gso_segment() {
                 skb_network_protocol();
                 inet_gso_segment() {
                   udp4_ufo_fragment() {
                     skb_checksum() {
                       __skb_checksum() {
                         csum_partial() {
                           do_csum();
                         }
                         csum_partial() {
                           do_csum();
                         }
                       }


Since those skbs could remain in CHECKSUM_PARTIAL state even after
IPsec encryption, then ixgbe tries to calculate L4 checksums on
already encrypted skb where L4 layer is already protected through
IPsec integrity checks. Hence, ESP packets end up being corrupted and
dropped on receive side by XFRM. I clearly see this ESP packet
corruption happening by observing:
1. in wireshark that the same ESP packet differs at the offset where
UDP checksum field should reside; AND
2. in dmesg that ixgbe driver complains on send side with "partial
checksum but L4 proto is 0x32 (ESP)". AND
3. in /proc/net/xfrm_stat where XfrmInStateProtoErrorcounter is
incremented on receive side each time it receives corrupted packet.


>>
>>
>> We don't announce any checksum capabilities, so the GSO
>> layer should provide the checksum. If this is not the case,
>> something along the path is taking wrong assumptions.
>
>
The same explicit checksum calculation is done from xfrm_output() for
non-GSO case, so it was tempting for me to simply put a similar
skb_checksum_help() for GSO case as well.
>
>> Btw. all GSO packets on a standard IPv4 xfrm tunnel are getting
>> dropped with your patch applied.
>>
I think I just noticed possible issue with my patch that I sent out.
In your setup were packets getting dropped on receive side due to UDP
checksum failure (and not IPsec integrity check failure)? If so then I
wonder if after my patch applied skb_checksum_help() was called twice
under conditions that you tested for. Hence the skbs ended up with
wrong checksums.

So, would you mind to give another spin for my patch after applying
this small amendment that calls skb_checkum_help() only in
CHECKSUM_PARTIAL case:

diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
index 7ad7e5f..a870164 100644
--- a/net/xfrm/xfrm_output.c
+++ b/net/xfrm/xfrm_output.c
@@ -184,10 +184,12 @@ static int xfrm_output_gso(struct net *net,
struct sock *sk, struct sk_buff *skb
                nskb = segs->next;

                segs->next = NULL;
-               err = skb_checksum_help(segs);
-               if (unlikely(err)) {
-                       XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
-                       goto error;
+               if (skb->ip_summed == CHECKSUM_PARTIAL) {
+                       err = skb_checksum_help(segs);
+                       if (unlikely(err)) {
+                               XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
+                               goto error;
+                       }
                }

                err = xfrm_output2(net, sk, segs);

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH net] xfrm: calculate L4 checksums also for GSO case before encrypting packets
       [not found]   ` <CAMQa7Biajree5Kc1fOWQN42R1UDDGp7ZevZZRtUMZOKDWTk-Vw@mail.gmail.com>
  2017-04-14  2:54     ` Ansis Atteka
@ 2017-04-18  9:09     ` Steffen Klassert
  2017-04-19  2:10       ` Ansis Atteka
  1 sibling, 1 reply; 8+ messages in thread
From: Steffen Klassert @ 2017-04-18  9:09 UTC (permalink / raw)
  To: Ansis Atteka; +Cc: Ansis Atteka, netdev

On Thu, Apr 13, 2017 at 07:45:08PM -0700, Ansis Atteka wrote:
> On 11 April 2017 at 00:07, Steffen Klassert <steffen.klassert@secunet.com>
> wrote:
> >
> > What's wrong with the checksum provided by the GSO layer and
> > why we have to do this unconditionally here?
> >
> 
> I believe with "GSO layer" you meant the skb_gso_segment() function
> invocation from xfrm_output_gso()?
> 
> If so, then the problem with that is that the list of the skb's returned by
> that function could be in CHECKSUM_PARTIAL state if skb came from a UDP
> tunnel such as Geneve:

This should not happen. We don't announce checksum capabilities,
so the GSO layer should generate the full checksum.

__skb_udp_tunnel_segment() and udp4_ufo_fragment() add the
NETIF_F_HW_CSUM flag to features. This is certainly wrong
if the packet undergoes an IPsec transformation.

I don't have a testcase for this, so not sure if this is
your problem. Could you try the (untested) patch below?

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index b2be1d9..cc0c89c 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -74,7 +74,7 @@ static struct sk_buff *__skb_udp_tunnel_segment(struct sk_buff *skb,
 	 * outer one so strip the existing checksum feature flags and
 	 * instead set the flag based on our outer checksum offload value.
 	 */
-	if (remcsum || ufo) {
+	if ((remcsum || ufo) && !(skb_dst(skb) && dst_xfrm(skb_dst(skb)))) {
 		features &= ~NETIF_F_CSUM_MASK;
 		if (!need_csum || offload_csum)
 			features |= NETIF_F_HW_CSUM;
@@ -238,7 +238,7 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb,
 	 * due to the fact that we have already done the checksum in
 	 * software prior to segmenting the frame.
 	 */
-	if (!skb->encap_hdr_csum)
+	if (!skb->encap_hdr_csum && !(skb_dst(skb) && dst_xfrm(skb_dst(skb))))
 		features |= NETIF_F_HW_CSUM;
 
 	/* Fragment the skb. IP headers of the fragments are updated in

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH net] xfrm: calculate L4 checksums also for GSO case before encrypting packets
  2017-04-18  9:09     ` Steffen Klassert
@ 2017-04-19  2:10       ` Ansis Atteka
  2017-04-20  9:47         ` Steffen Klassert
  0 siblings, 1 reply; 8+ messages in thread
From: Ansis Atteka @ 2017-04-19  2:10 UTC (permalink / raw)
  To: Steffen Klassert; +Cc: Ansis Atteka, netdev

On 18 April 2017 at 02:09, Steffen Klassert
<steffen.klassert@secunet.com> wrote:
> On Thu, Apr 13, 2017 at 07:45:08PM -0700, Ansis Atteka wrote:
>> On 11 April 2017 at 00:07, Steffen Klassert <steffen.klassert@secunet.com>
>> wrote:
>> >
>> > What's wrong with the checksum provided by the GSO layer and
>> > why we have to do this unconditionally here?
>> >
>>
>> I believe with "GSO layer" you meant the skb_gso_segment() function
>> invocation from xfrm_output_gso()?
>>
>> If so, then the problem with that is that the list of the skb's returned by
>> that function could be in CHECKSUM_PARTIAL state if skb came from a UDP
>> tunnel such as Geneve:
>
> This should not happen. We don't announce checksum capabilities,
> so the GSO layer should generate the full checksum.
>
> __skb_udp_tunnel_segment() and udp4_ufo_fragment() add the
> NETIF_F_HW_CSUM flag to features. This is certainly wrong
> if the packet undergoes an IPsec transformation.
>
> I don't have a testcase for this, so not sure if this is
> your problem. Could you try the (untested) patch below?

I am still seeing the same ESP packet corruption issue with the patch
you attached.

However, after taking pointers from your patch I came up with this one
that may solve this problem once and for all (note, that I was seeing
this bug only with ixgbe NIC that supports tx csum offloads). I hope
it does not break any other IPsec tests that you have.

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index b2be1d9..7812501 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -29,6 +29,7 @@ static struct sk_buff
*__skb_udp_tunnel_segment(struct sk_buff *skb,
        u16 mac_len = skb->mac_len;
        int udp_offset, outer_hlen;
        __wsum partial;
+       bool need_ipsec;

        if (unlikely(!pskb_may_pull(skb, tnl_hlen)))
                goto out;
@@ -62,8 +63,10 @@ static struct sk_buff
*__skb_udp_tunnel_segment(struct sk_buff *skb,

        ufo = !!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP);

+       need_ipsec = skb_dst(skb) && dst_xfrm(skb_dst(skb));
        /* Try to offload checksum if possible */
        offload_csum = !!(need_csum &&
+                         !need_ipsec &&
                          (skb->dev->features &
                           (is_ipv6 ? (NETIF_F_HW_CSUM | NETIF_F_IPV6_CSUM) :
                                      (NETIF_F_HW_CSUM | NETIF_F_IP_CSUM))));

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH net] xfrm: calculate L4 checksums also for GSO case before encrypting packets
  2017-04-19  2:10       ` Ansis Atteka
@ 2017-04-20  9:47         ` Steffen Klassert
  2017-04-21 21:45           ` Ansis Atteka
  0 siblings, 1 reply; 8+ messages in thread
From: Steffen Klassert @ 2017-04-20  9:47 UTC (permalink / raw)
  To: Ansis Atteka; +Cc: Ansis Atteka, netdev

On Tue, Apr 18, 2017 at 07:10:03PM -0700, Ansis Atteka wrote:
> 
> However, after taking pointers from your patch I came up with this one
> that may solve this problem once and for all (note, that I was seeing
> this bug only with ixgbe NIC that supports tx csum offloads). I hope
> it does not break any other IPsec tests that you have.
> 
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index b2be1d9..7812501 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -29,6 +29,7 @@ static struct sk_buff
> *__skb_udp_tunnel_segment(struct sk_buff *skb,
>         u16 mac_len = skb->mac_len;
>         int udp_offset, outer_hlen;
>         __wsum partial;
> +       bool need_ipsec;
> 
>         if (unlikely(!pskb_may_pull(skb, tnl_hlen)))
>                 goto out;
> @@ -62,8 +63,10 @@ static struct sk_buff
> *__skb_udp_tunnel_segment(struct sk_buff *skb,
> 
>         ufo = !!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP);
> 
> +       need_ipsec = skb_dst(skb) && dst_xfrm(skb_dst(skb));
>         /* Try to offload checksum if possible */
>         offload_csum = !!(need_csum &&
> +                         !need_ipsec &&
>                           (skb->dev->features &
>                            (is_ipv6 ? (NETIF_F_HW_CSUM | NETIF_F_IPV6_CSUM) :
>                                       (NETIF_F_HW_CSUM | NETIF_F_IP_CSUM))));

This looks good, but we should fix udp4_ufo_fragment() too.

Thanks!

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net] xfrm: calculate L4 checksums also for GSO case before encrypting packets
  2017-04-20  9:47         ` Steffen Klassert
@ 2017-04-21 21:45           ` Ansis Atteka
  2017-04-27  9:04             ` Steffen Klassert
  0 siblings, 1 reply; 8+ messages in thread
From: Ansis Atteka @ 2017-04-21 21:45 UTC (permalink / raw)
  To: Steffen Klassert; +Cc: Ansis Atteka, netdev

On 20 April 2017 at 02:47, Steffen Klassert
<steffen.klassert@secunet.com> wrote:
> On Tue, Apr 18, 2017 at 07:10:03PM -0700, Ansis Atteka wrote:
>>
>> However, after taking pointers from your patch I came up with this one
>> that may solve this problem once and for all (note, that I was seeing
>> this bug only with ixgbe NIC that supports tx csum offloads). I hope
>> it does not break any other IPsec tests that you have.
>>
>> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>> index b2be1d9..7812501 100644
>> --- a/net/ipv4/udp_offload.c
>> +++ b/net/ipv4/udp_offload.c
>> @@ -29,6 +29,7 @@ static struct sk_buff
>> *__skb_udp_tunnel_segment(struct sk_buff *skb,
>>         u16 mac_len = skb->mac_len;
>>         int udp_offset, outer_hlen;
>>         __wsum partial;
>> +       bool need_ipsec;
>>
>>         if (unlikely(!pskb_may_pull(skb, tnl_hlen)))
>>                 goto out;
>> @@ -62,8 +63,10 @@ static struct sk_buff
>> *__skb_udp_tunnel_segment(struct sk_buff *skb,
>>
>>         ufo = !!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP);
>>
>> +       need_ipsec = skb_dst(skb) && dst_xfrm(skb_dst(skb));
>>         /* Try to offload checksum if possible */
>>         offload_csum = !!(need_csum &&
>> +                         !need_ipsec &&
>>                           (skb->dev->features &
>>                            (is_ipv6 ? (NETIF_F_HW_CSUM | NETIF_F_IPV6_CSUM) :
>>                                       (NETIF_F_HW_CSUM | NETIF_F_IP_CSUM))));
>
> This looks good, but we should fix udp4_ufo_fragment() too.
>
> Thanks!

I removed Geneve tunneling from equation and tried to run a simple
iperf underlay UDP test while IPsec was still enabled to observe
issues with the udp4_ufo_fragment() case.

Unfortunately, as can be seen from kernel tracer output below, I was
unable to come up with a test case where udp4_ufo_fragment function
would ever be invoked while IPsec is enabled:

admin1@ubuntu1:~/xfrm_test/net$ ifconfig em2.4001 | grep "inet addr"
          inet addr:192.168.1.1  Bcast:192.168.1.255  Mask:255.255.255.0
admin1@ubuntu1:~/xfrm_test/net$ ethtool -k em2.4001 | grep
udp-fragmentation-offload
udp-fragmentation-offload: on
admin1@ubuntu1:~/xfrm_test/net$ sudo trace-cmd record -p
function_graph -c -F iperf -c 192.168.1.2 -u -l20000
admin1@ubuntu1:~/xfrm_test/net$ trace-cmd report | grep udp4
admin1@ubuntu1:~/xfrm_test/net$


Nevertheless, after disabling IPsec and leaving everything else the
same, I start to see that udp4_ufo_fragment() gets invoked:

admin1@ubuntu1:~/xfrm_test/net$ trace-cmd report | grep udp4
           iperf-25466 [004] 242431.203307: funcgraph_entry:
0.113 us   |                  udp4_hwcsum();
           iperf-25466 [004] 242431.203360: funcgraph_entry:
        |
udp4_ufo_fragment() {
           iperf-25466 [004] 242431.508436: funcgraph_entry:
0.080 us   |                  udp4_hwcsum();
           iperf-25466 [004] 242431.508542: funcgraph_entry:
        |
udp4_ufo_fragment() {


However, non-IPsec case really does not have this ESP packet
corruption problem, because then the packets are in plain and can
utilize checksum offloads. Do we really have a problem there for
IPsec? I did not have time yet to look into the code to understand why
exactly udp4_ufo_fragment() is not called for IPsec case, but since I
can't come up with a real life test case, then for now I will simply
resubmit the previous patch as-is to netdev mailinglist to solve the
problem I encountered previously for Geneve tunneling case.

If you could drop more hints on how to come up with an IPsec test case
where udp4_ufo_fragment() is still invoked and packets get corrupted,
then I can later send another patch for that.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net] xfrm: calculate L4 checksums also for GSO case before encrypting packets
  2017-04-21 21:45           ` Ansis Atteka
@ 2017-04-27  9:04             ` Steffen Klassert
  0 siblings, 0 replies; 8+ messages in thread
From: Steffen Klassert @ 2017-04-27  9:04 UTC (permalink / raw)
  To: Ansis Atteka; +Cc: Ansis Atteka, netdev

On Fri, Apr 21, 2017 at 02:45:17PM -0700, Ansis Atteka wrote:
> 
> I removed Geneve tunneling from equation and tried to run a simple
> iperf underlay UDP test while IPsec was still enabled to observe
> issues with the udp4_ufo_fragment() case.
> 
> Unfortunately, as can be seen from kernel tracer output below, I was
> unable to come up with a test case where udp4_ufo_fragment function
> would ever be invoked while IPsec is enabled:
> 
> admin1@ubuntu1:~/xfrm_test/net$ ifconfig em2.4001 | grep "inet addr"
>           inet addr:192.168.1.1  Bcast:192.168.1.255  Mask:255.255.255.0
> admin1@ubuntu1:~/xfrm_test/net$ ethtool -k em2.4001 | grep
> udp-fragmentation-offload
> udp-fragmentation-offload: on
> admin1@ubuntu1:~/xfrm_test/net$ sudo trace-cmd record -p
> function_graph -c -F iperf -c 192.168.1.2 -u -l20000
> admin1@ubuntu1:~/xfrm_test/net$ trace-cmd report | grep udp4
> admin1@ubuntu1:~/xfrm_test/net$
> 
> 
> Nevertheless, after disabling IPsec and leaving everything else the
> same, I start to see that udp4_ufo_fragment() gets invoked:
> 
> admin1@ubuntu1:~/xfrm_test/net$ trace-cmd report | grep udp4
>            iperf-25466 [004] 242431.203307: funcgraph_entry:
> 0.113 us   |                  udp4_hwcsum();
>            iperf-25466 [004] 242431.203360: funcgraph_entry:
>         |
> udp4_ufo_fragment() {
>            iperf-25466 [004] 242431.508436: funcgraph_entry:
> 0.080 us   |                  udp4_hwcsum();
>            iperf-25466 [004] 242431.508542: funcgraph_entry:
>         |
> udp4_ufo_fragment() {
> 
> 
> However, non-IPsec case really does not have this ESP packet
> corruption problem, because then the packets are in plain and can
> utilize checksum offloads. Do we really have a problem there for
> IPsec?

Probably not, at least locally generated packets don't do
ufo if they have an IPsec route. So it seems to be ok to
leave udp4_ufo_fragment as it is.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2017-04-27  9:23 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-10 18:42 [PATCH net] xfrm: calculate L4 checksums also for GSO case before encrypting packets Ansis Atteka
2017-04-11  7:07 ` Steffen Klassert
     [not found]   ` <CAMQa7Biajree5Kc1fOWQN42R1UDDGp7ZevZZRtUMZOKDWTk-Vw@mail.gmail.com>
2017-04-14  2:54     ` Ansis Atteka
2017-04-18  9:09     ` Steffen Klassert
2017-04-19  2:10       ` Ansis Atteka
2017-04-20  9:47         ` Steffen Klassert
2017-04-21 21:45           ` Ansis Atteka
2017-04-27  9:04             ` Steffen Klassert

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.