netdev - Re: Setting skb->csum in shared skb with csum-complete

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 for Android: free password hash cracker in your pocket

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

Date:   Thu, 19 Jan 2017 18:00:34 -0800
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Tom Herbert <tom@...bertland.com>
Cc:     Herbert Xu <herbert@...dor.apana.org.au>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: Setting skb->csum in shared skb with csum-complete

On Thu, 2017-01-19 at 17:29 -0800, Tom Herbert wrote:
> Hi Herbert,
> 
> I am looking at "net: Fix skb csum races when peeking". The context is
> that I'm seeing checksum faults after backporting mlx5 from driver.
> The handling of the checksum faults in interesting in itself. One side
> effect is that we end up recalculating and doing the dump twice when
> GRO is enabled. Another is that we validate checksum complete in a
> shared skbuf with a knowingly bad skb->csum value.
> 
> In the checksum fault case we recalculate the checksum and find it was
> actually correct in the packet. If the skb is not shared then we set
> the skb->csum to the correct value and also set skb->csum_complete_sw
> and skb->csum_valid. But if skb is shared we don't do any of that.
> This is kind of odd since __skb_checksum_complete_head then returns
> zero for an skb with checksum_complete where the checksum complete
> value is not correct. I'm not sure what the consequences of doing that
> are (I don't think it's related to issues I'm seeing) but this doesn't
> seem like a good thing. Maybe we should set skb->csum in this case
> since we know it's already wrong and the fact that skbuf is shared
> can't make it more wrong...

Why GRO can not change skb fields ?

skb can not be shared at GRO layer, so really this should be done there
if you want to avoid a double checksumming later.

You added all these skb fields, you know better than us ;)