netdev - Re: [RFC net v7] net: 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]

Message-ID: <20210920111259.18f9cc01@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 20 Sep 2021 11:12:59 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Vasily Averin <vvs@...tuozzo.com>
Cc:     eric.dumazet@...il.com, netdev@...r.kernel.org,
        Christoph Paasch <christoph.paasch@...il.com>,
        Hao Sun <sunhao.th@...il.com>
Subject: Re: [RFC net v7] net: skb_expand_head() adjust skb->truesize
 incorrectly

On Sat, 18 Sep 2021 13:05:28 +0300 Vasily Averin wrote:
> On 9/17/21 7:24 PM, Jakub Kicinski wrote:
> > From: Vasily Averin <vvs@...tuozzo.com>
> > 
> > Christoph Paasch reports [1] about incorrect skb->truesize
> > after skb_expand_head() call in ip6_xmit.
> > This may happen because of two reasons:
> >  - skb_set_owner_w() for newly cloned skb is called too early,
> >    before pskb_expand_head() where truesize is adjusted for (!skb-sk) case.
> >  - pskb_expand_head() does not adjust truesize in (skb->sk) case.
> >    In this case sk->sk_wmem_alloc should be adjusted too.
> > 
> > Eric cautions us against increasing sk_wmem_alloc if the old
> > skb did not hold any wmem references.

> > @@ -1810,21 +1829,28 @@ struct sk_buff *skb_expand_head(struct sk_buff *skb, unsigned int headroom)
> >  	if (skb_shared(skb)) {
> >  		struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
> >  
> > -		if (likely(nskb)) {
> > -			if (skb->sk)
> > -				skb_set_owner_w(nskb, skb->sk);
> > -			consume_skb(skb);
> > -		} else {
> > -			kfree_skb(skb);
> > -		}
> > +		if (unlikely(!nskb))
> > +			goto err_free;
> > +
> > +		skb_owner_inherit(nskb, skb);
> > +		consume_skb(skb);
> >  		skb = nskb;
> >  	}
> > -	if (skb &&
> > -	    pskb_expand_head(skb, SKB_DATA_ALIGN(delta), 0, GFP_ATOMIC)) {
> > -		kfree_skb(skb);
> > -		skb = NULL;
> > -	}
> > +
> > +	if (pskb_expand_head(skb, SKB_DATA_ALIGN(delta), 0, GFP_ATOMIC))
> > +		goto err_free;
> > +	delta = skb_end_offset(skb) - osize;
> > +
> > +	/* pskb_expand_head() will adjust truesize itself for non-sk cases
> > +	 * todo: move the adjustment there at some point?
> > +	 */
> > +	if (skb->sk && skb->destructor != sock_edemux)
> > +		skb_increase_truesize(skb, delta);  
> 
> I think it is wrong.
> 1) there are a few skb destructors called sock_wfree inside. I've found: 
>    tpacket_destruct_skb, sctp_wfree, unix_destruct_scm and xsk_destruct_skb.
>    If any such skb can be use here it will not adjust sk_wmem_alloc.   I afraid there might be other similar destructors, out of tree,
>    so we cannot have full white list for wfree-compatible destructors.
> 
> 2) in fact you increase truesize here for all skb types.
>    If it is acceptable it could be done directly inside pskb_expand_head().
>    However it isn't.  As you pointed sock_rfree case is handled incorrectly. 
>    I've found other similar destructors: sock_rmem_free, netlink_skb_destructor,
>    kcm_rfree, sock_ofree. They will be handled incorrectly too, but even without WARN_ON.
>    Few other descriptors seems should not fail but do not require truesize update.
> 
> From my POV v6 patch version works correctly in any cases. If necessary it calls
> original destructor, correctly set up new one and correctly adjust truesize
> and sk_wmem_alloc.
> If you still have doubts, we can just go back and clone non-wmem skb, 
> like we did before.

Thanks for taking a look. I would prefer not to bake any ideas about
the skb's function into generic functions. Enumerating every destructor
callback in generic code is impossible (technically so, since the code
may reside in modules).

Let me think about it. Perhaps we can extend sock callbacks with
skb_sock_inherit, and skb_adjust_trusize? That'd transfer the onus of
handling the adjustments done on splitting to the protocols. I'll see
if that's feasible unless someone can immediately call this path
ghastly.