From: Christoph Paasch <hidden> Date: 2021-08-20 22:44:24
(resend without html - thanks gmail web-interface...)
On Fri, Aug 20, 2021 at 3:41 PM Christoph Paasch
[off-list ref] wrote:
Hello,
On Fri, Aug 6, 2021 at 1:18 AM Vasily Averin [off-list ref] wrote:
quoted
Unlike skb_realloc_headroom, new helper skb_expand_head
does not allocate a new skb if possible.
Additionally this patch replaces commonly used dereferencing with variables.
Signed-off-by: Vasily Averin <redacted>
---
net/ipv6/ip6_output.c | 27 +++++++++++----------------
1 file changed, 11 insertions(+), 16 deletions(-)
(resend without html - thanks gmail web-interface...)
On Fri, Aug 20, 2021 at 3:41 PM Christoph Paasch
quoted
AFAICS, this is because pskb_expand_head (called from
skb_expand_head) is not adjusting skb->truesize when skb->sk is set
(which I guess is the case in this particular scenario). I'm not
sure what the proper fix would be though...
Could you please elaborate?
it seems to me skb_realloc_headroom used before my patch called pskb_expand_head() too
and did not adjusted skb->truesize too. Am I missed something perhaps?
The only difference in my patch is that skb_clone can be not called,
though I do not understand how this can affect skb->truesize.
Thank you,
Vasily Averin
From: Christoph Paasch <hidden> Date: 2021-08-22 17:04:32
Hello Vasily,
On Fri, Aug 20, 2021 at 11:21 PM Vasily Averin [off-list ref] wrote:
On 8/21/21 1:44 AM, Christoph Paasch wrote:
quoted
(resend without html - thanks gmail web-interface...)
On Fri, Aug 20, 2021 at 3:41 PM Christoph Paasch
quoted
AFAICS, this is because pskb_expand_head (called from
skb_expand_head) is not adjusting skb->truesize when skb->sk is set
(which I guess is the case in this particular scenario). I'm not
sure what the proper fix would be though...
Could you please elaborate?
it seems to me skb_realloc_headroom used before my patch called pskb_expand_head() too
and did not adjusted skb->truesize too. Am I missed something perhaps?
The only difference in my patch is that skb_clone can be not called,
though I do not understand how this can affect skb->truesize.
I *believe* that the difference is that after skb_clone() skb->sk is
NULL and thus truesize will be adjusted.
I will try to confirm that with some more debugging.
Christoph
From: Christoph Paasch <hidden> Date: 2021-08-22 17:13:48
On Sun, Aug 22, 2021 at 10:04 AM Christoph Paasch
[off-list ref] wrote:
Hello Vasily,
On Fri, Aug 20, 2021 at 11:21 PM Vasily Averin [off-list ref] wrote:
quoted
On 8/21/21 1:44 AM, Christoph Paasch wrote:
quoted
(resend without html - thanks gmail web-interface...)
On Fri, Aug 20, 2021 at 3:41 PM Christoph Paasch
quoted
AFAICS, this is because pskb_expand_head (called from
skb_expand_head) is not adjusting skb->truesize when skb->sk is set
(which I guess is the case in this particular scenario). I'm not
sure what the proper fix would be though...
Could you please elaborate?
it seems to me skb_realloc_headroom used before my patch called pskb_expand_head() too
and did not adjusted skb->truesize too. Am I missed something perhaps?
The only difference in my patch is that skb_clone can be not called,
though I do not understand how this can affect skb->truesize.
I *believe* that the difference is that after skb_clone() skb->sk is
NULL and thus truesize will be adjusted.
I will try to confirm that with some more debugging.
Yes indeed.
Before your patch:
[ 19.154039] ip6_xmit before realloc truesize 4864 sk? 000000002ccd6868
[ 19.155230] ip6_xmit after realloc truesize 5376 sk? 0000000000000000
skb->sk is not set and thus truesize will be adjusted.
With your change:
[ 15.092933] ip6_xmit before realloc truesize 4864 sk? 00000000072930fd
[ 15.094131] ip6_xmit after realloc truesize 4864 sk? 00000000072930fd
skb->sk is set and thus truesize is not adjusted.
Christoph
On Sun, Aug 22, 2021 at 10:04 AM Christoph Paasch
[off-list ref] wrote:
quoted
Hello Vasily,
On Fri, Aug 20, 2021 at 11:21 PM Vasily Averin [off-list ref] wrote:
quoted
On 8/21/21 1:44 AM, Christoph Paasch wrote:
quoted
(resend without html - thanks gmail web-interface...)
On Fri, Aug 20, 2021 at 3:41 PM Christoph Paasch
quoted
AFAICS, this is because pskb_expand_head (called from
skb_expand_head) is not adjusting skb->truesize when skb->sk is set
(which I guess is the case in this particular scenario). I'm not
sure what the proper fix would be though...
Could you please elaborate?
it seems to me skb_realloc_headroom used before my patch called pskb_expand_head() too
and did not adjusted skb->truesize too. Am I missed something perhaps?
The only difference in my patch is that skb_clone can be not called,
though I do not understand how this can affect skb->truesize.
I *believe* that the difference is that after skb_clone() skb->sk is
NULL and thus truesize will be adjusted.
I will try to confirm that with some more debugging.
Yes indeed.
Before your patch:
[ 19.154039] ip6_xmit before realloc truesize 4864 sk? 000000002ccd6868
[ 19.155230] ip6_xmit after realloc truesize 5376 sk? 0000000000000000
skb->sk is not set and thus truesize will be adjusted.
This looks strange for me. skb should not lost sk reference.
Could you please clarify where exactly you cheked it?
sk on newly allocated skb is set on line 291
net/ipv6/ip6_output.c::ip6_xmit()
282 if (unlikely(skb_headroom(skb) < head_room)) {
283 struct sk_buff *skb2 = skb_realloc_headroom(skb, head_room);
284 if (!skb2) {
285 IP6_INC_STATS(net, ip6_dst_idev(skb_dst(skb)),
286 IPSTATS_MIB_OUTDISCARDS);
287 kfree_skb(skb);
288 return -ENOBUFS;
289 }
290 if (skb->sk)
291 skb_set_owner_w(skb2, skb->sk); <<<<< here
292 consume_skb(skb);
293 skb = skb2;
294 }
With your change:
[ 15.092933] ip6_xmit before realloc truesize 4864 sk? 00000000072930fd
[ 15.094131] ip6_xmit after realloc truesize 4864 sk? 00000000072930fd
skb->sk is set and thus truesize is not adjusted.
In this case skb_set_owner_w() is called inside skb_expand_head()
net/ipv6/ip6_output.c::ip6_xmit()
265 if (unlikely(head_room > skb_headroom(skb))) {
266 skb = skb_expand_head(skb, head_room);
267 if (!skb) {
268 IP6_INC_STATS(net, idev, IPSTATS_MIB_OUTDISCARDS);
269 return -ENOBUFS;
270 }
271 }
net/core/skbuff.c::skb_expand_head()
1813 if (skb_shared(skb)) {
1814 struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
1815
1816 if (likely(nskb)) {
1817 if (skb->sk)
1818 skb_set_owner_w(nskb, skb->sk); <<<< here
1819 consume_skb(skb);
1820 } else {
1821 kfree_skb(skb);
1822 }
1823 skb = nskb;
1824 }
So I do not understand how this can happen.
With my patch:
a) if skb is not shared -- it should keep original skb->sk
b) if skb is shared -- new skb should set sk if it was set on original skb.
Your results can be explained if you looked and skb->sk and truesize right after skb_realloc_headroom() call
but before following skb_set_owner_w(). Could you please check it?
Thank you,
Vasily Averin
On Sun, Aug 22, 2021 at 10:04 AM Christoph Paasch
[off-list ref] wrote:
quoted
Hello Vasily,
On Fri, Aug 20, 2021 at 11:21 PM Vasily Averin [off-list ref] wrote:
quoted
On 8/21/21 1:44 AM, Christoph Paasch wrote:
quoted
(resend without html - thanks gmail web-interface...)
On Fri, Aug 20, 2021 at 3:41 PM Christoph Paasch
quoted
AFAICS, this is because pskb_expand_head (called from
skb_expand_head) is not adjusting skb->truesize when skb->sk is set
(which I guess is the case in this particular scenario). I'm not
sure what the proper fix would be though...
Could you please elaborate?
it seems to me skb_realloc_headroom used before my patch called pskb_expand_head() too
and did not adjusted skb->truesize too. Am I missed something perhaps?
The only difference in my patch is that skb_clone can be not called,
though I do not understand how this can affect skb->truesize.
I *believe* that the difference is that after skb_clone() skb->sk is
NULL and thus truesize will be adjusted.
I will try to confirm that with some more debugging.
Yes indeed.
Before your patch:
[ 19.154039] ip6_xmit before realloc truesize 4864 sk? 000000002ccd6868
[ 19.155230] ip6_xmit after realloc truesize 5376 sk? 0000000000000000
skb->sk is not set and thus truesize will be adjusted.
This looks strange for me. skb should not lost sk reference.
Could you please clarify where exactly you cheked it?
sk on newly allocated skb is set on line 291
net/ipv6/ip6_output.c::ip6_xmit()
282 if (unlikely(skb_headroom(skb) < head_room)) {
283 struct sk_buff *skb2 = skb_realloc_headroom(skb, head_room);
284 if (!skb2) {
285 IP6_INC_STATS(net, ip6_dst_idev(skb_dst(skb)),
286 IPSTATS_MIB_OUTDISCARDS);
287 kfree_skb(skb);
288 return -ENOBUFS;
289 }
290 if (skb->sk)
291 skb_set_owner_w(skb2, skb->sk); <<<<< here
292 consume_skb(skb);
293 skb = skb2;
294 }
quoted
With your change:
[ 15.092933] ip6_xmit before realloc truesize 4864 sk? 00000000072930fd
[ 15.094131] ip6_xmit after realloc truesize 4864 sk? 00000000072930fd
skb->sk is set and thus truesize is not adjusted.
In this case skb_set_owner_w() is called inside skb_expand_head()
net/ipv6/ip6_output.c::ip6_xmit()
265 if (unlikely(head_room > skb_headroom(skb))) {
266 skb = skb_expand_head(skb, head_room);
267 if (!skb) {
268 IP6_INC_STATS(net, idev, IPSTATS_MIB_OUTDISCARDS);
269 return -ENOBUFS;
270 }
271 }
net/core/skbuff.c::skb_expand_head()
1813 if (skb_shared(skb)) {
1814 struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
1815
1816 if (likely(nskb)) {
1817 if (skb->sk)
1818 skb_set_owner_w(nskb, skb->sk); <<<< here
1819 consume_skb(skb);
1820 } else {
1821 kfree_skb(skb);
1822 }
1823 skb = nskb;
1824 }
So I do not understand how this can happen.
With my patch:
a) if skb is not shared -- it should keep original skb->sk
b) if skb is shared -- new skb should set sk if it was set on original skb.
Your results can be explained if you looked and skb->sk and truesize right after skb_realloc_headroom() call
but before following skb_set_owner_w(). Could you please check it?
It seems I've found the reason:
before my change pskb_expand_head() is called for newly cloned skb where sk was not set.
after my change skb->sk is set before following pskb_expand_head() call
On own turn pskb_expand_head() adjust truesize:
net/core/skbuff.c::pskb_expand_head()
1751 /* It is not generally safe to change skb->truesize.
1752 * For the moment, we really care of rx path, or
1753 * when skb is orphaned (not attached to a socket).
1754 */
1755 if (!skb->sk || skb->destructor == sock_edemux)
1756 skb->truesize += size - osize;
1757
1758 return 0;
Could you please confirm it?
Thank you,
Vasily Averin
Christoph Paasch reports [1] about incorrect skb->truesize
after skb_expand_head() call in ip6_xmit.
This happen because skb_set_owner_w() for newly clone skb is called
too early, before pskb_expand_head() where truesize is adjusted for
(!skb-sk) case.
[1] https://lkml.org/lkml/2021/8/20/1082
Reported-by: Christoph Paasch <redacted>
Signed-off-by: Vasily Averin <redacted>
---
net/core/skbuff.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
From: Christoph Paasch <hidden> Date: 2021-08-23 17:25:59
Hello,
On Mon, Aug 23, 2021 at 12:56 AM Vasily Averin [off-list ref] wrote:
quoted hunk
Christoph Paasch reports [1] about incorrect skb->truesize
after skb_expand_head() call in ip6_xmit.
This happen because skb_set_owner_w() for newly clone skb is called
too early, before pskb_expand_head() where truesize is adjusted for
(!skb-sk) case.
[1] https://lkml.org/lkml/2021/8/20/1082
Reported-by: Christoph Paasch <redacted>
Signed-off-by: Vasily Averin <redacted>
---
net/core/skbuff.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
@@ -1811,21 +1813,21 @@ struct sk_buff *skb_expand_head(struct sk_buff *skb, unsigned int headroom)/* pskb_expand_head() might crash, if skb is shared */if(skb_shared(skb)){-structsk_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);-}+nskb=skb_clone(skb,GFP_ATOMIC);skb=nskb;}if(skb&&-pskb_expand_head(skb,SKB_DATA_ALIGN(delta),0,GFP_ATOMIC)){-kfree_skb(skb);+pskb_expand_head(skb,SKB_DATA_ALIGN(delta),0,GFP_ATOMIC))skb=NULL;++if(!skb){+kfree_skb(oskb);+if(nskb)+kfree_skb(nskb);+}elseif(nskb){+if(oskb->sk)+skb_set_owner_w(nskb,oskb->sk);+consume_skb(oskb);
sorry, this does not fix the problem. The syzkaller repro still
triggers the WARN.
When it happens, the skb in ip6_xmit() is not shared as it comes from
__tcp_transmit_skb, where it is skb_clone()'d.
Christoph
From: Eric Dumazet <hidden> Date: 2021-08-23 21:46:03
On 8/23/21 10:25 AM, Christoph Paasch wrote:
Hello,
On Mon, Aug 23, 2021 at 12:56 AM Vasily Averin [off-list ref] wrote:
quoted
Christoph Paasch reports [1] about incorrect skb->truesize
after skb_expand_head() call in ip6_xmit.
This happen because skb_set_owner_w() for newly clone skb is called
too early, before pskb_expand_head() where truesize is adjusted for
(!skb-sk) case.
[1] https://lkml.org/lkml/2021/8/20/1082
Reported-by: Christoph Paasch <redacted>
Signed-off-by: Vasily Averin <redacted>
---
net/core/skbuff.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
@@ -1811,21 +1813,21 @@ struct sk_buff *skb_expand_head(struct sk_buff *skb, unsigned int headroom)/* pskb_expand_head() might crash, if skb is shared */if(skb_shared(skb)){-structsk_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);-}+nskb=skb_clone(skb,GFP_ATOMIC);skb=nskb;}if(skb&&-pskb_expand_head(skb,SKB_DATA_ALIGN(delta),0,GFP_ATOMIC)){-kfree_skb(skb);+pskb_expand_head(skb,SKB_DATA_ALIGN(delta),0,GFP_ATOMIC))skb=NULL;++if(!skb){+kfree_skb(oskb);+if(nskb)+kfree_skb(nskb);+}elseif(nskb){+if(oskb->sk)+skb_set_owner_w(nskb,oskb->sk);+consume_skb(oskb);
sorry, this does not fix the problem. The syzkaller repro still
triggers the WARN.
When it happens, the skb in ip6_xmit() is not shared as it comes from
__tcp_transmit_skb, where it is skb_clone()'d.
Old code (in skb_realloc_headroom())
was first calling skb2 = skb_clone(skb, GFP_ATOMIC);
At this point, skb2->sk was NULL
So pskb_expand_head(skb2, SKB_DATA_ALIGN(delta), 0, ...) was able to tweak skb2->truesize
I would try :
@@ -1804,6 +1804,7 @@ EXPORT_SYMBOL(skb_realloc_headroom);structsk_buff*skb_expand_head(structsk_buff*skb,unsignedintheadroom){intdelta=headroom-skb_headroom(skb);+structsk_buff*oskb=NULL;if(WARN_ONCE(delta<=0,"%s is expecting an increase in the headroom",__func__))
From: Eric Dumazet <hidden> Date: 2021-08-23 21:51:49
On 8/23/21 2:45 PM, Eric Dumazet wrote:
quoted hunk
On 8/23/21 10:25 AM, Christoph Paasch wrote:
quoted
Hello,
On Mon, Aug 23, 2021 at 12:56 AM Vasily Averin [off-list ref] wrote:
quoted
Christoph Paasch reports [1] about incorrect skb->truesize
after skb_expand_head() call in ip6_xmit.
This happen because skb_set_owner_w() for newly clone skb is called
too early, before pskb_expand_head() where truesize is adjusted for
(!skb-sk) case.
[1] https://lkml.org/lkml/2021/8/20/1082
Reported-by: Christoph Paasch <redacted>
Signed-off-by: Vasily Averin <redacted>
---
net/core/skbuff.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
@@ -1811,21 +1813,21 @@ struct sk_buff *skb_expand_head(struct sk_buff *skb, unsigned int headroom)/* pskb_expand_head() might crash, if skb is shared */if(skb_shared(skb)){-structsk_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);-}+nskb=skb_clone(skb,GFP_ATOMIC);skb=nskb;}if(skb&&-pskb_expand_head(skb,SKB_DATA_ALIGN(delta),0,GFP_ATOMIC)){-kfree_skb(skb);+pskb_expand_head(skb,SKB_DATA_ALIGN(delta),0,GFP_ATOMIC))skb=NULL;++if(!skb){+kfree_skb(oskb);+if(nskb)+kfree_skb(nskb);+}elseif(nskb){+if(oskb->sk)+skb_set_owner_w(nskb,oskb->sk);+consume_skb(oskb);
sorry, this does not fix the problem. The syzkaller repro still
triggers the WARN.
When it happens, the skb in ip6_xmit() is not shared as it comes from
__tcp_transmit_skb, where it is skb_clone()'d.
Old code (in skb_realloc_headroom())
was first calling skb2 = skb_clone(skb, GFP_ATOMIC);
At this point, skb2->sk was NULL
So pskb_expand_head(skb2, SKB_DATA_ALIGN(delta), 0, ...) was able to tweak skb2->truesize
I would try :
@@ -1804,6 +1804,7 @@ EXPORT_SYMBOL(skb_realloc_headroom);structsk_buff*skb_expand_head(structsk_buff*skb,unsignedintheadroom){intdelta=headroom-skb_headroom(skb);+structsk_buff*oskb=NULL;if(WARN_ONCE(delta<=0,"%s is expecting an increase in the headroom",__func__))
From: Eric Dumazet <hidden> Date: 2021-08-23 22:23:36
On 8/23/21 2:51 PM, Eric Dumazet wrote:
On 8/23/21 2:45 PM, Eric Dumazet wrote:
quoted
On 8/23/21 10:25 AM, Christoph Paasch wrote:
quoted
Hello,
On Mon, Aug 23, 2021 at 12:56 AM Vasily Averin [off-list ref] wrote:
quoted
Christoph Paasch reports [1] about incorrect skb->truesize
after skb_expand_head() call in ip6_xmit.
This happen because skb_set_owner_w() for newly clone skb is called
too early, before pskb_expand_head() where truesize is adjusted for
(!skb-sk) case.
[1] https://lkml.org/lkml/2021/8/20/1082
Reported-by: Christoph Paasch <redacted>
Signed-off-by: Vasily Averin <redacted>
---
net/core/skbuff.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
@@ -1811,21 +1813,21 @@ struct sk_buff *skb_expand_head(struct sk_buff *skb, unsigned int headroom)/* pskb_expand_head() might crash, if skb is shared */if(skb_shared(skb)){-structsk_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);-}+nskb=skb_clone(skb,GFP_ATOMIC);skb=nskb;}if(skb&&-pskb_expand_head(skb,SKB_DATA_ALIGN(delta),0,GFP_ATOMIC)){-kfree_skb(skb);+pskb_expand_head(skb,SKB_DATA_ALIGN(delta),0,GFP_ATOMIC))skb=NULL;++if(!skb){+kfree_skb(oskb);+if(nskb)+kfree_skb(nskb);+}elseif(nskb){+if(oskb->sk)+skb_set_owner_w(nskb,oskb->sk);+consume_skb(oskb);
sorry, this does not fix the problem. The syzkaller repro still
triggers the WARN.
When it happens, the skb in ip6_xmit() is not shared as it comes from
__tcp_transmit_skb, where it is skb_clone()'d.
Old code (in skb_realloc_headroom())
was first calling skb2 = skb_clone(skb, GFP_ATOMIC);
At this point, skb2->sk was NULL
So pskb_expand_head(skb2, SKB_DATA_ALIGN(delta), 0, ...) was able to tweak skb2->truesize
I would try :
@@ -1804,6 +1804,7 @@ EXPORT_SYMBOL(skb_realloc_headroom);structsk_buff*skb_expand_head(structsk_buff*skb,unsignedintheadroom){intdelta=headroom-skb_headroom(skb);+structsk_buff*oskb=NULL;if(WARN_ONCE(delta<=0,"%s is expecting an increase in the headroom",__func__))
Oh well, probably not going to work.
We have to find a way to properly increase skb->truesize, even if skb_clone() is _not_ called.
I also note that current use of skb_set_owner_w(), forcing skb->destructor to sock_wfree()
is probably breaking TCP Small queues, since original skb->destructor would be tcp_wfree() or __sock_wfree()
Hello,
On Mon, Aug 23, 2021 at 12:56 AM Vasily Averin [off-list ref] wrote:
quoted
Christoph Paasch reports [1] about incorrect skb->truesize
after skb_expand_head() call in ip6_xmit.
This happen because skb_set_owner_w() for newly clone skb is called
too early, before pskb_expand_head() where truesize is adjusted for
(!skb-sk) case.
[1] https://lkml.org/lkml/2021/8/20/1082
Reported-by: Christoph Paasch <redacted>
Signed-off-by: Vasily Averin <redacted>
---
net/core/skbuff.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
@@ -1811,21 +1813,21 @@ struct sk_buff *skb_expand_head(struct sk_buff *skb, unsigned int headroom)/* pskb_expand_head() might crash, if skb is shared */if(skb_shared(skb)){-structsk_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);-}+nskb=skb_clone(skb,GFP_ATOMIC);skb=nskb;}if(skb&&-pskb_expand_head(skb,SKB_DATA_ALIGN(delta),0,GFP_ATOMIC)){-kfree_skb(skb);+pskb_expand_head(skb,SKB_DATA_ALIGN(delta),0,GFP_ATOMIC))skb=NULL;++if(!skb){+kfree_skb(oskb);+if(nskb)+kfree_skb(nskb);+}elseif(nskb){+if(oskb->sk)+skb_set_owner_w(nskb,oskb->sk);+consume_skb(oskb);
sorry, this does not fix the problem. The syzkaller repro still
triggers the WARN.
When it happens, the skb in ip6_xmit() is not shared as it comes from
__tcp_transmit_skb, where it is skb_clone()'d.
Old code (in skb_realloc_headroom())
was first calling skb2 = skb_clone(skb, GFP_ATOMIC);
At this point, skb2->sk was NULL
So pskb_expand_head(skb2, SKB_DATA_ALIGN(delta), 0, ...) was able to tweak skb2->truesize
I would try :
@@ -1804,6 +1804,7 @@ EXPORT_SYMBOL(skb_realloc_headroom);structsk_buff*skb_expand_head(structsk_buff*skb,unsignedintheadroom){intdelta=headroom-skb_headroom(skb);+structsk_buff*oskb=NULL;if(WARN_ONCE(delta<=0,"%s is expecting an increase in the headroom",__func__))
Oh well, probably not going to work.
We have to find a way to properly increase skb->truesize, even if skb_clone() is _not_ called.
Can we adjust truesize outside pskb_expand_head()?
Could you please explain why it can be not safe?
I also note that current use of skb_set_owner_w(), forcing skb->destructor to sock_wfree()
is probably breaking TCP Small queues, since original skb->destructor would be tcp_wfree() or __sock_wfree()
I agree, however as far as I understand it is separate and more global problem.
Thank you,
Vasily Averin
Hello,
On Mon, Aug 23, 2021 at 12:56 AM Vasily Averin [off-list ref] wrote:
quoted
Christoph Paasch reports [1] about incorrect skb->truesize
after skb_expand_head() call in ip6_xmit.
This happen because skb_set_owner_w() for newly clone skb is called
too early, before pskb_expand_head() where truesize is adjusted for
(!skb-sk) case.
[1] https://lkml.org/lkml/2021/8/20/1082
Reported-by: Christoph Paasch <redacted>
Signed-off-by: Vasily Averin <redacted>
---
net/core/skbuff.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
@@ -1811,21 +1813,21 @@ struct sk_buff *skb_expand_head(struct sk_buff *skb, unsigned int headroom)/* pskb_expand_head() might crash, if skb is shared */if(skb_shared(skb)){-structsk_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);-}+nskb=skb_clone(skb,GFP_ATOMIC);skb=nskb;}if(skb&&-pskb_expand_head(skb,SKB_DATA_ALIGN(delta),0,GFP_ATOMIC)){-kfree_skb(skb);+pskb_expand_head(skb,SKB_DATA_ALIGN(delta),0,GFP_ATOMIC))skb=NULL;++if(!skb){+kfree_skb(oskb);+if(nskb)+kfree_skb(nskb);+}elseif(nskb){+if(oskb->sk)+skb_set_owner_w(nskb,oskb->sk);+consume_skb(oskb);
sorry, this does not fix the problem. The syzkaller repro still
triggers the WARN.
When it happens, the skb in ip6_xmit() is not shared as it comes from
__tcp_transmit_skb, where it is skb_clone()'d.
Old code (in skb_realloc_headroom())
was first calling skb2 = skb_clone(skb, GFP_ATOMIC);
At this point, skb2->sk was NULL
So pskb_expand_head(skb2, SKB_DATA_ALIGN(delta), 0, ...) was able to tweak skb2->truesize
I would try :
@@ -1804,6 +1804,7 @@ EXPORT_SYMBOL(skb_realloc_headroom);structsk_buff*skb_expand_head(structsk_buff*skb,unsignedintheadroom){intdelta=headroom-skb_headroom(skb);+structsk_buff*oskb=NULL;if(WARN_ONCE(delta<=0,"%s is expecting an increase in the headroom",__func__))
Oh well, probably not going to work.
We have to find a way to properly increase skb->truesize, even if skb_clone() is _not_ called.
Can we adjust truesize outside pskb_expand_head()?
Could you please explain why it can be not safe?
Do you mean truesize change should not break balance of sk->sk_wmem_alloc?
quoted
I also note that current use of skb_set_owner_w(), forcing skb->destructor to sock_wfree()
is probably breaking TCP Small queues, since original skb->destructor would be tcp_wfree() or __sock_wfree()
I agree, however as far as I understand it is separate and more global problem.
Thank you,
Vasily Averin
From: Christoph Paasch <hidden> Date: 2021-08-25 17:49:31
On Tue, Aug 24, 2021 at 10:22 AM Vasily Averin [off-list ref] wrote:
On 8/24/21 11:50 AM, Vasily Averin wrote:
quoted
On 8/24/21 1:23 AM, Eric Dumazet wrote:
quoted
On 8/23/21 2:51 PM, Eric Dumazet wrote:
quoted
On 8/23/21 2:45 PM, Eric Dumazet wrote:
quoted
On 8/23/21 10:25 AM, Christoph Paasch wrote:
quoted
Hello,
On Mon, Aug 23, 2021 at 12:56 AM Vasily Averin [off-list ref] wrote:
quoted
Christoph Paasch reports [1] about incorrect skb->truesize
after skb_expand_head() call in ip6_xmit.
This happen because skb_set_owner_w() for newly clone skb is called
too early, before pskb_expand_head() where truesize is adjusted for
(!skb-sk) case.
[1] https://lkml.org/lkml/2021/8/20/1082
Reported-by: Christoph Paasch <redacted>
Signed-off-by: Vasily Averin <redacted>
---
net/core/skbuff.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
@@ -1811,21 +1813,21 @@ struct sk_buff *skb_expand_head(struct sk_buff *skb, unsigned int headroom)/* pskb_expand_head() might crash, if skb is shared */if(skb_shared(skb)){-structsk_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);-}+nskb=skb_clone(skb,GFP_ATOMIC);skb=nskb;}if(skb&&-pskb_expand_head(skb,SKB_DATA_ALIGN(delta),0,GFP_ATOMIC)){-kfree_skb(skb);+pskb_expand_head(skb,SKB_DATA_ALIGN(delta),0,GFP_ATOMIC))skb=NULL;++if(!skb){+kfree_skb(oskb);+if(nskb)+kfree_skb(nskb);+}elseif(nskb){+if(oskb->sk)+skb_set_owner_w(nskb,oskb->sk);+consume_skb(oskb);
sorry, this does not fix the problem. The syzkaller repro still
triggers the WARN.
When it happens, the skb in ip6_xmit() is not shared as it comes from
__tcp_transmit_skb, where it is skb_clone()'d.
Old code (in skb_realloc_headroom())
was first calling skb2 = skb_clone(skb, GFP_ATOMIC);
At this point, skb2->sk was NULL
So pskb_expand_head(skb2, SKB_DATA_ALIGN(delta), 0, ...) was able to tweak skb2->truesize
I would try :
@@ -1804,6 +1804,7 @@ EXPORT_SYMBOL(skb_realloc_headroom);structsk_buff*skb_expand_head(structsk_buff*skb,unsignedintheadroom){intdelta=headroom-skb_headroom(skb);+structsk_buff*oskb=NULL;if(WARN_ONCE(delta<=0,"%s is expecting an increase in the headroom",__func__))
Oh well, probably not going to work.
We have to find a way to properly increase skb->truesize, even if skb_clone() is _not_ called.
Can we adjust truesize outside pskb_expand_head()?
Could you please explain why it can be not safe?
Do you mean truesize change should not break balance of sk->sk_wmem_alloc?
AFAICS, that's the problem around adjusting truesize. So, maybe "just"
refcount_add the increase of the truesize.
The below does fix the syzkaller bug for me and seems to do the right
thing overall. But I honestly think that this is becoming too hacky
and not worth it... and who knows what other corner-cases this now
exposes...
Maybe a revert is a better course of action?
---
@@ -1756,9 +1757,14 @@ int pskb_expand_head(struct sk_buff *skb, int
nhead, int ntail,
* For the moment, we really care of rx path, or
* when skb is orphaned (not attached to a socket).
*/
- if (!skb->sk || skb->destructor == sock_edemux)
+ if (!skb->sk || skb->destructor == sock_edemux || skb->destructor ==
tcp_wfree) {
skb->truesize += size - osize;
+ if (skb->sk && skb->destructor == tcp_wfree) {
+ refcount_add(size - osize, &skb->sk->sk_wmem_alloc);
+ }
+ }
+
return 0;
nofrags:
Hello,
On Mon, Aug 23, 2021 at 12:56 AM Vasily Averin [off-list ref] wrote:
quoted
Christoph Paasch reports [1] about incorrect skb->truesize
after skb_expand_head() call in ip6_xmit.
This happen because skb_set_owner_w() for newly clone skb is called
too early, before pskb_expand_head() where truesize is adjusted for
(!skb-sk) case.
[1] https://lkml.org/lkml/2021/8/20/1082
Reported-by: Christoph Paasch <redacted>
Signed-off-by: Vasily Averin <redacted>
---
net/core/skbuff.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
@@ -1811,21 +1813,21 @@ struct sk_buff *skb_expand_head(struct sk_buff *skb, unsigned int headroom)/* pskb_expand_head() might crash, if skb is shared */if(skb_shared(skb)){-structsk_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);-}+nskb=skb_clone(skb,GFP_ATOMIC);skb=nskb;}if(skb&&-pskb_expand_head(skb,SKB_DATA_ALIGN(delta),0,GFP_ATOMIC)){-kfree_skb(skb);+pskb_expand_head(skb,SKB_DATA_ALIGN(delta),0,GFP_ATOMIC))skb=NULL;++if(!skb){+kfree_skb(oskb);+if(nskb)+kfree_skb(nskb);+}elseif(nskb){+if(oskb->sk)+skb_set_owner_w(nskb,oskb->sk);+consume_skb(oskb);
sorry, this does not fix the problem. The syzkaller repro still
triggers the WARN.
When it happens, the skb in ip6_xmit() is not shared as it comes from
__tcp_transmit_skb, where it is skb_clone()'d.
Old code (in skb_realloc_headroom())
was first calling skb2 = skb_clone(skb, GFP_ATOMIC);
At this point, skb2->sk was NULL
So pskb_expand_head(skb2, SKB_DATA_ALIGN(delta), 0, ...) was able to tweak skb2->truesize
I would try :
@@ -1804,6 +1804,7 @@ EXPORT_SYMBOL(skb_realloc_headroom);structsk_buff*skb_expand_head(structsk_buff*skb,unsignedintheadroom){intdelta=headroom-skb_headroom(skb);+structsk_buff*oskb=NULL;if(WARN_ONCE(delta<=0,"%s is expecting an increase in the headroom",__func__))
Oh well, probably not going to work.
We have to find a way to properly increase skb->truesize, even if skb_clone() is _not_ called.
I also note that current use of skb_set_owner_w(), forcing skb->destructor to sock_wfree()
is probably breaking TCP Small queues, since original skb->destructor would be tcp_wfree() or __sock_wfree()
I asked Alexey Kuznetsov to look at this problem. Below is his answer:
"I think the current scheme is obsolete. It was created
when we had only two kinds of skb accounting (rmem & wmem)
and with more kinds of accounting it just does not work.
Even there we had ignored problems with adjusting accounting.
Logically the best solution would be replacing ->destructor,
set_owner* etc with skb_ops. Something like:
struct skb_ops
{
void init(struct sk_buff * skb, struct skb_ops * ops, struct
sock * owner);
void fini(struct sk_buff * skb);
void update(struct sk_buff * skb, int adjust);
void inherit(struct sk_buff * skb2, struct sk_buff * skb);
};
init - is replacement for skb_set_owner_r|w
fini - is replacement for skb_orphan
update - is new operation to be used in places where skb->truesize changes,
instead of awful constructions like:
if (!skb->sk || skb->destructor == sock_edemux)
skb->truesize += size - osize;
Now it will look like:
if (skb->ops)
skb->ops->update(skb, size - osize);
inherit - is replacement for also awful constructs like:
if (skb->sk)
skb_set_owner_w(skb2, skb->sk);
Now it will be:
if (skb->ops)
skb->ops->inherit(skb2, skb);
The implementation looks mostly obvious.
Some troubles can be only with new functionality:
update of accounting was never done before.
More efficient, functionally equivalent, but uglier and less flexible
alternative would be removal of ->destructor, replaced with
a small numeric indicator of ownership:
enum
{
SKB_OWNER_NONE, /* aka destructor == NULL */
SKB_OWNER_WMEM, /* aka destructor == sk_wfree */
SKB_OWNER_RMEM, /* aka destructor == sk_rfree */
SKB_OWNER_SK, /* aka destructor == sk_edemux */
SKB_OWNER_TCP, /* aka destructor == tcp_wfree */
}
And the same init,fini,inherit,update become functions
w/o any inidirect calls. Not sure it is really more efficient though."
Thank you,
Vasily Averin
From: Eric Dumazet <hidden> Date: 2021-08-27 16:47:37
On 8/27/21 8:23 AM, Vasily Averin wrote:
I asked Alexey Kuznetsov to look at this problem. Below is his answer:
"I think the current scheme is obsolete. It was created
when we had only two kinds of skb accounting (rmem & wmem)
and with more kinds of accounting it just does not work.
Even there we had ignored problems with adjusting accounting.
Logically the best solution would be replacing ->destructor,
set_owner* etc with skb_ops. Something like:
struct skb_ops
{
void init(struct sk_buff * skb, struct skb_ops * ops, struct
sock * owner);
void fini(struct sk_buff * skb);
void update(struct sk_buff * skb, int adjust);
void inherit(struct sk_buff * skb2, struct sk_buff * skb);
};
init - is replacement for skb_set_owner_r|w
fini - is replacement for skb_orphan
update - is new operation to be used in places where skb->truesize changes,
instead of awful constructions like:
if (!skb->sk || skb->destructor == sock_edemux)
skb->truesize += size - osize;
Now it will look like:
if (skb->ops)
skb->ops->update(skb, size - osize);
inherit - is replacement for also awful constructs like:
if (skb->sk)
skb_set_owner_w(skb2, skb->sk);
Now it will be:
if (skb->ops)
skb->ops->inherit(skb2, skb);
The implementation looks mostly obvious.
Some troubles can be only with new functionality:
update of accounting was never done before.
More efficient, functionally equivalent, but uglier and less flexible
alternative would be removal of ->destructor, replaced with
a small numeric indicator of ownership:
enum
{
SKB_OWNER_NONE, /* aka destructor == NULL */
SKB_OWNER_WMEM, /* aka destructor == sk_wfree */
SKB_OWNER_RMEM, /* aka destructor == sk_rfree */
SKB_OWNER_SK, /* aka destructor == sk_edemux */
SKB_OWNER_TCP, /* aka destructor == tcp_wfree */
}
And the same init,fini,inherit,update become functions
w/o any inidirect calls. Not sure it is really more efficient though."
Well, this does not look as stable material, and would add a bunch
of indirect calls which are quite expensive these days (CONFIG_RETPOLINE=y)
I suggest we work on a fix, using existing infra, then eventually later
try to refactor if this is really bringing improvements.
A fix could simply be a revert of 0c9f227bee119 ("ipv6: use skb_expand_head in ip6_xmit")
since only IPv6 has the problem (because of arbitrary headers size)
I asked Alexey Kuznetsov to look at this problem. Below is his answer:
"I think the current scheme is obsolete. It was created
when we had only two kinds of skb accounting (rmem & wmem)
and with more kinds of accounting it just does not work.
Even there we had ignored problems with adjusting accounting.
Logically the best solution would be replacing ->destructor,
set_owner* etc with skb_ops. Something like:
struct skb_ops
{
void init(struct sk_buff * skb, struct skb_ops * ops, struct
sock * owner);
void fini(struct sk_buff * skb);
void update(struct sk_buff * skb, int adjust);
void inherit(struct sk_buff * skb2, struct sk_buff * skb);
};
init - is replacement for skb_set_owner_r|w
fini - is replacement for skb_orphan
update - is new operation to be used in places where skb->truesize changes,
instead of awful constructions like:
if (!skb->sk || skb->destructor == sock_edemux)
skb->truesize += size - osize;
Now it will look like:
if (skb->ops)
skb->ops->update(skb, size - osize);
inherit - is replacement for also awful constructs like:
if (skb->sk)
skb_set_owner_w(skb2, skb->sk);
Now it will be:
if (skb->ops)
skb->ops->inherit(skb2, skb);
The implementation looks mostly obvious.
Some troubles can be only with new functionality:
update of accounting was never done before.
More efficient, functionally equivalent, but uglier and less flexible
alternative would be removal of ->destructor, replaced with
a small numeric indicator of ownership:
enum
{
SKB_OWNER_NONE, /* aka destructor == NULL */
SKB_OWNER_WMEM, /* aka destructor == sk_wfree */
SKB_OWNER_RMEM, /* aka destructor == sk_rfree */
SKB_OWNER_SK, /* aka destructor == sk_edemux */
SKB_OWNER_TCP, /* aka destructor == tcp_wfree */
}
And the same init,fini,inherit,update become functions
w/o any inidirect calls. Not sure it is really more efficient though."
Well, this does not look as stable material, and would add a bunch
of indirect calls which are quite expensive these days (CONFIG_RETPOLINE=y)
I suggest we work on a fix, using existing infra, then eventually later
try to refactor if this is really bringing improvements.
A fix could simply be a revert of 0c9f227bee119 ("ipv6: use skb_expand_head in ip6_xmit")
since only IPv6 has the problem (because of arbitrary headers size)
I think it is not enough.
Root of the problem is that skb_expand_head() works incorrectly with non-shared skb.
In this case it do not call skb_clone before pskb_expand_head() execution,
and as result pskb_expand_head() and does not adjust skb->truesize.
I think non-shared skb is more frequent case,
so all skb_expand_head() are affected.
Therefore we need to revert all my patch set in net-next:
f1260ff skbuff: introduce skb_expand_head()
e415ed3 ipv6: use skb_expand_head in ip6_finish_output2
0c9f227 ipv6: use skb_expand_head in ip6_xmit
5678a59 ipv4: use skb_expand_head in ip_finish_output2
14ee70c vrf: use skb_expand_head in vrf_finish_output
53744a4 ax25: use skb_expand_head
a1e975e bpf: use skb_expand_head in bpf_out_neigh_v4/6
07e1d6b Merge branch 'skb_expand_head'
with fixup
06669e6 vrf: fix NULL dereference in vrf_finish_output()
And then rework ip6_finish_output2() in upstream,
to call skb_realloc_headroom() like it was done in first patch version:
https://lkml.org/lkml/2021/7/7/469.
Thank you,
Vasily Averin
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
Reported-by: Christoph Paasch <redacted>
Signed-off-by: Vasily Averin <redacted>
---
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.
---
net/core/skbuff.c | 43 +++++++++++++++++++++++++++++--------------
1 file changed, 29 insertions(+), 14 deletions(-)
@@ -1756,9 +1756,13 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,*Forthemoment,wereallycareofrxpath,or*whenskbisorphaned(notattachedtoasocket).*/-if(!skb->sk||skb->destructor==sock_edemux)-skb->truesize+=size-osize;-+delta=size-osize;+if(!skb->sk||skb->destructor==sock_edemux){+skb->truesize+=delta;+}elseif(update_truesize){+refcount_add(delta,&skb->sk->sk_wmem_alloc);+skb->truesize+=delta;+}return0;nofrags:
@@ -1766,6 +1770,12 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,nodata:return-ENOMEM;}++intpskb_expand_head(structsk_buff*skb,intnhead,intntail,+gfp_tgfp_mask)+{+return__pskb_expand_head(skb,nhead,ntail,gfp_mask,false);+}EXPORT_SYMBOL(pskb_expand_head);/* Make private copy of skb with writable head and some headroom */
@@ -1804,28 +1814,33 @@ struct sk_buff *skb_realloc_headroom(struct sk_buff *skb, unsigned int headroom)structsk_buff*skb_expand_head(structsk_buff*skb,unsignedintheadroom){intdelta=headroom-skb_headroom(skb);+structsk_buff*oskb=NULL;if(WARN_ONCE(delta<=0,"%s is expecting an increase in the headroom",__func__))returnskb;+delta=SKB_DATA_ALIGN(delta);/* pskb_expand_head() might crash, if skb is shared */if(skb_shared(skb)){structsk_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);+returnNULL;}+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,true)){kfree_skb(skb);-skb=NULL;+kfree_skb(oskb);+returnNULL;+}+if(oskb){+if(oskb->sk)+skb_set_owner_w(skb,oskb->sk);+consume_skb(oskb);}returnskb;}
1) I forgot to specify that the patch is intended fro net-next git
2) I forgot to ad Alexey Kuznetsov in cc. I resend the patch to him
in a separate letter and received his consent.
3) I forgot to set Fixed mark
Fixes: f1260ff15a71 ("skbuff: introduce skb_expand_head()")
Thank you,
Vasily Averin
On 8/29/21 3:59 PM, Vasily Averin wrote:
quoted hunk
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
Reported-by: Christoph Paasch <redacted>
Signed-off-by: Vasily Averin <redacted>
---
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.
---
net/core/skbuff.c | 43 +++++++++++++++++++++++++++++--------------
1 file changed, 29 insertions(+), 14 deletions(-)
@@ -1756,9 +1756,13 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,*Forthemoment,wereallycareofrxpath,or*whenskbisorphaned(notattachedtoasocket).*/-if(!skb->sk||skb->destructor==sock_edemux)-skb->truesize+=size-osize;-+delta=size-osize;+if(!skb->sk||skb->destructor==sock_edemux){+skb->truesize+=delta;+}elseif(update_truesize){+refcount_add(delta,&skb->sk->sk_wmem_alloc);+skb->truesize+=delta;+}return0;nofrags:
@@ -1766,6 +1770,12 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,nodata:return-ENOMEM;}++intpskb_expand_head(structsk_buff*skb,intnhead,intntail,+gfp_tgfp_mask)+{+return__pskb_expand_head(skb,nhead,ntail,gfp_mask,false);+}EXPORT_SYMBOL(pskb_expand_head);/* Make private copy of skb with writable head and some headroom */
@@ -1804,28 +1814,33 @@ struct sk_buff *skb_realloc_headroom(struct sk_buff *skb, unsigned int headroom)structsk_buff*skb_expand_head(structsk_buff*skb,unsignedintheadroom){intdelta=headroom-skb_headroom(skb);+structsk_buff*oskb=NULL;if(WARN_ONCE(delta<=0,"%s is expecting an increase in the headroom",__func__))returnskb;+delta=SKB_DATA_ALIGN(delta);/* pskb_expand_head() might crash, if skb is shared */if(skb_shared(skb)){structsk_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);+returnNULL;}+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,true)){kfree_skb(skb);-skb=NULL;+kfree_skb(oskb);+returnNULL;+}+if(oskb){+if(oskb->sk)+skb_set_owner_w(skb,oskb->sk);+consume_skb(oskb);}returnskb;}
From: Eric Dumazet <hidden> Date: 2021-08-30 16:01:07
On 8/29/21 5:59 AM, Vasily Averin wrote:
quoted hunk
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
Reported-by: Christoph Paasch <redacted>
Signed-off-by: Vasily Averin <redacted>
---
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.
---
net/core/skbuff.c | 43 +++++++++++++++++++++++++++++--------------
1 file changed, 29 insertions(+), 14 deletions(-)
@@ -1756,9 +1756,13 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,*Forthemoment,wereallycareofrxpath,or*whenskbisorphaned(notattachedtoasocket).*/-if(!skb->sk||skb->destructor==sock_edemux)-skb->truesize+=size-osize;-+delta=size-osize;+if(!skb->sk||skb->destructor==sock_edemux){+skb->truesize+=delta;+}elseif(update_truesize){
Unfortunately we can not always do this sk_wmem_alloc change here.
Some skb have skb->sk set, but the 'reference on socket' is not through sk_wmem_alloc
It seems you need a helper to make sure skb->destructor is one of
the destructors that use skb->truesize and sk->sk_wmem_alloc
For instance, skb_orphan_partial() could have been used.
@@ -1766,6 +1770,12 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail, nodata: return -ENOMEM; }++int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,+ gfp_t gfp_mask)+{+ return __pskb_expand_head(skb, nhead, ntail, gfp_mask, false);+} EXPORT_SYMBOL(pskb_expand_head); /* Make private copy of skb with writable head and some headroom */
@@ -1804,28 +1814,33 @@ 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);+ struct sk_buff *oskb = NULL; if (WARN_ONCE(delta <= 0, "%s is expecting an increase in the headroom", __func__)) return skb;+ delta = SKB_DATA_ALIGN(delta); /* pskb_expand_head() might crash, if skb is shared */ 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, true)) { kfree_skb(skb);- skb = NULL;+ kfree_skb(oskb);+ return NULL;+ }+ if (oskb) {+ if (oskb->sk)+ skb_set_owner_w(skb, oskb->sk);+ consume_skb(oskb); } return skb; }
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
@@ -1756,9 +1756,13 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail, * For the moment, we really care of rx path, or * when skb is orphaned (not attached to a socket). */- if (!skb->sk || skb->destructor == sock_edemux)- skb->truesize += size - osize;-+ delta = size - osize;+ if (!skb->sk || skb->destructor == sock_edemux) {+ skb->truesize += delta;+ } else if (update_truesize) {
Unfortunately we can not always do this sk_wmem_alloc change here.
Some skb have skb->sk set, but the 'reference on socket' is not through sk_wmem_alloc
Could you please provide some example?
In past in all handeled cases we have cloned original skb and then unconditionally assigned skb sock_wfree destructor.
Do you want to say that it worked correctly somehow?
I expected if we set sock_wfree, we have guarantee that old skb adjusted sk_wmem_alloc.
Am I wrong?
Could you please point on such case?
It seems you need a helper to make sure skb->destructor is one of
the destructors that use skb->truesize and sk->sk_wmem_alloc
For instance, skb_orphan_partial() could have been used.
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
@@ -1756,9 +1756,13 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail, * For the moment, we really care of rx path, or * when skb is orphaned (not attached to a socket). */- if (!skb->sk || skb->destructor == sock_edemux)- skb->truesize += size - osize;-+ delta = size - osize;+ if (!skb->sk || skb->destructor == sock_edemux) {+ skb->truesize += delta;+ } else if (update_truesize) {
Unfortunately we can not always do this sk_wmem_alloc change here.
Some skb have skb->sk set, but the 'reference on socket' is not through sk_wmem_alloc
Could you please provide some example?
In past in all handeled cases we have cloned original skb and then unconditionally assigned skb sock_wfree destructor.
Do you want to say that it worked correctly somehow?
I expected if we set sock_wfree, we have guarantee that old skb adjusted sk_wmem_alloc.
Am I wrong?
Could you please point on such case?
However if it is true -- it is not enough to adjust sk_wmem_alloc for proper destructors,
because another destructors may require to do something else.
In this case I can check destructor first and clone skb before pskb_expand_head() call,
like it was happen before.
quoted
It seems you need a helper to make sure skb->destructor is one of
the destructors that use skb->truesize and sk->sk_wmem_alloc
For instance, skb_orphan_partial() could have been used.
From: Eric Dumazet <hidden> Date: 2021-08-30 19:58:23
On 8/30/21 11:09 AM, Vasily Averin wrote:
On 8/30/21 7:01 PM, Eric Dumazet wrote:
quoted
On 8/29/21 5:59 AM, Vasily Averin wrote:
quoted
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
@@ -1756,9 +1756,13 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail, * For the moment, we really care of rx path, or * when skb is orphaned (not attached to a socket). */- if (!skb->sk || skb->destructor == sock_edemux)- skb->truesize += size - osize;-+ delta = size - osize;+ if (!skb->sk || skb->destructor == sock_edemux) {+ skb->truesize += delta;+ } else if (update_truesize) {
Unfortunately we can not always do this sk_wmem_alloc change here.
Some skb have skb->sk set, but the 'reference on socket' is not through sk_wmem_alloc
Could you please provide some example?
In past in all handeled cases we have cloned original skb and then unconditionally assigned skb sock_wfree destructor.
In the past we ignored old value of skb->destructor,
since the clone got a NULL destructor.
In your patch you assumes it is sock_wfree, or other destructors changing sk_wmem_alloc
You need to make sure skb->destructor is one of the known destructors which
will basically remove skb->truesize from sk->sk_wmem_alloc.
This will also make sure skb->sk is a 'full socket'
If not, you should not change sk->sk_wmem_alloc
Do you want to say that it worked correctly somehow?
I am simply saying your patch adds a wrong assumption.
I expected if we set sock_wfree, we have guarantee that old skb adjusted sk_wmem_alloc.
Am I wrong?
Could you please point on such case?
quoted
It seems you need a helper to make sure skb->destructor is one of
the destructors that use skb->truesize and sk->sk_wmem_alloc
For instance, skb_orphan_partial() could have been used.
RFC because it have an extra changes:
new is_skb_wmem() helper can be called
- either before pskb_expand_head(), to create skb clones
for skb with destructors that does not change sk->sk_wmem_alloc
- or after pskb_expand_head(), to change owner in skb_set_owner_w()
In current patch I've added both these ways,
we need to keep one of them.
---
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
Reported-by: Christoph Paasch <redacted>
Signed-off-by: Vasily Averin <redacted>
---
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 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 | 39 ++++++++++++++++++++++++++++-----------
net/core/sock.c | 8 ++++++++
3 files changed, 37 insertions(+), 11 deletions(-)
@@ -1804,30 +1804,47 @@ struct sk_buff *skb_realloc_headroom(struct sk_buff *skb, unsigned int headroom)structsk_buff*skb_expand_head(structsk_buff*skb,unsignedintheadroom){intdelta=headroom-skb_headroom(skb);+intosize=skb_end_offset(skb);+structsk_buff*oskb=NULL;+structsock*sk=skb->sk;if(WARN_ONCE(delta<=0,"%s is expecting an increase in the headroom",__func__))returnskb;-/* pskb_expand_head() might crash, if skb is shared */-if(skb_shared(skb)){+delta=SKB_DATA_ALIGN(delta);+/* pskb_expand_head() might crash, if skb is shared.+*Alsoweshouldcloneskbifitsdestructordoes+*notadjustskb->truesizeandsk->sk_wmem_alloc+*/+if(skb_shared(skb)||+(sk&&(!sk_fullsock(sk)||!is_skb_wmem(skb)))){structsk_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);+returnNULL;}+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);+returnNULL;}-returnskb;+if(oskb){+if(sk)+skb_set_owner_w(skb,sk);+consume_skb(oskb);+}elseif(sk){+delta=osize-skb_end_offset(skb);+if(!is_skb_wmem(skb))+skb_set_owner_w(skb,sk);+skb->truesize+=delta;+if(sk_fullsock(sk))+refcount_add(delta,&sk->sk_wmem_alloc);+}returnskb;}EXPORT_SYMBOL(skb_expand_head);
From: Eric Dumazet <hidden> Date: 2021-08-31 19:38:45
On 8/31/21 7:34 AM, Vasily Averin wrote:
quoted hunk
RFC because it have an extra changes:
new is_skb_wmem() helper can be called
- either before pskb_expand_head(), to create skb clones
for skb with destructors that does not change sk->sk_wmem_alloc
- or after pskb_expand_head(), to change owner in skb_set_owner_w()
In current patch I've added both these ways,
we need to keep one of them.
---
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
Reported-by: Christoph Paasch <redacted>
Signed-off-by: Vasily Averin <redacted>
---
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 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 | 39 ++++++++++++++++++++++++++++-----------
net/core/sock.c | 8 ++++++++
3 files changed, 37 insertions(+), 11 deletions(-)
@@ -1804,30 +1804,47 @@ struct sk_buff *skb_realloc_headroom(struct sk_buff *skb, unsigned int headroom)structsk_buff*skb_expand_head(structsk_buff*skb,unsignedintheadroom){intdelta=headroom-skb_headroom(skb);+intosize=skb_end_offset(skb);+structsk_buff*oskb=NULL;+structsock*sk=skb->sk;if(WARN_ONCE(delta<=0,"%s is expecting an increase in the headroom",__func__))returnskb;-/* pskb_expand_head() might crash, if skb is shared */-if(skb_shared(skb)){+delta=SKB_DATA_ALIGN(delta);+/* pskb_expand_head() might crash, if skb is shared.+*Alsoweshouldcloneskbifitsdestructordoes+*notadjustskb->truesizeandsk->sk_wmem_alloc+*/+if(skb_shared(skb)||+(sk&&(!sk_fullsock(sk)||!is_skb_wmem(skb)))){
is_skb_wmem() is only possibly true for full sockets by definition.
So the (sk_fullsock(sk) && is_skb_wmem(skb)) can be reduced to is_skb_wmem(skb)
RFC because it have an extra changes:
new is_skb_wmem() helper can be called
- either before pskb_expand_head(), to create skb clones
for skb with destructors that does not change sk->sk_wmem_alloc
- or after pskb_expand_head(), to change owner in skb_set_owner_w()
In current patch I've added both these ways,
we need to keep one of them.
- return skb;
+ if (oskb) {
+ if (sk)
+ skb_set_owner_w(skb, sk);
Broken for non full sockets.
Calling skb_set_owner_w(skb, sk) for them is a bug.
I think you're wrong here.
It is 100% equivalent of old code,
skb_set_owner_w() handles sk_fullsock(sk) inside and does not adjust sk->sk_wmem_alloc.
Please explain if I'm wrong.
quoted
+ consume_skb(oskb);
+ } else if (sk) {
+ delta = osize - skb_end_offset(skb);
+ if (!is_skb_wmem(skb))
+ skb_set_owner_w(skb, sk);
This would be broken for non full sockets.
Calling skb_set_owner_w(skb, sk) for them is a bug.
See my comment above.
quoted
+ skb->truesize += delta;
+ if (sk_fullsock(sk))
+ refcount_add(delta, &sk->sk_wmem_alloc);
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 <redacted>
Signed-off-by: Vasily Averin <redacted>
---
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(-)
@@ -1804,28 +1804,45 @@ struct sk_buff *skb_realloc_headroom(struct sk_buff *skb, unsigned int headroom)structsk_buff*skb_expand_head(structsk_buff*skb,unsignedintheadroom){intdelta=headroom-skb_headroom(skb);+intosize=skb_end_offset(skb);+structsk_buff*oskb=NULL;+structsock*sk=skb->sk;if(WARN_ONCE(delta<=0,"%s is expecting an increase in the headroom",__func__))returnskb;-/* pskb_expand_head() might crash, if skb is shared */+delta=SKB_DATA_ALIGN(delta);+/* pskb_expand_head() might crash, if skb is shared.+*Alsoweshouldcloneskbifitsdestructordoes+*notadjustskb->truesizeandsk->sk_wmem_alloc+*/if(skb_shared(skb)){structsk_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);+returnNULL;}+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);+returnNULL;+}+if(oskb){+if(sk)+skb_set_owner_w(skb,sk);+consume_skb(oskb);+}elseif(sk){+delta=osize-skb_end_offset(skb);+if(!is_skb_wmem(skb))+skb_set_owner_w(skb,sk);+skb->truesize+=delta;+if(sk_fullsock(sk))+refcount_add(delta,&sk->sk_wmem_alloc);}returnskb;}
From: Christoph Paasch <hidden> Date: 2021-09-01 16:58:45
Hello,
On Wed, Sep 1, 2021 at 1:12 AM Vasily Averin [off-list ref] 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 <redacted>
Signed-off-by: Vasily Averin <redacted>
---
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(-)
@@ -1804,28 +1804,45 @@ struct sk_buff *skb_realloc_headroom(struct sk_buff *skb, unsigned int headroom)structsk_buff*skb_expand_head(structsk_buff*skb,unsignedintheadroom){intdelta=headroom-skb_headroom(skb);+intosize=skb_end_offset(skb);+structsk_buff*oskb=NULL;+structsock*sk=skb->sk;if(WARN_ONCE(delta<=0,"%s is expecting an increase in the headroom",__func__))returnskb;-/* pskb_expand_head() might crash, if skb is shared */+delta=SKB_DATA_ALIGN(delta);+/* pskb_expand_head() might crash, if skb is shared.+*Alsoweshouldcloneskbifitsdestructordoes+*notadjustskb->truesizeandsk->sk_wmem_alloc+*/if(skb_shared(skb)){structsk_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);+returnNULL;}+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);+returnNULL;+}+if(oskb){+if(sk)+skb_set_owner_w(skb,sk);+consume_skb(oskb);+}elseif(sk){+delta=osize-skb_end_offset(skb);+if(!is_skb_wmem(skb))+skb_set_owner_w(skb,sk);+skb->truesize+=delta;+if(sk_fullsock(sk))+refcount_add(delta,&sk->sk_wmem_alloc);}returnskb;}
From: Eric Dumazet <hidden> Date: 2021-09-01 19:17:36
On 9/1/21 1:11 AM, Vasily Averin wrote:
quoted hunk
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 <redacted>
Signed-off-by: Vasily Averin <redacted>
---
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(-)
@@ -1804,28 +1804,45 @@ struct sk_buff *skb_realloc_headroom(struct sk_buff *skb, unsigned int headroom)structsk_buff*skb_expand_head(structsk_buff*skb,unsignedintheadroom){intdelta=headroom-skb_headroom(skb);+intosize=skb_end_offset(skb);+structsk_buff*oskb=NULL;+structsock*sk=skb->sk;if(WARN_ONCE(delta<=0,"%s is expecting an increase in the headroom",__func__))returnskb;-/* pskb_expand_head() might crash, if skb is shared */+delta=SKB_DATA_ALIGN(delta);+/* pskb_expand_head() might crash, if skb is shared.+*Alsoweshouldcloneskbifitsdestructordoes+*notadjustskb->truesizeandsk->sk_wmem_alloc+*/if(skb_shared(skb)){structsk_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);+returnNULL;}+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);+returnNULL;+}+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.
&& (skb->destructor != sock_edemux)
(Because in this case , pskb_expand_head() already adjusted skb->truesize)
+ 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.
(skb->truesize can be adjusted without issue)
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 <redacted>
Signed-off-by: Vasily Averin <redacted>
---
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(-)
@@ -1804,28 +1804,45 @@ struct sk_buff *skb_realloc_headroom(struct sk_buff *skb, unsigned int headroom)structsk_buff*skb_expand_head(structsk_buff*skb,unsignedintheadroom){intdelta=headroom-skb_headroom(skb);+intosize=skb_end_offset(skb);+structsk_buff*oskb=NULL;+structsock*sk=skb->sk;if(WARN_ONCE(delta<=0,"%s is expecting an increase in the headroom",__func__))returnskb;-/* pskb_expand_head() might crash, if skb is shared */+delta=SKB_DATA_ALIGN(delta);+/* pskb_expand_head() might crash, if skb is shared.+*Alsoweshouldcloneskbifitsdestructordoes+*notadjustskb->truesizeandsk->sk_wmem_alloc+*/if(skb_shared(skb)){structsk_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);+returnNULL;}+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);+returnNULL;+}+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->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.
quoted
+ delta = osize - skb_end_offset(skb);
quoted
+ 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
From: Eric Dumazet <hidden> Date: 2021-09-02 04:40:45
On 9/1/21 8:59 PM, Vasily Averin wrote:
On 9/1/21 10:17 PM, Eric Dumazet wrote:
quoted
On 9/1/21 1:11 AM, Vasily Averin wrote:
quoted
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 <redacted>
Signed-off-by: Vasily Averin <redacted>
---
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(-)
@@ -1804,28 +1804,45 @@ struct sk_buff *skb_realloc_headroom(struct sk_buff *skb, unsigned int headroom)structsk_buff*skb_expand_head(structsk_buff*skb,unsignedintheadroom){intdelta=headroom-skb_headroom(skb);+intosize=skb_end_offset(skb);+structsk_buff*oskb=NULL;+structsock*sk=skb->sk;if(WARN_ONCE(delta<=0,"%s is expecting an increase in the headroom",__func__))returnskb;-/* pskb_expand_head() might crash, if skb is shared */+delta=SKB_DATA_ALIGN(delta);+/* pskb_expand_head() might crash, if skb is shared.+*Alsoweshouldcloneskbifitsdestructordoes+*notadjustskb->truesizeandsk->sk_wmem_alloc+*/if(skb_shared(skb)){structsk_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);+returnNULL;}+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);+returnNULL;+}+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,
We can not adjust sk_wmem_alloc if this is already 0
The only way you can guarantee this is :
to look at is_skb_wmem(oskb)
Because then you are certain that _at_ least this skb owns a reference on sk->sk_wmem_alloc
If another kind of destructor is held by oskb, then you can not assume this.
Otherwise we need a new refcount_add_if_not_zero() function, and make skb_set_owner_w()
more expensive for a very corner case.
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->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.
quoted
quoted
+ delta = osize - skb_end_offset(skb);
quoted
+ 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.
I think you missed netem case, in particular
skb_orphan_partial() which I already pointed out.
You can setup a stack of virtual devices (tunnels),
with a qdisc on them, before ip6_xmit() is finally called...
Socket might have been closed already.
To test your patch, you could force a skb_orphan_partial() at the beginning
of skb_expand_head() (extending code coverage)
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.
You can not change sk_wmem_alloc if this is already 0.
refcount_add() will trigger a warning (panic under KASAN)
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
From: Eric Dumazet <hidden> Date: 2021-09-02 04:48:36
On 9/1/21 9:32 PM, Eric Dumazet wrote:
I think you missed netem case, in particular
skb_orphan_partial() which I already pointed out.
You can setup a stack of virtual devices (tunnels),
with a qdisc on them, before ip6_xmit() is finally called...
Socket might have been closed already.
To test your patch, you could force a skb_orphan_partial() at the beginning
of skb_expand_head() (extending code coverage)
To clarify :
It is ok to 'downgrade' an skb->destructor having a ref on sk->sk_wmem_alloc to
something owning a ref on sk->refcnt.
But the opposite operation (ref on sk->sk_refcnt --> ref on sk->sk_wmem_alloc) is not safe.
I think you missed netem case, in particular
skb_orphan_partial() which I already pointed out.
You can setup a stack of virtual devices (tunnels),
with a qdisc on them, before ip6_xmit() is finally called...
Socket might have been closed already.
To test your patch, you could force a skb_orphan_partial() at the beginning
of skb_expand_head() (extending code coverage)
To clarify :
It is ok to 'downgrade' an skb->destructor having a ref on sk->sk_wmem_alloc to
something owning a ref on sk->refcnt.
But the opposite operation (ref on sk->sk_refcnt --> ref on sk->sk_wmem_alloc) is not safe.
Could you please explain in more details, since I stil have a completely opposite point of view?
Every sk referenced in skb have sk_wmem_alloc > 9
It is assigned to 1 in sk_alloc and decremented right before last __sk_free(),
inside both sk_free() sock_wfree() and __sock_wfree()
So it is safe to adjust skb->sk->sk_wmem_alloc,
because alive skb keeps reference to alive sk and last one keeps sk_wmem_alloc > 0
So any destructor used sk->sk_refcnt will already have sk_wmem_alloc > 0,
because last sock_put() calls sk_free().
However now I'm not sure in reversed direction.
skb_set_owner_w() check !sk_fullsock(sk) and call sock_hold(sk);
If sk->sk_refcnt can be 0 here (i.e. after execution of old destructor inside skb_orphan)
-- it can be trigger pointed problem:
"refcount_add() will trigger a warning (panic under KASAN)".
Could you please explain where I'm wrong?
Thank you,
Vasily Averin
I think you missed netem case, in particular
skb_orphan_partial() which I already pointed out.
You can setup a stack of virtual devices (tunnels),
with a qdisc on them, before ip6_xmit() is finally called...
Socket might have been closed already.
To test your patch, you could force a skb_orphan_partial() at the beginning
of skb_expand_head() (extending code coverage)
To clarify :
It is ok to 'downgrade' an skb->destructor having a ref on sk->sk_wmem_alloc to
something owning a ref on sk->refcnt.
But the opposite operation (ref on sk->sk_refcnt --> ref on sk->sk_wmem_alloc) is not safe.
Could you please explain in more details, since I stil have a completely opposite point of view?
Every sk referenced in skb have sk_wmem_alloc > 9
It is assigned to 1 in sk_alloc and decremented right before last __sk_free(),
inside both sk_free() sock_wfree() and __sock_wfree()
So it is safe to adjust skb->sk->sk_wmem_alloc,
because alive skb keeps reference to alive sk and last one keeps sk_wmem_alloc > 0
So any destructor used sk->sk_refcnt will already have sk_wmem_alloc > 0,
because last sock_put() calls sk_free().
However now I'm not sure in reversed direction.
skb_set_owner_w() check !sk_fullsock(sk) and call sock_hold(sk);
If sk->sk_refcnt can be 0 here (i.e. after execution of old destructor inside skb_orphan)
-- it can be trigger pointed problem:
"refcount_add() will trigger a warning (panic under KASAN)".
Could you please explain where I'm wrong?
To clarify:
I'm agree it is unsafe to call on alive skb:
skb_orphan(skb)
adjust(skb_>sk->sk_wmem_alloc)
becasue 2 reasone:
1) old destructor can decrease sk_vmem_alloc to zero and free sk
2) becasue old destructor if !sk_fullsock(sk) can call sock_out and release last sk->sk_refcnt reference.
in this case sock_hold() will trigger warning.
1) can be handled, we can adjust(sk_wmem_alloc) before skb_orphan()
but I badly understand how to handle 2nd case.
Thank you,
Vasily Averin
I think you missed netem case, in particular
skb_orphan_partial() which I already pointed out.
You can setup a stack of virtual devices (tunnels),
with a qdisc on them, before ip6_xmit() is finally called...
Socket might have been closed already.
To test your patch, you could force a skb_orphan_partial() at the beginning
of skb_expand_head() (extending code coverage)
To clarify :
It is ok to 'downgrade' an skb->destructor having a ref on sk->sk_wmem_alloc to
something owning a ref on sk->refcnt.
But the opposite operation (ref on sk->sk_refcnt --> ref on sk->sk_wmem_alloc) is not safe.
Could you please explain in more details, since I stil have a completely opposite point of view?
Every sk referenced in skb have sk_wmem_alloc > 9
It is assigned to 1 in sk_alloc and decremented right before last __sk_free(),
inside both sk_free() sock_wfree() and __sock_wfree()
So it is safe to adjust skb->sk->sk_wmem_alloc,
because alive skb keeps reference to alive sk and last one keeps sk_wmem_alloc > 0
So any destructor used sk->sk_refcnt will already have sk_wmem_alloc > 0,
because last sock_put() calls sk_free().
However now I'm not sure in reversed direction.
skb_set_owner_w() check !sk_fullsock(sk) and call sock_hold(sk);
If sk->sk_refcnt can be 0 here (i.e. after execution of old destructor inside skb_orphan)
-- it can be trigger pointed problem:
"refcount_add() will trigger a warning (panic under KASAN)".
Could you please explain where I'm wrong?
To clarify:
I'm agree it is unsafe to call on alive skb:
I badly explained the problem in previous letter, let me repeat once again:
I'm told about this piece of code:
+ } else if (sk && skb->destructor != sock_edemux) {
+ delta = osize - skb_end_offset(skb);
+ if (!is_skb_wmem(skb))
+ skb_set_owner_w(skb, sk);
+ skb->truesize += delta;
+ if (sk_fullsock(sk))
+ refcount_add(delta, &sk->sk_wmem_alloc);
}
it is called on alive expanded skb and it is incorrect because 2 reasons:
a) if old destructor use ref on sk->sk_wmem_alloc
It can decrease to 0 and release sk.
b) if old descriptor use ref on sk->refcnt and !sk_fullsock(sk)
old decriptor can release last reference and release sk.
We can workaround release of sk by move of
refcount_add(delta, &sk->sk_wmem_alloc) before skb_set_owner_w()
} else if (sk && skb->destructor != sock_edemux) {
delta = osize - skb_end_offset(skb);
refcount_add(delta, &sk->sk_wmem_alloc);
if (!is_skb_wmem(skb))
skb_set_owner_w(skb, sk);
skb->truesize += delta;
#ifdef CONFIG_INET
if (!sk_fullsock(sk))
refcount_dec(delta, &sk->sk_wmem_alloc);
#endif
}
However it it does not resolve b) completely
oid skb_set_owner_w(struct sk_buff *skb, struct sock *sk)
{
skb_orphan(skb); <<< old destructor releases last sk->refcnt ...
skb->sk = sk;
...
if (unlikely(!sk_fullsock(sk))) {
skb->destructor = sock_edemux;
sock_hold(sk); <<<< ...and it trigger wrining/panic
return;
}
Thank you,
Vasily Averin
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 <redacted>
Signed-off-by: Vasily Averin <redacted>
---
v5: fixed else condition, thanks to Eric
reworked update of expanded skb,
added corresponding comments
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 | 63 ++++++++++++++++++++++++++++++++++++++++++++++--------
net/core/sock.c | 8 +++++++
3 files changed, 63 insertions(+), 9 deletions(-)
@@ -1804,28 +1804,73 @@ struct sk_buff *skb_realloc_headroom(struct sk_buff *skb, unsigned int headroom)structsk_buff*skb_expand_head(structsk_buff*skb,unsignedintheadroom){intdelta=headroom-skb_headroom(skb);+intosize=skb_end_offset(skb);+structsk_buff*oskb=NULL;+structsock*sk=skb->sk;if(WARN_ONCE(delta<=0,"%s is expecting an increase in the headroom",__func__))returnskb;-/* pskb_expand_head() might crash, if skb is shared */+delta=SKB_DATA_ALIGN(delta);+/* pskb_expand_head() might crash, if skb is shared.+*Alsoweshouldcloneskbifitsdestructordoes+*notadjustskb->truesizeandsk->sk_wmem_alloc+*/if(skb_shared(skb)){structsk_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);+returnNULL;}+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);+returnNULL;+}+if(oskb){+if(sk)+skb_set_owner_w(skb,sk);+consume_skb(oskb);+}elseif(sk&&skb->destructor!=sock_edemux){+boolref,set_owner;++ref=false;set_owner=false;+delta=osize-skb_end_offset(skb);+/* skb_set_owner_w() calls current skb destructor.+*Itcandecreasesk_wmem_allocto0andreleasesk,+*Toprevntitweincreasesk_wmem_allocearlier.+*Anotherkindofdestructorscanreleaselastsk_refcnt,+*soitwillbeimpossibletocallsock_holdfor!fullsock+*Takeextrask_refcnttopreventit.+*Otherwisejustincreasetruesizeofexpandedskb.+*/+refcount_add(delta,&sk->sk_wmem_alloc);+if(!is_skb_wmem(skb)){+set_owner=true;+if(!sk_fullsock(sk)&&IS_ENABLED(CONFIG_INET)){+/* skb_set_owner_w can set sock_edemux */+ref=refcount_inc_not_zero(&sk->sk_refcnt);+if(!ref){+set_owner=false;+WARN_ON(refcount_sub_and_test(delta,&sk->sk_wmem_alloc));+}+}+}+if(set_owner)+skb_set_owner_w(skb,sk);+#ifdef CONFIG_INET+if(skb->destructor==sock_edemux){+WARN_ON(refcount_sub_and_test(delta,&sk->sk_wmem_alloc));+if(ref)+WARN_ON(refcount_dec_and_test(&sk->sk_refcnt));+}+#endif+skb->truesize+=delta;}returnskb;}
From: Christoph Paasch <hidden> Date: 2021-09-02 15:53:54
On Thu, Sep 2, 2021 at 4:12 AM Vasily Averin [off-list ref] 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 <redacted>
Signed-off-by: Vasily Averin <redacted>
---
v5: fixed else condition, thanks to Eric
reworked update of expanded skb,
added corresponding comments
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 | 63 ++++++++++++++++++++++++++++++++++++++++++++++--------
net/core/sock.c | 8 +++++++
3 files changed, 63 insertions(+), 9 deletions(-)
Still the same issues around refcount as I reported in my other email.
Did you try running the syzkaller reproducer on your setup?
Christoph
@@ -1804,28 +1804,73 @@ struct sk_buff *skb_realloc_headroom(struct sk_buff *skb, unsigned int headroom)structsk_buff*skb_expand_head(structsk_buff*skb,unsignedintheadroom){intdelta=headroom-skb_headroom(skb);+intosize=skb_end_offset(skb);+structsk_buff*oskb=NULL;+structsock*sk=skb->sk;if(WARN_ONCE(delta<=0,"%s is expecting an increase in the headroom",__func__))returnskb;-/* pskb_expand_head() might crash, if skb is shared */+delta=SKB_DATA_ALIGN(delta);+/* pskb_expand_head() might crash, if skb is shared.+*Alsoweshouldcloneskbifitsdestructordoes+*notadjustskb->truesizeandsk->sk_wmem_alloc+*/if(skb_shared(skb)){structsk_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);+returnNULL;}+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);+returnNULL;+}+if(oskb){+if(sk)+skb_set_owner_w(skb,sk);+consume_skb(oskb);+}elseif(sk&&skb->destructor!=sock_edemux){+boolref,set_owner;++ref=false;set_owner=false;+delta=osize-skb_end_offset(skb);+/* skb_set_owner_w() calls current skb destructor.+*Itcandecreasesk_wmem_allocto0andreleasesk,+*Toprevntitweincreasesk_wmem_allocearlier.+*Anotherkindofdestructorscanreleaselastsk_refcnt,+*soitwillbeimpossibletocallsock_holdfor!fullsock+*Takeextrask_refcnttopreventit.+*Otherwisejustincreasetruesizeofexpandedskb.+*/+refcount_add(delta,&sk->sk_wmem_alloc);+if(!is_skb_wmem(skb)){+set_owner=true;+if(!sk_fullsock(sk)&&IS_ENABLED(CONFIG_INET)){+/* skb_set_owner_w can set sock_edemux */+ref=refcount_inc_not_zero(&sk->sk_refcnt);+if(!ref){+set_owner=false;+WARN_ON(refcount_sub_and_test(delta,&sk->sk_wmem_alloc));+}+}+}+if(set_owner)+skb_set_owner_w(skb,sk);+#ifdef CONFIG_INET+if(skb->destructor==sock_edemux){+WARN_ON(refcount_sub_and_test(delta,&sk->sk_wmem_alloc));+if(ref)+WARN_ON(refcount_dec_and_test(&sk->sk_refcnt));+}+#endif+skb->truesize+=delta;}returnskb;}
On Thu, Sep 2, 2021 at 4:12 AM Vasily Averin [off-list ref] wrote:
quoted
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 <redacted>
Signed-off-by: Vasily Averin <redacted>
---
v5: fixed else condition, thanks to Eric
reworked update of expanded skb,
added corresponding comments
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 | 63 ++++++++++++++++++++++++++++++++++++++++++++++--------
net/core/sock.c | 8 +++++++
3 files changed, 63 insertions(+), 9 deletions(-)
Still the same issues around refcount as I reported in my other email.
Did you try running the syzkaller reproducer on your setup?
error is here, should be instead
delta = skb_end_offset(skb) - osize;
quoted
+ /* skb_set_owner_w() calls current skb destructor.
+ * It can decrease sk_wmem_alloc to 0 and release sk,
+ * To prevnt it we increase sk_wmem_alloc earlier.
+ * Another kind of destructors can release last sk_refcnt,
+ * so it will be impossible to call sock_hold for !fullsock
+ * Take extra sk_refcnt to prevent it.
+ * Otherwise just increase truesize of expanded skb.
+ */
+ refcount_add(delta, &sk->sk_wmem_alloc);
+ if (!is_skb_wmem(skb)) {
+ set_owner = true;
+ if (!sk_fullsock(sk) && IS_ENABLED(CONFIG_INET)) {
+ /* skb_set_owner_w can set sock_edemux */
+ ref = refcount_inc_not_zero(&sk->sk_refcnt);
+ if (!ref) {
+ set_owner = false;
+ WARN_ON(refcount_sub_and_test(delta, &sk->sk_wmem_alloc));
+ }
+ }
+ }
+ if (set_owner)
+ skb_set_owner_w(skb, sk);
+#ifdef CONFIG_INET
+ if (skb->destructor == sock_edemux) {
+ WARN_ON(refcount_sub_and_test(delta, &sk->sk_wmem_alloc));
+ if (ref)
+ WARN_ON(refcount_dec_and_test(&sk->sk_refcnt));
+ }
+#endif
+ skb->truesize += delta;
}
return skb;
}
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()")
Fixes: 2d85a1b31dde ("ipv6: ip6_finish_output2: set sk into newly allocated nskb")
Reported-by: Christoph Paasch <redacted>
Signed-off-by: Vasily Averin <redacted>
---
v6: fixed delta,
improved comments
v5: fixed else condition, thanks to Eric
reworked update of expanded skb,
added corresponding comments
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 | 60 ++++++++++++++++++++++++++++++++++++++++++++++--------
net/core/sock.c | 8 ++++++++
3 files changed, 60 insertions(+), 9 deletions(-)
@@ -1804,28 +1804,70 @@ struct sk_buff *skb_realloc_headroom(struct sk_buff *skb, unsigned int headroom)structsk_buff*skb_expand_head(structsk_buff*skb,unsignedintheadroom){intdelta=headroom-skb_headroom(skb);+intosize=skb_end_offset(skb);+structsk_buff*oskb=NULL;+structsock*sk=skb->sk;if(WARN_ONCE(delta<=0,"%s is expecting an increase in the headroom",__func__))returnskb;-/* pskb_expand_head() might crash, if skb is shared */+delta=SKB_DATA_ALIGN(delta);+/* pskb_expand_head() might crash, if skb is shared. */if(skb_shared(skb)){structsk_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);+returnNULL;}+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);+returnNULL;+}+if(oskb){+if(sk)+skb_set_owner_w(skb,sk);+consume_skb(oskb);+}elseif(sk&&skb->destructor!=sock_edemux){+boolref,set_owner;++ref=false;set_owner=false;+delta=skb_end_offset(skb)-osize;+/* skb_set_owner_w() calls current skb destructor.+*Itcanreducesk_wmem_allocto0andreleasesk,+*Toprevntthis,weincreasesk_wmem_allocinadvance.+*Somedestructorsmightreleasethelastsk_refcnt,+*soitwon'tbepossibletocallsock_holdfor!fullsock+*Wetakeanextrask_refcnttopreventthis.+*Inanycaseweincreasetruesizeofexpandedskb.+*/+refcount_add(delta,&sk->sk_wmem_alloc);+if(!is_skb_wmem(skb)){+set_owner=true;+if(!sk_fullsock(sk)&&IS_ENABLED(CONFIG_INET)){+/* skb_set_owner_w can set sock_edemux */+ref=refcount_inc_not_zero(&sk->sk_refcnt);+if(!ref){+set_owner=false;+WARN_ON(refcount_sub_and_test(delta,&sk->sk_wmem_alloc));+}+}+}+if(set_owner)+skb_set_owner_w(skb,sk);+#ifdef CONFIG_INET+if(skb->destructor==sock_edemux){+WARN_ON(refcount_sub_and_test(delta,&sk->sk_wmem_alloc));+if(ref)+WARN_ON(refcount_dec_and_test(&sk->sk_refcnt));+}+#endif+skb->truesize+=delta;}returnskb;}
I've finally reproduced original issue by using reproducer from Christoph Paasch,
and was able locally validate this patch.
Thank you,
Vasily Averin
On 9/6/21 9:01 PM, Vasily Averin wrote:
quoted hunk
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()")
Fixes: 2d85a1b31dde ("ipv6: ip6_finish_output2: set sk into newly allocated nskb")
Reported-by: Christoph Paasch <redacted>
Signed-off-by: Vasily Averin <redacted>
---
v6: fixed delta,
improved comments
v5: fixed else condition, thanks to Eric
reworked update of expanded skb,
added corresponding comments
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 | 60 ++++++++++++++++++++++++++++++++++++++++++++++--------
net/core/sock.c | 8 ++++++++
3 files changed, 60 insertions(+), 9 deletions(-)
@@ -1804,28 +1804,70 @@ struct sk_buff *skb_realloc_headroom(struct sk_buff *skb, unsigned int headroom)structsk_buff*skb_expand_head(structsk_buff*skb,unsignedintheadroom){intdelta=headroom-skb_headroom(skb);+intosize=skb_end_offset(skb);+structsk_buff*oskb=NULL;+structsock*sk=skb->sk;if(WARN_ONCE(delta<=0,"%s is expecting an increase in the headroom",__func__))returnskb;-/* pskb_expand_head() might crash, if skb is shared */+delta=SKB_DATA_ALIGN(delta);+/* pskb_expand_head() might crash, if skb is shared. */if(skb_shared(skb)){structsk_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);+returnNULL;}+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);+returnNULL;+}+if(oskb){+if(sk)+skb_set_owner_w(skb,sk);+consume_skb(oskb);+}elseif(sk&&skb->destructor!=sock_edemux){+boolref,set_owner;++ref=false;set_owner=false;+delta=skb_end_offset(skb)-osize;+/* skb_set_owner_w() calls current skb destructor.+*Itcanreducesk_wmem_allocto0andreleasesk,+*Toprevntthis,weincreasesk_wmem_allocinadvance.+*Somedestructorsmightreleasethelastsk_refcnt,+*soitwon'tbepossibletocallsock_holdfor!fullsock+*Wetakeanextrask_refcnttopreventthis.+*Inanycaseweincreasetruesizeofexpandedskb.+*/+refcount_add(delta,&sk->sk_wmem_alloc);+if(!is_skb_wmem(skb)){+set_owner=true;+if(!sk_fullsock(sk)&&IS_ENABLED(CONFIG_INET)){+/* skb_set_owner_w can set sock_edemux */+ref=refcount_inc_not_zero(&sk->sk_refcnt);+if(!ref){+set_owner=false;+WARN_ON(refcount_sub_and_test(delta,&sk->sk_wmem_alloc));+}+}+}+if(set_owner)+skb_set_owner_w(skb,sk);+#ifdef CONFIG_INET+if(skb->destructor==sock_edemux){+WARN_ON(refcount_sub_and_test(delta,&sk->sk_wmem_alloc));+if(ref)+WARN_ON(refcount_dec_and_test(&sk->sk_refcnt));+}+#endif+skb->truesize+=delta;}returnskb;}