Message-ID: <20181130150116.GG24285@hmswarspite.think-freely.org>
Date:   Fri, 30 Nov 2018 10:01:16 -0500
From:   Neil Horman <nhorman@...driver.com>
To:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Cc:     Xin Long <lucien.xin@...il.com>,
        network dev <netdev@...r.kernel.org>,
        linux-sctp@...r.kernel.org, davem <davem@...emloft.net>
Subject: Re: [PATCHv2 net] sctp: hold transport before accessing its asoc in
 sctp_epaddr_lookup_transport

On Fri, Nov 30, 2018 at 12:18:52PM -0200, Marcelo Ricardo Leitner wrote:
> On Fri, Nov 30, 2018 at 09:05:06AM -0500, Neil Horman wrote:
> > On Fri, Nov 30, 2018 at 11:33:15AM -0200, Marcelo Ricardo Leitner wrote:
> > > On Fri, Nov 30, 2018 at 07:32:36AM -0500, Neil Horman wrote:
> > > > On Fri, Nov 30, 2018 at 02:04:16PM +0900, Xin Long wrote:
> > > > > On Fri, Nov 30, 2018 at 5:52 AM Neil Horman <nhorman@...driver.com> wrote:
> > > > > >
> > > > > > On Thu, Nov 29, 2018 at 02:44:07PM +0800, Xin Long wrote:
> > > > > > > Without holding transport to dereference its asoc, a use after
> > > > > > > free panic can be caused in sctp_epaddr_lookup_transport. Note
> > > > > > > that a sock lock can't protect these transports that belong to
> > > > > > > other socks.
> > > > > > >
> > > > > > > A similar fix as Commit bab1be79a516 ("sctp: hold transport
> > > > > > > before accessing its asoc in sctp_transport_get_next") is
> > > > > > > needed to hold the transport before accessing its asoc in
> > > > > > > sctp_epaddr_lookup_transport.
> > > > > > >
> > > > > > > Note that this extra atomic operation is on the datapath,
> > > > > > > but as rhlist keeps the lists to a small size, it won't
> > > > > > > see a noticeable performance hurt.
> > > > > > >
> > > > > > > v1->v2:
> > > > > > >   - improve the changelog.
> > > > > > >
> > > > > > > Fixes: 7fda702f9315 ("sctp: use new rhlist interface on sctp transport rhashtable")
> > > > > > > Reported-by: syzbot+aad231d51b1923158444@...kaller.appspotmail.com
> > > > > > > Signed-off-by: Xin Long <lucien.xin@...il.com>
> > > > > > > ---
> > > > > > >  net/sctp/input.c | 10 ++++++++--
> > > > > > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/net/sctp/input.c b/net/sctp/input.c
> > > > > > > index 5c36a99..ce7351c 100644
> > > > > > > --- a/net/sctp/input.c
> > > > > > > +++ b/net/sctp/input.c
> > > > > > > @@ -967,9 +967,15 @@ struct sctp_transport *sctp_epaddr_lookup_transport(
> > > > > > >       list = rhltable_lookup(&sctp_transport_hashtable, &arg,
> > > > > > >                              sctp_hash_params);
> > > > > > >
> > > > > > > -     rhl_for_each_entry_rcu(t, tmp, list, node)
> > > > > > > -             if (ep == t->asoc->ep)
> > > > > > > +     rhl_for_each_entry_rcu(t, tmp, list, node) {
> > > > > > > +             if (!sctp_transport_hold(t))
> > > > > > > +                     continue;
> > > > > > > +             if (ep == t->asoc->ep) {
> > > > > > > +                     sctp_transport_put(t);
> > > > > > >                       return t;
> > > > > > > +             }
> > > > > > > +             sctp_transport_put(t);
> > > > > > > +     }
> > > > > > >
> > > > > > >       return NULL;
> > > > > > >  }
> > > > > >
> > > > > > Wait a second, what if we just added an rcu_head to the association structure
> > > > > > and changed the kfree call in sctp_association_destroy to a kfree_rcu call
> > > > > > instead?  That would force the actual freeing of the association to pass through
> > > > > > a grace period, during which any in flight list traversal in
> > > > > > sctp_epaddr_lookup_transport could complete safely.  Its another two pointers
> > > > > We discussed this in last thread:
> > > > > https://www.spinics.net/lists/netdev/msg535191.html
> > > > > 
> > > > > It will cause closed sk to linger longer.
> > > > > 
> > > > Yes, but we never really got resolution on that topic.  I don't see that a
> > > 
> > > Fair point. We should have brought back the discussion online.
> > > 
> > > > socket lingering for an extra grace period is that big a deal.  I also don't see
> > > 
> > > What we really don't want is to bring back
> > > 8c98653f0553 ("sctp: sctp_close: fix release of bindings for deferred call_rcu's").
> > > (more below). That's where our fear lies.
> > > 
> > > > how sending the actual kfree through a grace period is going to cause the socket
> > > > to linger.  If you look at sctp_association_destroy, we call sock_put prior to
> > > > calling kfree at the end of the function.  All I'm looking for here is for the
> > > > memory free to wait until any list traversal in sctp_epaddr_lookup_transport is
> > > > done, which is what you are trying to do with your atomics.
> > > > 
> > > > As for your comment regarding sctp_transport_destroy_rcu, yes, that forces a
> > > > grace period when a transport is being destroyed, which will protect against
> > > > list corruption of the transport list here.  Thats good, but isn't what you are
> > > > trying to fix.  Your initial claim was that the asoc pointer for a given
> > > > transport was no longer valid, because it was getting freed while the transport
> > > > was still on the list.  That can clearly happen as we release all the transports
> > > > in sctp_association_free prior to calling what ostensibly is the last refrence
> > > > to their parent association at the end of that function, but its only the
> > > > transports that pass through a grace period before getting freed, the
> > > > association happens synchrnously, ignoring any grace period, and thats what we
> > > > need to change.
> > > > 
> > > > The more I look at it the more I'm convinced. What you're doing here is
> > > > definately overkill.  You need to add an rcu_head to the association and just do
> > > > the kfree of its memory after a grace period.  Its actually only a single grace
> > > > period as well.  If someone is traversing the transport list, both the transport
> > > > and association rcu callbacks will get run once the rcu_read_lock is released.
> > > 
> > > Ok, delaying *just* the kfree works too. It wouldn't bring back the
> > > issue I mentioned above.
> > > 
> > > We have basically 3 options then:
> > > 
> > > 1) your proposal above
> > >    extends sctp_association by rcu_head
> > >    delays the assoc kfree by a grace period, but just the kfree
> > > 2) the atomics, patch above
> > >    no struct growth
> > >    datapath atomics, but with no measurable impacts (kudos to rhlist)
> > > 3) cache ep pointer in sctp_transport
> > >    extends sctp_transport by a pointer
> > >    avoids double deref (t->asoc->ep)
> > >    this should work because we are only comparing ep pointers in
> > >      there and not using it after that.
> > >    might be tricky considering peeloff operation, but shouldn't be
> > >      much different from what we already have today with the asoc
> > >      migration itself.
> > > 
> > > Considering 2 is a no go, we have the other 2 options. Between 1 and
> > > 3, WDYT?
> > > 
> > I think that option (1) is the correct philisophical approach.  If we have
> > pointers that are accessed with the expectation of safety from rcu traversal, we
> > should bind the addition and removal of those data structures to rcu semantics.
> > Given that we are using rcu list traversal to walk through the transport list,
> > it seems to me we are implying rcu semantics.
> 
> rcu doesn't have to be like a plague that spreads around, but I guess
> I see your point.
> 
I agree with that.  But I do think theres a benefit to using it consistently.  I
would be equally ok with removing rcu locking from the transport level and doing
everything with refcounting, but I think the impact of that would be greater
than using rcu (though I'd be happy to be shown to be wrong)

Neil

> > 
> > That said, Option 3 works too, and might offer superior performance (in fact it
> > likely will), but I don't like the idea of caching the ep pointer.  We would be
> > doing so as much for safety as for performance, and I would be concerned that
> > caching that pointer opens up the possibility of likely bugs down the road (such
> > a cached pointer could only be used for comparison, and not for derefence, which
> > is wierd to say the least).
> 
> Good point, agree.
> 
> > 
> > I would vote for option 1
> 
> Xin?
> 
>   Marcelo
> 
> > 
> > Neil
> > 
> > > > 
> > > > 
> > > > Nak to this patch
> > > > Neil
> > > > 
> > > > > > worth of space in the association, but I think that would be a worthwhile
> > > > > > tradeoff for not having to do N atomic adds/puts every time you wanted to
> > > > > > receive or send a frame.
> > > > > N is not a big value, as rhlist itself keeps lists in a size.
> > > > > 
> > > > > >
> > > > > > Neil
> > > > > >
> > > > > > > --
> > > > > > > 2.1.0
> > > > > > >
> > > > > > >
> > > > > 
> > > 
> 

Powered by blists - more mailing lists