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....@gmail.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. Marcelo
