On Thu, Jun 6, 2024 at 9:49 AM Mina Almasry [off-list ref] wrote:

On Tue, Jun 4, 2024 at 3:46 AM Paolo Abeni [off-list ref] wrote:

quoted

On Thu, 2024-05-30 at 20:16 +0000, Mina Almasry wrote:

quoted

diff --git a/net/core/gro.c b/net/core/gro.c
index 26f09c3e830b7..7b9d018f552bd 100644
--- a/net/core/gro.c
+++ b/net/core/gro.c

@@ -422,6 +422,9 @@ static void gro_pull_from_frag0(struct sk_buff *skb, int grow)
 {
      struct skb_shared_info *pinfo = skb_shinfo(skb);

+     if (WARN_ON_ONCE(!skb_frags_readable(skb)))
+             return;
+
      BUG_ON(skb->end - skb->tail < grow);

      memcpy(skb_tail_pointer(skb), NAPI_GRO_CB(skb)->frag0, grow);

@@ -443,7 +446,7 @@ static void gro_try_pull_from_frag0(struct sk_buff *skb)
 {
      int grow = skb_gro_offset(skb) - skb_headlen(skb);

-     if (grow > 0)
+     if (grow > 0 && skb_frags_readable(skb))
              gro_pull_from_frag0(skb, grow);
 }

I'm unsure if this was already mentioned, so please pardon the eventual
duplicate...

The above code is quite critical performance wise, and the previous
patch already prevent frag0 from being set to a non paged frag,

Hi Paolo!

The last patch, d4d25dd237a61 ("net: support non paged skb frags"),
AFAICT doesn't prevent frag0 from being a non-paged frag. What we do
is set ->frag0=skb->data, then prevent it from being reset to
skb_frag_address() for non-paged skbs. ->frag0 will likely actually be
a bad value for non-paged frags, so we need to check in
gro_pul_from_frag0() so that we don't accidentally pull from a bad
->frag0 value.

What I think I should do here is what you said. I should make sure
frag0 and frag0_len is not set if it's a non-paged frag. Then, we
don't need special checks in gro_pull_from_frag0 I think, because
skb_gro_may_pull() should detect that frag0_len is 0 and should
prevent a pull.

I will apply this fix to the next iteration for your review. Let me
know if I missed something.

Actually, sorry you're right. As written, d4d25dd237a61 ("net: support
non paged skb frags") prevents frag0 from being a non-paged frag. I
can just drop these excessive checks with no downside. Sorry for the
noise!

-- 
Thanks,
Mina