* [PATCH] net: linearizing skb when downgrade gso_size
2024-07-06 14:26 ` Willem de Bruijn
@ 2024-07-08 14:31 ` Fred Li
2024-07-09 15:53 ` Willem de Bruijn
2024-07-17 5:35 ` [PATCH v3] " Fred Li
` (2 subsequent siblings)
3 siblings, 1 reply; 21+ messages in thread
From: Fred Li @ 2024-07-08 14:31 UTC (permalink / raw)
To: willemdebruijn.kernel
Cc: aleksander.lobakin, andrii, ast, bpf, daniel, davem, dracodingfly,
edumazet, haoluo, hawk, herbert, john.fastabend, jolsa, kpsingh,
kuba, linux-kernel, linux, martin.lau, mkhalfella, nbd, netdev,
pabeni, sashal, sdf, song, yonghong.song
Here is a patch that linearizing skb when downgrade
gso_size and sg should disabled, If there are no issues,
I will submit a formal patch shortly.
Signed-off-by: Fred Li <dracodingfly@gmail.com>
---
include/linux/skbuff.h | 22 ++++++++++++++++++++++
net/core/filter.c | 16 ++++++++++++----
net/core/skbuff.c | 19 ++-----------------
3 files changed, 36 insertions(+), 21 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 5f11f9873341..99b7fc1e826a 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2400,6 +2400,28 @@ static inline unsigned int skb_headlen(const struct sk_buff *skb)
return skb->len - skb->data_len;
}
+static inline bool skb_is_nonsg(const struct sk_buff *skb)
+{
+ struct sk_buff *list_skb = skb_shinfo(skb)->frag_list;
+ struct sk_buff *check_skb;
+ for (check_skb = list_skb; check_skb; check_skb = check_skb->next) {
+ if (skb_headlen(check_skb) && !check_skb->head_frag) {
+ /* gso_size is untrusted, and we have a frag_list with
+ * a linear non head_frag item.
+ *
+ * If head_skb's headlen does not fit requested gso_size,
+ * it means that the frag_list members do NOT terminate
+ * on exact gso_size boundaries. Hence we cannot perform
+ * skb_frag_t page sharing. Therefore we must fallback to
+ * copying the frag_list skbs; we do so by disabling SG.
+ */
+ return true;
+ }
+ }
+
+ return false;
+}
+
static inline unsigned int __skb_pagelen(const struct sk_buff *skb)
{
unsigned int i, len = 0;
diff --git a/net/core/filter.c b/net/core/filter.c
index df4578219e82..c0e6e7f28635 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3525,13 +3525,21 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff,
if (skb_is_gso(skb)) {
struct skb_shared_info *shinfo = skb_shinfo(skb);
- /* Due to header grow, MSS needs to be downgraded. */
- if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO))
- skb_decrease_gso_size(shinfo, len_diff);
-
/* Header must be checked, and gso_segs recomputed. */
shinfo->gso_type |= gso_type;
shinfo->gso_segs = 0;
+
+ /* Due to header grow, MSS needs to be downgraded.
+ * There is BUG_ON When segment the frag_list with
+ * head_frag true so linearize skb after downgrade
+ * the MSS.
+ */
+ if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) {
+ skb_decrease_gso_size(shinfo, len_diff);
+ if (skb_is_nonsg(skb))
+ return skb_linearize(skb) ? : 0;
+ }
+
}
return 0;
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index b1dab1b071fc..81e018185527 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4458,23 +4458,8 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
if ((skb_shinfo(head_skb)->gso_type & SKB_GSO_DODGY) &&
mss != GSO_BY_FRAGS && mss != skb_headlen(head_skb)) {
- struct sk_buff *check_skb;
-
- for (check_skb = list_skb; check_skb; check_skb = check_skb->next) {
- if (skb_headlen(check_skb) && !check_skb->head_frag) {
- /* gso_size is untrusted, and we have a frag_list with
- * a linear non head_frag item.
- *
- * If head_skb's headlen does not fit requested gso_size,
- * it means that the frag_list members do NOT terminate
- * on exact gso_size boundaries. Hence we cannot perform
- * skb_frag_t page sharing. Therefore we must fallback to
- * copying the frag_list skbs; we do so by disabling SG.
- */
- features &= ~NETIF_F_SG;
- break;
- }
- }
+ if (skb_is_nonsg(head_skb))
+ features &= ~NETIF_F_SG;
}
__skb_push(head_skb, doffset);
--
2.33.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH] net: linearizing skb when downgrade gso_size
2024-07-08 14:31 ` [PATCH] net: linearizing skb when downgrade gso_size Fred Li
@ 2024-07-09 15:53 ` Willem de Bruijn
2024-07-09 20:16 ` Herbert Xu
2024-07-12 8:17 ` Fred Li
0 siblings, 2 replies; 21+ messages in thread
From: Willem de Bruijn @ 2024-07-09 15:53 UTC (permalink / raw)
To: Fred Li, willemdebruijn.kernel
Cc: aleksander.lobakin, andrii, ast, bpf, daniel, davem, dracodingfly,
edumazet, haoluo, hawk, herbert, john.fastabend, jolsa, kpsingh,
kuba, linux-kernel, linux, martin.lau, mkhalfella, nbd, netdev,
pabeni, sashal, sdf, song, yonghong.song
Fred Li wrote:
> Here is a patch that linearizing skb when downgrade
> gso_size and sg should disabled, If there are no issues,
> I will submit a formal patch shortly.
Target bpf.
Probably does not need quite as many direct CCs.
> Signed-off-by: Fred Li <dracodingfly@gmail.com>
> ---
> include/linux/skbuff.h | 22 ++++++++++++++++++++++
> net/core/filter.c | 16 ++++++++++++----
> net/core/skbuff.c | 19 ++-----------------
> 3 files changed, 36 insertions(+), 21 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 5f11f9873341..99b7fc1e826a 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -2400,6 +2400,28 @@ static inline unsigned int skb_headlen(const struct sk_buff *skb)
> return skb->len - skb->data_len;
> }
>
> +static inline bool skb_is_nonsg(const struct sk_buff *skb)
> +{
is_nonsg does not cover the functionality, which is fairly subtle.
But maybe we don't need this function at all, see below..
> + struct sk_buff *list_skb = skb_shinfo(skb)->frag_list;
> + struct sk_buff *check_skb;
No need for separate check_skb
> + for (check_skb = list_skb; check_skb; check_skb = check_skb->next) {
> + if (skb_headlen(check_skb) && !check_skb->head_frag) {
> + /* gso_size is untrusted, and we have a frag_list with
> + * a linear non head_frag item.
> + *
> + * If head_skb's headlen does not fit requested gso_size,
> + * it means that the frag_list members do NOT terminate
> + * on exact gso_size boundaries. Hence we cannot perform
> + * skb_frag_t page sharing. Therefore we must fallback to
> + * copying the frag_list skbs; we do so by disabling SG.
> + */
> + return true;
> + }
> + }
> +
> + return false;
> +}
> +
> static inline unsigned int __skb_pagelen(const struct sk_buff *skb)
> {
> unsigned int i, len = 0;
> diff --git a/net/core/filter.c b/net/core/filter.c
> index df4578219e82..c0e6e7f28635 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3525,13 +3525,21 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff,
> if (skb_is_gso(skb)) {
> struct skb_shared_info *shinfo = skb_shinfo(skb);
>
> - /* Due to header grow, MSS needs to be downgraded. */
> - if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO))
> - skb_decrease_gso_size(shinfo, len_diff);
> -
> /* Header must be checked, and gso_segs recomputed. */
> shinfo->gso_type |= gso_type;
> shinfo->gso_segs = 0;
> +
> + /* Due to header grow, MSS needs to be downgraded.
> + * There is BUG_ON When segment the frag_list with
> + * head_frag true so linearize skb after downgrade
> + * the MSS.
> + */
> + if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) {
> + skb_decrease_gso_size(shinfo, len_diff);
> + if (skb_is_nonsg(skb))
> + return skb_linearize(skb) ? : 0;
> + }
> +
No need for ternary statement.
Instead of the complex test in skb_is_nonsg, can we just assume that
alignment will be off if having frag_list and changing gso_size.
The same will apply to bpf_skb_net_shrink too.
Not sure that it is okay to linearize inside a BPF helper function.
Hopefully bpf experts can chime in on that.
> }
>
> return 0;
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index b1dab1b071fc..81e018185527 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4458,23 +4458,8 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>
> if ((skb_shinfo(head_skb)->gso_type & SKB_GSO_DODGY) &&
> mss != GSO_BY_FRAGS && mss != skb_headlen(head_skb)) {
> - struct sk_buff *check_skb;
> -
> - for (check_skb = list_skb; check_skb; check_skb = check_skb->next) {
> - if (skb_headlen(check_skb) && !check_skb->head_frag) {
> - /* gso_size is untrusted, and we have a frag_list with
> - * a linear non head_frag item.
> - *
> - * If head_skb's headlen does not fit requested gso_size,
> - * it means that the frag_list members do NOT terminate
> - * on exact gso_size boundaries. Hence we cannot perform
> - * skb_frag_t page sharing. Therefore we must fallback to
> - * copying the frag_list skbs; we do so by disabling SG.
> - */
> - features &= ~NETIF_F_SG;
> - break;
> - }
> - }
> + if (skb_is_nonsg(head_skb))
> + features &= ~NETIF_F_SG;
> }
>
> __skb_push(head_skb, doffset);
> --
> 2.33.0
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH] net: linearizing skb when downgrade gso_size
2024-07-09 15:53 ` Willem de Bruijn
@ 2024-07-09 20:16 ` Herbert Xu
2024-07-09 21:29 ` Willem de Bruijn
2024-07-12 8:17 ` Fred Li
1 sibling, 1 reply; 21+ messages in thread
From: Herbert Xu @ 2024-07-09 20:16 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Fred Li, aleksander.lobakin, andrii, ast, bpf, daniel, davem,
edumazet, haoluo, hawk, john.fastabend, jolsa, kpsingh, kuba,
linux-kernel, linux, martin.lau, mkhalfella, nbd, netdev, pabeni,
sashal, sdf, song, yonghong.song
On Tue, Jul 09, 2024 at 11:53:21AM -0400, Willem de Bruijn wrote:
>
> > + /* Due to header grow, MSS needs to be downgraded.
> > + * There is BUG_ON When segment the frag_list with
> > + * head_frag true so linearize skb after downgrade
> > + * the MSS.
> > + */
This sounds completely wrong. You should never grow the TCP header
by changing gso_size. What is the usage-scenario for this?
Think about it, if a router forwards a TCP packet, and ends up
growing its TCP header and then splits the packet into two, then
this router is brain-dead.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] net: linearizing skb when downgrade gso_size
2024-07-09 20:16 ` Herbert Xu
@ 2024-07-09 21:29 ` Willem de Bruijn
2024-07-10 23:06 ` Herbert Xu
0 siblings, 1 reply; 21+ messages in thread
From: Willem de Bruijn @ 2024-07-09 21:29 UTC (permalink / raw)
To: Herbert Xu, Willem de Bruijn
Cc: Fred Li, aleksander.lobakin, andrii, ast, bpf, daniel, davem,
edumazet, haoluo, hawk, john.fastabend, jolsa, kpsingh, kuba,
linux-kernel, linux, martin.lau, mkhalfella, nbd, netdev, pabeni,
sashal, sdf, song, yonghong.song
Herbert Xu wrote:
> On Tue, Jul 09, 2024 at 11:53:21AM -0400, Willem de Bruijn wrote:
> >
> > > + /* Due to header grow, MSS needs to be downgraded.
> > > + * There is BUG_ON When segment the frag_list with
> > > + * head_frag true so linearize skb after downgrade
> > > + * the MSS.
> > > + */
>
> This sounds completely wrong. You should never grow the TCP header
> by changing gso_size. What is the usage-scenario for this?
>
> Think about it, if a router forwards a TCP packet, and ends up
> growing its TCP header and then splits the packet into two, then
> this router is brain-dead.
This is an unfortunate feature, but already exists.
It decreases gso_size to account for tunnel headers.
For USO, we added BPF_F_ADJ_ROOM_FIXED_GSO to avoid this in better,
newer users.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] net: linearizing skb when downgrade gso_size
2024-07-09 21:29 ` Willem de Bruijn
@ 2024-07-10 23:06 ` Herbert Xu
0 siblings, 0 replies; 21+ messages in thread
From: Herbert Xu @ 2024-07-10 23:06 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Fred Li, aleksander.lobakin, andrii, ast, bpf, daniel, davem,
edumazet, haoluo, hawk, john.fastabend, jolsa, kpsingh, kuba,
linux-kernel, linux, martin.lau, mkhalfella, nbd, netdev, pabeni,
sashal, sdf, song, yonghong.song
On Tue, Jul 09, 2024 at 05:29:59PM -0400, Willem de Bruijn wrote:
>
> This is an unfortunate feature, but already exists.
>
> It decreases gso_size to account for tunnel headers.
Growing the tunnel header is totally fine. But you should not
decrease gso_size because of that. Instead the correct course
of action is to drop the packet and generate an ICMP if it no
longer fits the MTU.
A router that resegments a TCP packet at the TCP-level (not IP)
is brain-dead.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] net: linearizing skb when downgrade gso_size
2024-07-09 15:53 ` Willem de Bruijn
2024-07-09 20:16 ` Herbert Xu
@ 2024-07-12 8:17 ` Fred Li
1 sibling, 0 replies; 21+ messages in thread
From: Fred Li @ 2024-07-12 8:17 UTC (permalink / raw)
To: willemdebruijn.kernel; +Cc: bpf, herbert, linux-kernel, netdev
> No need for ternary statement.
>
> Instead of the complex test in skb_is_nonsg, can we just assume that
> alignment will be off if having frag_list and changing gso_size.
>
> The same will apply to bpf_skb_net_shrink too.
increase gso_size may be no problem and we can use BPF_F_ADJ_ROOM_FIXED_GSO
to avoid update gso_size when shrink.
>
> Not sure that it is okay to linearize inside a BPF helper function.
> Hopefully bpf experts can chime in on that.
Thanks
Fred Li
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3] net: linearizing skb when downgrade gso_size
2024-07-06 14:26 ` Willem de Bruijn
2024-07-08 14:31 ` [PATCH] net: linearizing skb when downgrade gso_size Fred Li
@ 2024-07-17 5:35 ` Fred Li
2024-07-17 16:32 ` Willem de Bruijn
2024-07-19 2:46 ` [PATCH v4] bpf: Fixed a segment issue " Fred Li
2024-07-22 3:08 ` [PATCH bpf v5] bpf: Fixed " Fred Li
3 siblings, 1 reply; 21+ messages in thread
From: Fred Li @ 2024-07-17 5:35 UTC (permalink / raw)
To: pabeni, willemdebruijn.kernel, herbert
Cc: bpf, netdev, linux-kernel, ast, daniel, andrii, song,
yonghong.song, john.fastabend, kpsingh, Fred Li
Linearizing skb when downgrade gso_size because it may
trigger the BUG_ON when segment skb as described in [1].
v3 changes:
linearize skb if having frag_list as Willem de Bruijn suggested[2].
[1] https://lore.kernel.org/all/20240626065555.35460-2-dracodingfly@gmail.com/
[2] https://lore.kernel.org/all/668d5cf1ec330_1c18c32947@willemb.c.googlers.com.notmuch/
Signed-off-by: Fred Li <dracodingfly@gmail.com>
---
net/core/filter.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index df4578219e82..70919b532d68 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3525,13 +3525,21 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff,
if (skb_is_gso(skb)) {
struct skb_shared_info *shinfo = skb_shinfo(skb);
- /* Due to header grow, MSS needs to be downgraded. */
- if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO))
- skb_decrease_gso_size(shinfo, len_diff);
-
/* Header must be checked, and gso_segs recomputed. */
shinfo->gso_type |= gso_type;
shinfo->gso_segs = 0;
+
+ /* Due to header grow, MSS needs to be downgraded.
+ * There is BUG_ON When segment the frag_list with
+ * head_frag true so linearize skb after downgrade
+ * the MSS.
+ */
+ if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) {
+ skb_decrease_gso_size(shinfo, len_diff);
+ if (shinfo->frag_list)
+ return skb_linearize(skb);
+ }
+
}
return 0;
--
2.33.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v3] net: linearizing skb when downgrade gso_size
2024-07-17 5:35 ` [PATCH v3] " Fred Li
@ 2024-07-17 16:32 ` Willem de Bruijn
2024-07-18 7:42 ` Fred Li
0 siblings, 1 reply; 21+ messages in thread
From: Willem de Bruijn @ 2024-07-17 16:32 UTC (permalink / raw)
To: Fred Li, pabeni, willemdebruijn.kernel, herbert
Cc: bpf, netdev, linux-kernel, ast, daniel, andrii, song,
yonghong.song, john.fastabend, kpsingh, Fred Li
Fred Li wrote:
> Linearizing skb when downgrade gso_size because it may
> trigger the BUG_ON when segment skb as described in [1].
>
> v3 changes:
> linearize skb if having frag_list as Willem de Bruijn suggested[2].
>
> [1] https://lore.kernel.org/all/20240626065555.35460-2-dracodingfly@gmail.com/
> [2] https://lore.kernel.org/all/668d5cf1ec330_1c18c32947@willemb.c.googlers.com.notmuch/
>
> Signed-off-by: Fred Li <dracodingfly@gmail.com>
A fix needs a Fixed tag.
This might be the original commit that introduced gso_size adjustment,
commit 6578171a7ff0c ("bpf: add bpf_skb_change_proto helper")
Unless support for frag_list came later.
> ---
> net/core/filter.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index df4578219e82..70919b532d68 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3525,13 +3525,21 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff,
> if (skb_is_gso(skb)) {
> struct skb_shared_info *shinfo = skb_shinfo(skb);
>
> - /* Due to header grow, MSS needs to be downgraded. */
> - if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO))
> - skb_decrease_gso_size(shinfo, len_diff);
> -
> /* Header must be checked, and gso_segs recomputed. */
> shinfo->gso_type |= gso_type;
> shinfo->gso_segs = 0;
> +
> + /* Due to header grow, MSS needs to be downgraded.
> + * There is BUG_ON When segment the frag_list with
> + * head_frag true so linearize skb after downgrade
> + * the MSS.
> + */
Super tiny nit: no capitalization of When in the middle of a sentence.
> + if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) {
> + skb_decrease_gso_size(shinfo, len_diff);
> + if (shinfo->frag_list)
> + return skb_linearize(skb);
I previously asked whether it was safe to call pskb_expand_head from
within a BPF external function. There are actually plenty of existing
examples of this, so this is fine.
> + }
> +
> }
>
> return 0;
> --
> 2.33.0
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3] net: linearizing skb when downgrade gso_size
2024-07-17 16:32 ` Willem de Bruijn
@ 2024-07-18 7:42 ` Fred Li
0 siblings, 0 replies; 21+ messages in thread
From: Fred Li @ 2024-07-18 7:42 UTC (permalink / raw)
To: willemdebruijn.kernel
Cc: andrii, ast, bpf, daniel, dracodingfly, herbert, john.fastabend,
kpsingh, linux-kernel, netdev, pabeni, song, yonghong.song
>
> > Linearizing skb when downgrade gso_size because it may
> > trigger the BUG_ON when segment skb as described in [1].
> >
> > v3 changes:
> > linearize skb if having frag_list as Willem de Bruijn suggested[2].
> >
> > [1] https://lore.kernel.org/all/20240626065555.35460-2-dracodingfly@gmail.com/
> > [2] https://lore.kernel.org/all/668d5cf1ec330_1c18c32947@willemb.c.googlers.com.notmuch/
> >
> > Signed-off-by: Fred Li <dracodingfly@gmail.com>
>
> A fix needs a Fixed tag.
>
> This might be the original commit that introduced gso_size adjustment,
> commit 6578171a7ff0c ("bpf: add bpf_skb_change_proto helper")
>
Yes, this is the original commit, but it's already fixed by commit
364745fbe981a (bpf: Do not change gso_size during bpf_skb_change_proto())
Another commit 2be7e212d5419 (bpf: add bpf_skb_adjust_room helper) introduced
gso_size too.
> Unless support for frag_list came later.
>
> > ---
> > net/core/filter.c | 16 ++++++++++++----
> > 1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index df4578219e82..70919b532d68 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -3525,13 +3525,21 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff,
> > if (skb_is_gso(skb)) {
> > struct skb_shared_info *shinfo = skb_shinfo(skb);
> >
> > - /* Due to header grow, MSS needs to be downgraded. */
> > - if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO))
> > - skb_decrease_gso_size(shinfo, len_diff);
> > -
> > /* Header must be checked, and gso_segs recomputed. */
> > shinfo->gso_type |= gso_type;
> > shinfo->gso_segs = 0;
> > +
> > + /* Due to header grow, MSS needs to be downgraded.
> > + * There is BUG_ON When segment the frag_list with
> > + * head_frag true so linearize skb after downgrade
> > + * the MSS.
> > + */
>
> Super tiny nit: no capitalization of When in the middle of a sentence.
Thanks, i'will fix.
>
> > + if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) {
> > + skb_decrease_gso_size(shinfo, len_diff);
> > + if (shinfo->frag_list)
> > + return skb_linearize(skb);
>
> I previously asked whether it was safe to call pskb_expand_head from
> within a BPF external function. There are actually plenty of existing
> examples of this, so this is fine.
>
> > + }
> > +
> > }
> >
> > return 0;
> > --
> > 2.33.0
> >
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v4] bpf: Fixed a segment issue when downgrade gso_size
2024-07-06 14:26 ` Willem de Bruijn
2024-07-08 14:31 ` [PATCH] net: linearizing skb when downgrade gso_size Fred Li
2024-07-17 5:35 ` [PATCH v3] " Fred Li
@ 2024-07-19 2:46 ` Fred Li
2024-07-19 18:22 ` Willem de Bruijn
2024-07-22 3:08 ` [PATCH bpf v5] bpf: Fixed " Fred Li
3 siblings, 1 reply; 21+ messages in thread
From: Fred Li @ 2024-07-19 2:46 UTC (permalink / raw)
To: willemdebruijn.kernel
Cc: andrii, ast, bpf, daniel, dracodingfly, herbert, john.fastabend,
kpsingh, linux-kernel, netdev, pabeni, song, yonghong.song
Linearizing skb when downgrade gso_size because it may
trigger the BUG_ON when segment skb as described in [1].
v4 changes:
add fixed tag.
v3 changes:
linearize skb if having frag_list as Willem de Bruijn suggested[2].
[1] https://lore.kernel.org/all/20240626065555.35460-2-dracodingfly@gmail.com/
[2] https://lore.kernel.org/all/668d5cf1ec330_1c18c32947@willemb.c.googlers.com.notmuch/
Fixes: 2be7e212d5419 ("bpf: add bpf_skb_adjust_room helper")
Signed-off-by: Fred Li <dracodingfly@gmail.com>
---
net/core/filter.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index df4578219e82..71396ecfc574 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3525,13 +3525,21 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff,
if (skb_is_gso(skb)) {
struct skb_shared_info *shinfo = skb_shinfo(skb);
- /* Due to header grow, MSS needs to be downgraded. */
- if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO))
- skb_decrease_gso_size(shinfo, len_diff);
-
/* Header must be checked, and gso_segs recomputed. */
shinfo->gso_type |= gso_type;
shinfo->gso_segs = 0;
+
+ /* Due to header grow, MSS needs to be downgraded.
+ * There is BUG_ON when segment the frag_list with
+ * head_frag true so linearize skb after downgrade
+ * the MSS.
+ */
+ if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) {
+ skb_decrease_gso_size(shinfo, len_diff);
+ if (shinfo->frag_list)
+ return skb_linearize(skb);
+ }
+
}
return 0;
--
2.33.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v4] bpf: Fixed a segment issue when downgrade gso_size
2024-07-19 2:46 ` [PATCH v4] bpf: Fixed a segment issue " Fred Li
@ 2024-07-19 18:22 ` Willem de Bruijn
0 siblings, 0 replies; 21+ messages in thread
From: Willem de Bruijn @ 2024-07-19 18:22 UTC (permalink / raw)
To: Fred Li
Cc: andrii, ast, bpf, daniel, herbert, john.fastabend, kpsingh,
linux-kernel, netdev, pabeni, song, yonghong.song
On Thu, Jul 18, 2024 at 7:47 PM Fred Li <dracodingfly@gmail.com> wrote:
>
> Linearizing skb when downgrade gso_size because it may
> trigger the BUG_ON when segment skb as described in [1].
>
> v4 changes:
> add fixed tag.
>
> v3 changes:
> linearize skb if having frag_list as Willem de Bruijn suggested[2].
>
> [1] https://lore.kernel.org/all/20240626065555.35460-2-dracodingfly@gmail.com/
> [2] https://lore.kernel.org/all/668d5cf1ec330_1c18c32947@willemb.c.googlers.com.notmuch/
>
> Fixes: 2be7e212d5419 ("bpf: add bpf_skb_adjust_room helper")
> Signed-off-by: Fred Li <dracodingfly@gmail.com>
Reviewed-by: Willem de Bruijn <willemb@google.com>
For next time:
- add target: [PATCH bpf]
- use imperative mood in commit messages: "Linearize ..."
https://www.kernel.org/doc/Documentation/bpf/bpf_devel_QA.rst
https://www.kernel.org/doc/Documentation/process/submitting-patches.rst
> ---
> net/core/filter.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index df4578219e82..71396ecfc574 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3525,13 +3525,21 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff,
> if (skb_is_gso(skb)) {
> struct skb_shared_info *shinfo = skb_shinfo(skb);
>
> - /* Due to header grow, MSS needs to be downgraded. */
> - if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO))
> - skb_decrease_gso_size(shinfo, len_diff);
> -
> /* Header must be checked, and gso_segs recomputed. */
> shinfo->gso_type |= gso_type;
> shinfo->gso_segs = 0;
> +
> + /* Due to header grow, MSS needs to be downgraded.
> + * There is BUG_ON when segment the frag_list with
> + * head_frag true so linearize skb after downgrade
> + * the MSS.
> + */
> + if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) {
> + skb_decrease_gso_size(shinfo, len_diff);
> + if (shinfo->frag_list)
> + return skb_linearize(skb);
> + }
> +
> }
>
> return 0;
> --
> 2.33.0
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH bpf v5] bpf: Fixed segment issue when downgrade gso_size
2024-07-06 14:26 ` Willem de Bruijn
` (2 preceding siblings ...)
2024-07-19 2:46 ` [PATCH v4] bpf: Fixed a segment issue " Fred Li
@ 2024-07-22 3:08 ` Fred Li
2024-07-22 16:29 ` Willem de Bruijn
3 siblings, 1 reply; 21+ messages in thread
From: Fred Li @ 2024-07-22 3:08 UTC (permalink / raw)
To: willemdebruijn.kernel
Cc: andrii, ast, bpf, daniel, herbert, john.fastabend, kpsingh,
linux-kernel, netdev, pabeni, song, yonghong.song, Fred Li
Linearize skb when downgrad gso_size to prevent triggering
the BUG_ON during segment skb as described in [1].
v5 changes:
- add bpf subject prefix.
- adjust message to imperative mood.
v4 changes:
- add fixed tag.
v3 changes:
- linearize skb if having frag_list as Willem de Bruijn suggested [2].
[1] https://lore.kernel.org/all/20240626065555.35460-2-dracodingfly@gmail.com/
[2] https://lore.kernel.org/all/668d5cf1ec330_1c18c32947@willemb.c.googlers.com.notmuch/
Fixes: 2be7e212d541 ("bpf: add bpf_skb_adjust_room helper")
Signed-off-by: Fred Li <dracodingfly@gmail.com>
---
net/core/filter.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index df4578219e82..71396ecfc574 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3525,13 +3525,21 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff,
if (skb_is_gso(skb)) {
struct skb_shared_info *shinfo = skb_shinfo(skb);
- /* Due to header grow, MSS needs to be downgraded. */
- if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO))
- skb_decrease_gso_size(shinfo, len_diff);
-
/* Header must be checked, and gso_segs recomputed. */
shinfo->gso_type |= gso_type;
shinfo->gso_segs = 0;
+
+ /* Due to header grow, MSS needs to be downgraded.
+ * There is BUG_ON when segment the frag_list with
+ * head_frag true so linearize skb after downgrade
+ * the MSS.
+ */
+ if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) {
+ skb_decrease_gso_size(shinfo, len_diff);
+ if (shinfo->frag_list)
+ return skb_linearize(skb);
+ }
+
}
return 0;
--
2.33.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH bpf v5] bpf: Fixed segment issue when downgrade gso_size
2024-07-22 3:08 ` [PATCH bpf v5] bpf: Fixed " Fred Li
@ 2024-07-22 16:29 ` Willem de Bruijn
2024-07-24 13:37 ` Fred Li
0 siblings, 1 reply; 21+ messages in thread
From: Willem de Bruijn @ 2024-07-22 16:29 UTC (permalink / raw)
To: Fred Li
Cc: andrii, ast, bpf, daniel, herbert, john.fastabend, kpsingh,
linux-kernel, netdev, pabeni, song, yonghong.song
On Sun, Jul 21, 2024 at 8:08 PM Fred Li <dracodingfly@gmail.com> wrote:
>
> Linearize skb when downgrad gso_size to prevent triggering
> the BUG_ON during segment skb as described in [1].
>
> v5 changes:
> - add bpf subject prefix.
> - adjust message to imperative mood.
>
> v4 changes:
> - add fixed tag.
>
> v3 changes:
> - linearize skb if having frag_list as Willem de Bruijn suggested [2].
>
> [1] https://lore.kernel.org/all/20240626065555.35460-2-dracodingfly@gmail.com/
> [2] https://lore.kernel.org/all/668d5cf1ec330_1c18c32947@willemb.c.googlers.com.notmuch/
>
> Fixes: 2be7e212d541 ("bpf: add bpf_skb_adjust_room helper")
> Signed-off-by: Fred Li <dracodingfly@gmail.com>
Reviewed-by: Willem de Bruijn <willemb@google.com>
My comments were informational, for a next patch if any, really. v4
was fine. v5 is too.
^ permalink raw reply [flat|nested] 21+ messages in thread