[PATCH net-next] net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends

[PATCH net-next] net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends

[Eric Dumazet]

After working on IP defragmentation lately, I found that some large
packets defeat CHECKSUM_COMPLETE optimization because of NIC adding
zero paddings on the last (small) fragment.

While removing the padding with pskb_trim_rcsum(), we set skb->ip_summed
to CHECKSUM_NONE, forcing a full csum validation, even if all prior
fragments had CHECKSUM_COMPLETE set.

We can instead compute the checksum of the part we are trimming,
usually smaller than the part we keep.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/linux/skbuff.h |  5 ++---
 net/core/skbuff.c      | 14 ++++++++++++++
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 9065477ed255a48f7e01b8a28ea6321cce9127f5..d274059529eb5216d041dfdcad4a564a623c8ea0 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3131,6 +3131,7 @@ static inline void *skb_push_rcsum(struct sk_buff *skb, unsigned int len)
 	return skb->data;
 }
 
+int pskb_trim_rcsum_slow(struct sk_buff *skb, unsigned int len);
 /**
  *	pskb_trim_rcsum - trim received skb and update checksum
  *	@skb: buffer to trim
@@ -3144,9 +3145,7 @@ static inline int pskb_trim_rcsum(struct sk_buff *skb, unsigned int len)
 {
 	if (likely(len >= skb->len))
 		return 0;
-	if (skb->ip_summed == CHECKSUM_COMPLETE)
-		skb->ip_summed = CHECKSUM_NONE;
-	return __pskb_trim(skb, len);
+	return pskb_trim_rcsum_slow(skb, len);
 }
 
 static inline int __skb_trim_rcsum(struct sk_buff *skb, unsigned int len)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 345b51837ca80bb709bfffe04d58eedbba0b9907..ff49e352deea1d07049f5c5ac425fbcdb4fd96a8 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1839,6 +1839,20 @@ int ___pskb_trim(struct sk_buff *skb, unsigned int len)
 }
 EXPORT_SYMBOL(___pskb_trim);
 
+/* Note : use pskb_trim_rcsum() instead of calling this directly
+ */
+int pskb_trim_rcsum_slow(struct sk_buff *skb, unsigned int len)
+{
+	if (skb->ip_summed == CHECKSUM_COMPLETE) {
+		int delta = skb->len - len;
+
+		skb->csum = csum_sub(skb->csum,
+				     skb_checksum(skb, len, delta, 0));
+	}
+	return __pskb_trim(skb, len);
+}
+EXPORT_SYMBOL(pskb_trim_rcsum_slow);
+
 /**
  *	__pskb_pull_tail - advance tail of skb header
  *	@skb: buffer to reallocate
-- 
2.17.0.484.g0c8726318c-goog

Re: [PATCH net-next] net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends

[David Miller]

From: Eric Dumazet <edumazet@google.com>
Date: Wed, 18 Apr 2018 11:43:15 -0700

> After working on IP defragmentation lately, I found that some large
> packets defeat CHECKSUM_COMPLETE optimization because of NIC adding
> zero paddings on the last (small) fragment.
> 
> While removing the padding with pskb_trim_rcsum(), we set skb->ip_summed
> to CHECKSUM_NONE, forcing a full csum validation, even if all prior
> fragments had CHECKSUM_COMPLETE set.

Oops.

> We can instead compute the checksum of the part we are trimming,
> usually smaller than the part we keep.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Looks good, applied, thanks Eric.

Re: [PATCH net-next] net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends

[Alexander Duyck]

On Wed, Apr 18, 2018 at 11:43 AM, Eric Dumazet <edumazet@google.com> wrote:
> After working on IP defragmentation lately, I found that some large
> packets defeat CHECKSUM_COMPLETE optimization because of NIC adding
> zero paddings on the last (small) fragment.
>
> While removing the padding with pskb_trim_rcsum(), we set skb->ip_summed
> to CHECKSUM_NONE, forcing a full csum validation, even if all prior
> fragments had CHECKSUM_COMPLETE set.
>
> We can instead compute the checksum of the part we are trimming,
> usually smaller than the part we keep.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  include/linux/skbuff.h |  5 ++---
>  net/core/skbuff.c      | 14 ++++++++++++++
>  2 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 9065477ed255a48f7e01b8a28ea6321cce9127f5..d274059529eb5216d041dfdcad4a564a623c8ea0 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -3131,6 +3131,7 @@ static inline void *skb_push_rcsum(struct sk_buff *skb, unsigned int len)
>         return skb->data;
>  }
>
> +int pskb_trim_rcsum_slow(struct sk_buff *skb, unsigned int len);
>  /**
>   *     pskb_trim_rcsum - trim received skb and update checksum
>   *     @skb: buffer to trim
> @@ -3144,9 +3145,7 @@ static inline int pskb_trim_rcsum(struct sk_buff *skb, unsigned int len)
>  {
>         if (likely(len >= skb->len))
>                 return 0;
> -       if (skb->ip_summed == CHECKSUM_COMPLETE)
> -               skb->ip_summed = CHECKSUM_NONE;
> -       return __pskb_trim(skb, len);
> +       return pskb_trim_rcsum_slow(skb, len);
>  }
>

I'm wondering if in the past padding was somehow screwing up the
CHECKSUM_COMPLETE value being provided. I wonder if it wouldn't be in
our interest to just consider manually computing the checksum of the
fragment after stripping the padding instead of just subtracting the
offset of the padding.

I guess if we start seeing checksum errors popping up on some devices
we can reevaluate this if necessary.

Thanks.

- Alex