Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Vasily Averin <vvs@virtuozzo.com>
To: Eric Dumazet <eric.dumazet@gmail.com>,
	Christoph Paasch <christoph.paasch@gmail.com>,
	"David S. Miller" <davem@davemloft.net>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
	David Ahern <dsahern@kernel.org>,
	Jakub Kicinski <kuba@kernel.org>, netdev <netdev@vger.kernel.org>,
	linux-kernel@vger.kernel.org, kernel@openvz.org,
	Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>,
	Julian Wiedmann <jwi@linux.ibm.com>
Subject: Re: [PATCH net-next v4] skb_expand_head() adjust skb->truesize incorrectly
Date: Thu, 2 Sep 2021 06:59:28 +0300	[thread overview]
Message-ID: <b7c2cb05-7307-f04e-530e-89fc466aa83f@virtuozzo.com> (raw)
In-Reply-To: <ce783b33-c81f-4760-1f9e-90b7d8c51fd7@gmail.com>

On 9/1/21 10:17 PM, Eric Dumazet wrote:
> 
> 
> On 9/1/21 1:11 AM, Vasily Averin wrote:
>> 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.
>>
>> [1] https://lkml.org/lkml/2021/8/20/1082
>>
>> Fixes: f1260ff15a71 ("skbuff: introduce skb_expand_head()")
>> Reported-by: Christoph Paasch <christoph.paasch@gmail.com>
>> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
>> ---
>> v4: decided to use is_skb_wmem() after pskb_expand_head() call
>>     fixed 'return (EXPRESSION);' in os_skb_wmem according to Eric Dumazet
>> v3: removed __pskb_expand_head(),
>>     added is_skb_wmem() helper for skb with wmem-compatible destructors
>>     there are 2 ways to use it:
>>      - before pskb_expand_head(), to create skb clones
>>      - after successfull pskb_expand_head() to change owner on extended skb.
>> v2: based on patch version from Eric Dumazet,
>>     added __pskb_expand_head() function, which can be forced
>>     to adjust skb->truesize and sk->sk_wmem_alloc.
>> ---
>>  include/net/sock.h |  1 +
>>  net/core/skbuff.c  | 35 ++++++++++++++++++++++++++---------
>>  net/core/sock.c    |  8 ++++++++
>>  3 files changed, 35 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/net/sock.h b/include/net/sock.h
>> index 95b2577..173d58c 100644
>> --- a/include/net/sock.h
>> +++ b/include/net/sock.h
>> @@ -1695,6 +1695,7 @@ struct sk_buff *sock_wmalloc(struct sock *sk, unsigned long size, int force,
>>  			     gfp_t priority);
>>  void __sock_wfree(struct sk_buff *skb);
>>  void sock_wfree(struct sk_buff *skb);
>> +bool is_skb_wmem(const struct sk_buff *skb);
>>  struct sk_buff *sock_omalloc(struct sock *sk, unsigned long size,
>>  			     gfp_t priority);
>>  void skb_orphan_partial(struct sk_buff *skb);
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index f931176..09991cb 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -1804,28 +1804,45 @@ struct sk_buff *skb_realloc_headroom(struct sk_buff *skb, unsigned int headroom)
>>  struct sk_buff *skb_expand_head(struct sk_buff *skb, unsigned int headroom)
>>  {
>>  	int delta = headroom - skb_headroom(skb);
>> +	int osize = skb_end_offset(skb);
>> +	struct sk_buff *oskb = NULL;
>> +	struct sock *sk = skb->sk;
>>  
>>  	if (WARN_ONCE(delta <= 0,
>>  		      "%s is expecting an increase in the headroom", __func__))
>>  		return skb;
>>  
>> -	/* pskb_expand_head() might crash, if skb is shared */
>> +	delta = SKB_DATA_ALIGN(delta);
>> +	/* pskb_expand_head() might crash, if skb is shared.
>> +	 * Also we should clone skb if its destructor does
>> +	 * not adjust skb->truesize and sk->sk_wmem_alloc
>> + 	 */
>>  	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 {
>> +		if (unlikely(!nskb)) {
>>  			kfree_skb(skb);
>> +			return NULL;
>>  		}
>> +		oskb = skb;
>>  		skb = nskb;
>>  	}
>> -	if (skb &&
>> -	    pskb_expand_head(skb, SKB_DATA_ALIGN(delta), 0, GFP_ATOMIC)) {
>> +	if (pskb_expand_head(skb, delta, 0, GFP_ATOMIC)) {
>>  		kfree_skb(skb);
>> -		skb = NULL;
>> +		kfree_skb(oskb);
>> +		return NULL;
>> +	}
>> +	if (oskb) {
>> +		if (sk)
> 
> if (is_skb_wmem(oskb))
> Again, it is not valid to call skb_set_owner_w(skb, sk) on all kind of sockets.

I'm disagree.

In this particular case we have new skb with skb->sk = NULL,
In this case skb_orphan() called inside skb_set_owner_w(() will do nothing,
we just properly set destructor to sock_wfree and adjust sk->sk_wmem_alloc,

It is 100% equivalent of code used with skb_realloc_headroom(),
and there was no claims on this.
Cristoph's reproducer do not use shared skb and to not check this path,
so it cannot be the reason of troubles in his experiments.

Old destructor (sock_edemux?) can be calleda bit later, for old skb, inside consume_skb().
It can decrement last refcount and can trigger sk_free(). However in this case
adjusted sk_wmem_alloc did not allow to free sk.

So I'm sure it is safe.

>> +			skb_set_owner_w(skb, sk);
>> +		consume_skb(oskb);
>> +	} else if (sk) {
> 
> && (skb->destructor != sock_edemux)
> (Because in this case , pskb_expand_head() already adjusted skb->truesize)

Agree, thank you, my fault, I've missed it.
I think it was the reason of the troubles in last Cristoph's experiment.

>> +		delta = osize - skb_end_offset(skb);
> 
>> +		if (!is_skb_wmem(skb))
>> +			skb_set_owner_w(skb, sk);
> 
> This is dangerous, even if a socket is there, its sk->sk_wmem_alloc could be zero.
> We can not add skb->truesize to a refcount_t that already reached 0 (sk_free())
> 
> If is_skb_wmem() is false, you probably should do nothing, and leave
> current destructor as it is.

I;m still not sure and think it is tricky too.
I've found few destructors called sock_wfree inside, they require sk_wmem_alloc adjustement.
sctp_wfree, unix_destruct_scm and tpacket_destruct_skb

In the same time another ones do not use sk_wmem_alloc and I do not know how to detect proper ones.
Potentially there are some 3rd party protocols out-of-tree, and I cannot list all of them here.

However I think I can use the same trick as one described above:
I can increase sk_wmem_alloc before skb_orphan(), so sk_free() called by old destuctor 
cannot call __sk_free() and release sk.

I hope this should work, 
otherwise we'll need to clone skb for !is_skb_wmem(skb) before pskb_expand_head() call.

Thank you,
	Vasily Averin

  reply	other threads:[~2021-09-02  3:59 UTC|newest]

Thread overview: 106+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <cover.1625665132.git.vvs@virtuozzo.com>
2021-07-07 14:04 ` [PATCH IPV6 1/1] ipv6: allocate enough headroom in ip6_finish_output2() Vasily Averin
2021-07-07 14:45   ` David Ahern
2021-07-07 16:42     ` Jakub Kicinski
2021-07-07 17:41       ` Eric Dumazet
2021-07-07 17:53         ` Vasily Averin
2021-07-07 18:30         ` Jakub Kicinski
2021-07-07 18:50           ` Eric Dumazet
2021-07-09  9:04         ` [PATCH IPV6 v2 0/4] " Vasily Averin
2021-07-12  6:44           ` [PATCH IPV6 v3 0/1] " Vasily Averin
     [not found]           ` <cover.1626069562.git.vvs@virtuozzo.com>
2021-07-12  6:45             ` [PATCH IPV6 v3 1/1] " Vasily Averin
2021-07-12 18:30               ` patchwork-bot+netdevbpf
2021-07-13  7:46               ` Vasily Averin
2021-07-13 12:01                 ` [PATCH NET v4 0/1] " Vasily Averin
     [not found]                 ` <cover.1626177047.git.vvs@virtuozzo.com>
2021-07-13 12:01                   ` [PATCH NET v4 1/1] " Vasily Averin
2021-07-18 10:44                     ` Vasily Averin
2021-07-18 15:22                       ` David Ahern
2021-07-18 17:04                       ` David Miller
2021-07-19  7:55                         ` [PATCH NET] ipv6: ip6_finish_output2: set sk into newly allocated nskb Vasily Averin
2021-07-20 10:10                           ` patchwork-bot+netdevbpf
2021-07-13 12:31                 ` [PATCH IPV6 v3 1/1] ipv6: allocate enough headroom in ip6_finish_output2() Vasily Averin
2021-07-12 13:26           ` [PATCH NET 0/7] skbuff: introduce pskb_realloc_headroom() Vasily Averin
     [not found]           ` <cover.1626093470.git.vvs@virtuozzo.com>
2021-07-12 13:26             ` [PATCH NET 1/7] " Vasily Averin
2021-07-12 17:53               ` Jakub Kicinski
2021-07-12 18:45                 ` Vasily Averin
2021-07-13 20:57                   ` [PATCH NET v2 0/7] skbuff: introduce skb_expand_head() Vasily Averin
2021-08-02  8:52                     ` [PATCH NET v3 " Vasily Averin
     [not found]                     ` <cover.1627891754.git.vvs@virtuozzo.com>
2021-08-02  8:52                       ` [PATCH NET v3 1/7] " Vasily Averin
2021-08-02  8:52                       ` [PATCH NET v3 2/7] ipv6: use skb_expand_head in ip6_finish_output2 Vasily Averin
2021-08-02  8:52                       ` [PATCH NET v3 3/7] ipv6: use skb_expand_head in ip6_xmit Vasily Averin
2021-08-02  8:52                       ` [PATCH NET v3 4/7] ipv4: use skb_expand_head in ip_finish_output2 Vasily Averin
2021-08-02  8:52                       ` [PATCH NET v3 5/7] vrf: use skb_expand_head in vrf_finish_output Vasily Averin
2021-08-05 11:55                         ` Julian Wiedmann
2021-08-05 12:55                           ` Vasily Averin
2021-08-06  7:49                           ` [PATCH NET v4 0/7] skbuff: introduce skb_expand_head() Vasily Averin
2021-08-06 10:14                             ` David Miller
2021-08-06 12:53                               ` [PATCH NET] vrf: fix null pointer dereference in vrf_finish_output() Vasily Averin
2021-08-06 22:42                                 ` Jakub Kicinski
2021-08-07  6:41                                   ` Vasily Averin
     [not found]                           ` <cover.1628235065.git.vvs@virtuozzo.com>
2021-08-06  7:49                             ` [PATCH NET v4 1/7] skbuff: introduce skb_expand_head() Vasily Averin
2021-08-06  7:50                             ` [PATCH NET v4 2/7] ipv6: use skb_expand_head in ip6_finish_output2 Vasily Averin
2021-08-06  7:50                             ` [PATCH NET v4 3/7] ipv6: use skb_expand_head in ip6_xmit Vasily Averin
     [not found]                               ` <CALMXkpaay1y=0tkbnskr4gf-HTMjJJsVryh4Prnej_ws-hJvBg@mail.gmail.com>
2021-08-20 22:44                                 ` Christoph Paasch
2021-08-21  6:21                                   ` Vasily Averin
2021-08-22 17:04                                     ` Christoph Paasch
2021-08-22 17:13                                       ` Christoph Paasch
2021-08-23  5:44                                         ` Vasily Averin
2021-08-23  5:59                                           ` Vasily Averin
2021-08-23  7:56                                             ` [PATCH NET-NEXT] ipv6: skb_expand_head() adjust skb->truesize incorrectly Vasily Averin
2021-08-23 17:25                                               ` Christoph Paasch
2021-08-23 21:45                                                 ` Eric Dumazet
2021-08-23 21:51                                                   ` Eric Dumazet
2021-08-23 22:23                                                     ` Eric Dumazet
2021-08-24  8:50                                                       ` Vasily Averin
2021-08-24 17:21                                                         ` Vasily Averin
2021-08-25 17:49                                                           ` Christoph Paasch
2021-08-29 12:59                                                             ` [PATCH v2] " Vasily Averin
2021-08-30  5:52                                                               ` [PATCH net-next " Vasily Averin
2021-08-30 16:01                                                               ` [PATCH " Eric Dumazet
2021-08-30 18:09                                                                 ` Vasily Averin
2021-08-30 18:37                                                                   ` Vasily Averin
2021-08-30 19:58                                                                   ` Eric Dumazet
2021-08-31 14:34                                                                     ` [PATCH net-next v3 RFC] " Vasily Averin
2021-08-31 19:38                                                                       ` Eric Dumazet
2021-09-01  6:20                                                                         ` Vasily Averin
2021-09-01  8:11                                                                           ` [PATCH net-next v4] " Vasily Averin
2021-09-01 16:58                                                                             ` Christoph Paasch
2021-09-01 19:17                                                                             ` Eric Dumazet
2021-09-02  3:59                                                                               ` Vasily Averin [this message]
2021-09-02  4:32                                                                                 ` Eric Dumazet
2021-09-02  4:48                                                                                   ` Eric Dumazet
2021-09-02  7:13                                                                                     ` Vasily Averin
2021-09-02  7:33                                                                                       ` Vasily Averin
2021-09-02  8:31                                                                                         ` Vasily Averin
2021-09-02 11:12                                                                                           ` [PATCH net-next v5] " Vasily Averin
2021-09-02 15:53                                                                                             ` Christoph Paasch
2021-09-02 16:32                                                                                               ` Vasily Averin
2021-09-06 18:01                                                                                                 ` [PATCH net v6] " Vasily Averin
2021-09-06 18:03                                                                                                   ` Vasily Averin
2021-08-27 15:23                                                       ` [PATCH NET-NEXT] ipv6: " Vasily Averin
2021-08-27 16:47                                                         ` Eric Dumazet
2021-08-28  8:01                                                           ` Vasily Averin
2021-08-06  7:50                             ` [PATCH NET v4 4/7] ipv4: use skb_expand_head in ip_finish_output2 Vasily Averin
2021-08-06  7:50                             ` [PATCH NET v4 5/7] vrf: use skb_expand_head in vrf_finish_output Vasily Averin
2021-08-06  7:50                             ` [PATCH NET v4 6/7] ax25: use skb_expand_head Vasily Averin
2021-08-06  7:50                             ` [PATCH NET v4 7/7] bpf: use skb_expand_head in bpf_out_neigh_v4/6 Vasily Averin
2021-08-02  8:52                       ` [PATCH NET v3 6/7] ax25: use skb_expand_head Vasily Averin
2021-08-02  8:52                       ` [PATCH NET v3 7/7] bpf: use skb_expand_head in bpf_out_neigh_v4/6 Vasily Averin
     [not found]                   ` <cover.1626206993.git.vvs@virtuozzo.com>
2021-07-13 20:57                     ` [PATCH NET v2 1/7] skbuff: introduce skb_expand_head() Vasily Averin
2021-07-13 20:58                     ` [PATCH NET v2 2/7] ipv6: use skb_expand_head in ip6_finish_output2 Vasily Averin
2021-07-13 20:58                     ` [PATCH NET v2 3/7] ipv6: use skb_expand_head in ip6_xmit Vasily Averin
2021-07-13 20:58                     ` [PATCH NET v2 4/7] ipv4: use skb_expand_head in ip_finish_output2 Vasily Averin
2021-07-13 20:58                     ` [PATCH NET v2 5/7] vrf: use skb_expand_head in vrf_finish_output Vasily Averin
2021-07-13 20:58                     ` [PATCH NET v2 6/7] ax25: use skb_expand_head Vasily Averin
2021-07-13 20:58                     ` [PATCH NET v2 7/7] bpf: use skb_expand_head in bpf_out_neigh_v4/6 Vasily Averin
2021-07-12 13:26             ` [PATCH NET 2/7] ipv6: use pskb_realloc_headroom in ip6_finish_output2 Vasily Averin
2021-07-12 13:26             ` [PATCH NET 3/7] ipv6: use pskb_realloc_headroom in ip6_xmit refactoring Vasily Averin
2021-07-12 13:27             ` [PATCH NET 4/7] ipv4: use pskb_realloc_headroom in ip_finish_output2 Vasily Averin
2021-07-12 13:27             ` [PATCH NET 5/7] vrf: use pskb_realloc_headroom in vrf_finish_output Vasily Averin
2021-07-12 13:27             ` [PATCH NET 6/7] ax25: use pskb_realloc_headroom Vasily Averin
2021-07-12 13:27             ` [PATCH NET 7/7] bpf: use pskb_realloc_headroom in bpf_out_neigh_v4/6 Vasily Averin
     [not found]         ` <cover.1625818825.git.vvs@virtuozzo.com>
2021-07-09  9:04           ` [PATCH IPV6 v2 1/4] ipv6: allocate enough headroom in ip6_finish_output2() Vasily Averin
2021-07-09 17:58             ` David Miller
2021-07-10  2:53               ` Vasily Averin
2021-07-09  9:04           ` [PATCH IPV6 v2 2/4] ipv6: use new helper skb_expand_head() in ip6_xmit() Vasily Averin
2021-07-09  9:05           ` [PATCH IPV6 v2 3/4] ipv6: ip6_finish_output2 refactoring Vasily Averin
2021-07-09  9:05           ` [PATCH IPV6 v2 4/4] ipv6: ip6_xmit refactoring Vasily Averin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b7c2cb05-7307-f04e-530e-89fc466aa83f@virtuozzo.com \
    --to=vvs@virtuozzo.com \
    --cc=christoph.paasch@gmail.com \
    --cc=davem@davemloft.net \
    --cc=dsahern@kernel.org \
    --cc=eric.dumazet@gmail.com \
    --cc=jwi@linux.ibm.com \
    --cc=kernel@openvz.org \
    --cc=kuba@kernel.org \
    --cc=kuznet@ms2.inr.ac.ru \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=yoshfuji@linux-ipv6.org \
    --subject='Re: [PATCH net-next v4] skb_expand_head() adjust skb->truesize incorrectly' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).