netdev - Re: [PATCH net v9] skb_expand_head() adjust skb->truesize incorrectly

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:   Wed, 20 Oct 2021 09:18:13 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Vasily Averin <vvs@...tuozzo.com>,
        Eric Dumazet <eric.dumazet@...il.com>,
        Jakub Kicinski <kuba@...nel.org>
Cc:     netdev <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        David Ahern <dsahern@...nel.org>,
        Julian Wiedmann <jwi@...ux.ibm.com>,
        Christoph Paasch <christoph.paasch@...il.com>,
        linux-kernel@...r.kernel.org, kernel@...nvz.org
Subject: Re: [PATCH net v9] skb_expand_head() adjust skb->truesize incorrectly



On 10/4/21 10:57 PM, Vasily Averin wrote:
> On 10/4/21 10:26 PM, Eric Dumazet wrote:

>>
>>     Why not re-using is_skb_wmem() here ?
>>     Testing != sock_edemux looks strange.
> 
> All non-wmem skbs was cloned and then was freed already.
> After pskb_expand_head() call we can have:
> (1) either original wmem skbs 
> (2) or cloned skbs: 
>  (2a) either without sk at all,
>  (2b) or with sock_edemux destructor (that was set inside skb_set_owner_w() for !sk_fullsock(sk))
>  (2c) or with sock_wfree destructor (that was set inside skb_set_owner_w() for sk_fullsock(sk))
> 
> (2a) and (2b) do not require truesize/sk_wmem_alloc update, it was handled inside pskb_expand_head()
> (1) and (2c) cases are processed here.
> 
> If required I can add this explanation either into patch description or as comment.
> 

sock_edemux is one of the current destructors.

New ones will be added later. We can not expect that in two or three years,
at least one reviewer will remember this special case.

I would prefer you list the known destructors (allow-list, instead of disallow-list)



> Btw I just noticed that we can avoid cloning for original skbs without sk.
> How do you think should I do it?

Lets wait a bit before new features...