Re: [PATCH 4.14] tcp: refine memory limit test in tcp_fragment()

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Jun 26, 2019 at 08:48:46AM +0800, Greg KH wrote:
> On Tue, Jun 25, 2019 at 03:49:33PM -0700, Josh Hunt wrote:
> > On 6/25/19 3:18 PM, Greg KH wrote:
> > > On Tue, Jun 25, 2019 at 01:29:35PM -0700, Josh Hunt wrote:
> > > > On 6/25/19 1:26 PM, Sasha Levin wrote:
> > > > > On Tue, Jun 25, 2019 at 01:19:37PM -0400, Josh Hunt wrote:
> > > > > > Backport of dad3a9314ac95dedc007bc7dacacb396ea10e376:
> > > > > 
> > > > > You probably meant b6653b3629e5b88202be3c9abc44713973f5c4b4 here.
> > > > 
> > > > I wasn't sure if I should reference the upstream commit or stable commit.
> > > 
> > > The upstream commit please.
> > 
> > Thanks. I'll fix for next version.
> > 
> > > 
> > > > dad3a9314 is the version of the commit from linux-4.14.y. There may be a
> > > > similar issue with the Fixes tag below since that also references the 4.14
> > > > vers of the change.
> > > > 
> > > > > 
> > > > > > tcp_fragment() might be called for skbs in the write queue.
> > > > > > 
> > > > > > Memory limits might have been exceeded because tcp_sendmsg() only
> > > > > > checks limits at full skb (64KB) boundaries.
> > > > > > 
> > > > > > Therefore, we need to make sure tcp_fragment() wont punish applications
> > > > > > that might have setup very low SO_SNDBUF values.
> > > > > > 
> > > > > > Backport notes:
> > > > > > Initial version used tcp_queue type which is not present in older
> > > > > > kernels,
> > > > > > so added a new arg to tcp_fragment() to determine whether this is a
> > > > > > retransmit or not.
> > > > > > 
> > > > > > Fixes: 9daf226ff926 ("tcp: tcp_fragment() should apply sane memory
> > > > > > limits")
> > > > > > Signed-off-by: Josh Hunt <johunt@akamai.com>
> > > > > > Reviewed-by: Jason Baron <jbaron@akamai.com>
> > > > > > ---
> > > > > > 
> > > > > > Eric/Greg - This applies on top of v4.14.130. I did not see anything come
> > > > > > through for the older (<4.19) stable kernels yet. Without this change
> > > > > > Christoph Paasch's packetrill script (https://lore.kernel.org/netdev/CALMXkpYVRxgeqarp4gnmX7GqYh1sWOAt6UaRFqYBOaaNFfZ5sw@mail.gmail.com/)
> > > > > > 
> > > > > > will fail on 4.14 stable kernels, but passes with this change.
> > > > > 
> > > > > Eric, it would be great if you could Ack this, it's very different from
> > > > > your original patch.
> > > > 
> > > > Yes, that would be great.
> > > 
> > > I would prefer if this looks a bit more like the upstream fix, perhaps a
> > > backport of the function that added the "direction" of the packet first,
> > > and then Eric's patch?  As it is, this patch adds a different parameter
> > > to the function than what is in Linus's tree, and I bet will cause
> > > problems at some later point in time.
> > 
> > The commit which introduced the fn arguments is part of a much larger change
> > that created a separate rb-tree for the retransmit queue:
> > 
> > commit 75c119afe14f74b4dd967d75ed9f57ab6c0ef045
> > Author: Eric Dumazet <edumazet@google.com>
> > Date:   Thu Oct 5 22:21:27 2017 -0700
> > 
> >     tcp: implement rb-tree based retransmit queue
> > 
> > I can backport the portion of this change which basically does this:
> > 
> > +enum tcp_queue {
> > +       TCP_FRAG_IN_WRITE_QUEUE,
> > +       TCP_FRAG_IN_RTX_QUEUE,
> > +};
> > +int tcp_fragment(struct sock *sk, enum tcp_queue tcp_queue,
> > +                struct sk_buff *skb, u32 len,
> > +                unsigned int mss_now, gfp_t gfp);
> > 
> > and the corresponding call-sites of tcp_fragment(). If we do that then
> > Eric's fix (b6653b3629e5b88202be3c9abc44713973f5c4b4) should apply cleanly
> > on top of linux-4.14.y. I'm happy to do that if you'd rather go that route.
> 
> Yes, that is what I was thinking of, thanks.  You expressed it much
> better than I could have before my morning coffee :)
> 
> > If you want the full rb-tree change into 4.14 then I would defer that to
> > Eric, but would argue that IMHO is probably too invasive of a change for a
> > LTS kernel.
> 
> No, I don't think we should do that work for 4.14.
> 
> Also, your change would be suitable for backporting to the older stable
> kernels that also need this (4.9.y and 4.4.y.)

Nevermind with this, in another email thread Eric provided a simpler
patch which I have now just queued up to the stable kernel trees.

I'll probably just do a quick round of review/releases now for this
issue as people are hitting it already.

thanks,

greg k-h