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

Date:   Wed, 9 Aug 2017 20:06:53 +0800
From:   Tonghao Zhang <xiangxia.m.yue@...il.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...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.

Thanks, I send a patch, which will revert commit 6e7bc478c9a0 in net-next.

On Wed, Aug 9, 2017 at 1:46 AM, Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>>> @@ -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.
>
> On transmit, packets entering skb_gso_segment are expected to always
> have ip_summed CHECKSUM_PARTIAL. This check was added to track down
> unexpected exceptions in commit 67fd1a731ff1 ("net: Add debug info to
> track down GSO checksum bug").
>
> Only when called for the second time, after skb_mac_gso_segment, do we
> have to possibly handle the case where the GSO layer computes the
> checksum and changes ip_summed.
>
> Since this only goes into 4.11 to 4.13, making two separate
> skb_needs_check variants for these two call sites seems overkill. I
> will send the simple fix to convert CHECKSUM_NONE to
> CHECKSUM_UNNECESSARY.
>
> As a side effect of removing UFO in 4.14-rc1, we can also revert
> commit 6e7bc478c9a0 ("net: skb_needs_check() accepts CHECKSUM_NONE for
> tx") in net-next.