netdev - Re: [PATCH] net: Reduce skb_warn_bad

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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 7 Aug 2017 23:29:40 -0700
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Tonghao Zhang <xiangxia.m.yue@...il.com>
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Willem de Bruijn <willemb@...gle.com>,
        Pravin B Shelar <pshelar@....org>
Subject: Re: [PATCH] net: Reduce skb_warn_bad_offload() noise.

> In a case, there is also warn info. The test topo is shown as below.

Thanks for testing the patch, Tonghao.

> The warn info is shown as below [1]. If we change the CHECKSUM_NONE to
> CHECKSUM_UNNECESSARY in the udp4_ufo_fragment().
> and we should add a check in skb_needs_check() when outputting a packet.
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 416137c..8fe12a7 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2670,6 +2670,7 @@ static inline bool skb_needs_check(struct
> sk_buff *skb, bool tx_path)
>  {
>         if (tx_path)
>                 return skb->ip_summed != CHECKSUM_PARTIAL &&
> +                      skb->ip_summed != CHECKSUM_UNNECESSARY &&
>                        skb->ip_summed != CHECKSUM_NONE;

Good catch. Only, the CHECKSUM_NONE case was added specifically to
work around this UFO issue on the tx path in commit 6e7bc478c9a0
("net: skb_needs_check() accepts CHECKSUM_NONE for tx"). If we change
the value generated by UFO, we can remove that statement, so

+                      skb->ip_summed != CHECKSUM_UNNECESSARY;
-                        skb->ip_summed != CHECKSUM_NONE;

Else the entire check becomes a NOOP. These are the only three valid
states on tx. With very few codepaths generating CHECKSUM_UNNECESSARY
to begin with, it arguably already is practically a NOOP. I need to
look more closely what the statement is intended to protect against,
before we relax it even further.

The patch will need the same UFO change in ipv6_ufo_fragment.

I also had to verify that it is indeed correct to use
CHECKSUM_UNNECESSARY for this case. It is not commonly used as alias
for CHECKSUM_NONE on the tx path. And indeed does not carry the exact
same meaning. It was defined as "no need to checksum" on tx for
protocols that skb_checksum_help does not support:

  http://patchwork.ozlabs.org/patch/146567/

Given that, it should be fine to use in this case.