Re: [PATCH nf-next] netfilter: conntrack: don't acquire lock during seq_printf

[Florian Westphal <fw@strlen.de>]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, Apr 14, 2016 at 01:16:56PM +0200, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > >  net/netfilter/nf_conntrack_proto_sctp.c | 8 +-------
> > > >  net/netfilter/nf_conntrack_proto_tcp.c  | 8 +-------
> > > >  2 files changed, 2 insertions(+), 14 deletions(-)
> > > > 
> > > > diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c
> > > > index 9578a7c..1d7ab96 100644
> > > > --- a/net/netfilter/nf_conntrack_proto_sctp.c
> > > > +++ b/net/netfilter/nf_conntrack_proto_sctp.c
> > > > @@ -191,13 +191,7 @@ static void sctp_print_tuple(struct seq_file *s,
> > > >  /* Print out the private part of the conntrack. */
> > > >  static void sctp_print_conntrack(struct seq_file *s, struct nf_conn *ct)
> > > >  {
> > > > -	enum sctp_conntrack state;
> > > > -
> > > > -	spin_lock_bh(&ct->lock);
> > > > -	state = ct->proto.sctp.state;
> > > 
> > > Don't we need at least READ_ONCE() here?
> > 
> > Why?
> > 
> > seq_printf(s, "%s ", sctp_conntrack_names[ct->proto.sctp.state]);
> > 
> > I think thats fine, where do you see a problem?
> 
> ct->proto.sctp.state may be modified from another cpu while reading
> this, is this read guaranteed to be atomic in any arch?

Yes, safe on all.  Only problem would be if we change
sctp.state to u64, we'd have to add locking for 32bit arches.

I can add a compiletime_assert_atomic_type() for this but I think its
not needed.

I don't feel strongly about it; if you prefer locking here just mark patch
as rejected.