netdev - Re: [PATCH net 2/3] sctp: hold transport before we access t->asoc in sctp proc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160121200840.GH3452@mrl.redhat.com>
Date:	Thu, 21 Jan 2016 18:08:40 -0200
From:	Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	Xin Long <lucien.xin@...il.com>,
	network dev <netdev@...r.kernel.org>,
	linux-sctp@...r.kernel.org, Vlad Yasevich <vyasevich@...il.com>,
	daniel@...earbox.net, davem@...emloft.net
Subject: Re: [PATCH net 2/3] sctp: hold transport before we access t->asoc in
 sctp proc

On Thu, Jan 21, 2016 at 11:57:16AM -0800, Eric Dumazet wrote:
> On Thu, 2016-01-21 at 17:37 -0200, Marcelo Ricardo Leitner wrote:
> > On Thu, Jan 21, 2016 at 11:27:36AM -0800, Eric Dumazet wrote:
> > > On Fri, 2016-01-22 at 01:49 +0800, Xin Long wrote:
> > > > Previously, before rhashtable, /proc assoc listing was done by
> > > > read-locking the entire hash entry and dumping all assocs at once, so we
> > > > were sure that the assoc wasn't freed because it wouldn't be possible to
> > > > remove it from the hash meanwhile.
> > > > 
> > > > Now we use rhashtable to list transports, and dump entries one by one.
> > > > That is, now we have to check if the assoc is still a good one, as the
> > > > transport we got may be being freed.
> > > > 
> > > > Signed-off-by: Xin Long <lucien.xin@...il.com>
> > > > ---
> > > >  net/sctp/proc.c | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > > 
> > > > diff --git a/net/sctp/proc.c b/net/sctp/proc.c
> > > > index 684c5b3..c74a810 100644
> > > > --- a/net/sctp/proc.c
> > > > +++ b/net/sctp/proc.c
> > > > @@ -380,6 +380,8 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
> > > >  	}
> > > >  
> > > >  	transport = (struct sctp_transport *)v;
> > > 
> > > What protects you from this structure already being freed ?
> > 
> > rcu, rhashtable_walk_start() at sctp_assocs_seq_start() starts an
> > (implicit from this POV) rcu_read_lock() for us which is unlocked only
> > when the walking is terminated, thus covering this _show.
> > 
> > > > +	if (!sctp_transport_hold(transport))
> > > > +		return 0;
> > > 
> > > If this is rcu, then you do not need to increment the refcount, and
> > > decrement it later.
> > 
> > It's an implicit hold on sctp asoc.
> > 
> > This code is using contents from asoc pointer, which is not proctected
> > by rcu. As transport has a hold on the asoc, it's enough to just hold
> > the transport and not the asoc too, as we had to do in the previous
> > patch.
> 
> Then it means fast path also need to do this sctp_transport_hold() ?

Well, kind of broad question, but I think so, yes. It's mostly done when
the transport is identified and fetched from rhashtable. Otherwise, we
probably already have the asoc and doesn't need this jump.

It's the first patch in this series. It's the only way we found to
safely transfer the ref from transport to asoc.

> If sctp_association_put() was called from sctp_transport_destroy_rcu()
> (ie after rcu grace period), you would not need to increment/decrement
> the transport refcount.
> 
> Normally, RCU protection does not need to change the refcount, unless we
> need to keep an object alive after escaping the rcu section.

sctp_association_put() was in sctp_transport_destroy_rcu(), but it
caused sctp-issues which Daniel fixed on 8c98653f0553 ("sctp:
sctp_close: fix release of bindings for deferred call_rcu's").

So in this case, we are not leaving the protected section but jumping
from a RCU-protected object (transport) to a non-protected one (asoc).

  Marcelo