On Fri, Nov 30, 2018 at 10:33 PM Marcelo Ricardo Leitner
<marcelo.leitner@xxxxxxxxx> 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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > > > Signed-off-by: Xin Long <lucien.xin@xxxxxxxxx>
> > > > > ---
> > > > > 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 vote for 2 if 2 still has a chance, sorry :D
Unless I can see a case that shows this atomic operation
really hurts performance.
>
> >
> >
> > 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
> > > > >
> > > > >
> > >
