netdev - Re: [PATCH net-next] net: pskb_trim_rcsum() and CHECKSUM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAKgT0UcqTouO9Xj+KD5dEWQVMAfzQ4ye2hUXKVxycpiv5M3-eg@mail.gmail.com>
Date:   Thu, 19 Apr 2018 11:18:35 -0700
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Eric Dumazet <eric.dumazet@...il.com>
Subject: Re: [PATCH net-next] net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends

On Wed, Apr 18, 2018 at 11:43 AM, Eric Dumazet <edumazet@...gle.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@...gle.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