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

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

[Florian Westphal]

read access doesn't need any lock here.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 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;
-	spin_unlock_bh(&ct->lock);
-
-	seq_printf(s, "%s ", sctp_conntrack_names[state]);
+	seq_printf(s, "%s ", sctp_conntrack_names[ct->proto.sctp.state]);
 }
 
 #define for_each_sctp_chunk(skb, sch, _sch, offset, dataoff, count)	\
diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index 278f3b9..e0cb0ce 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -313,13 +313,7 @@ static void tcp_print_tuple(struct seq_file *s,
 /* Print out the private part of the conntrack. */
 static void tcp_print_conntrack(struct seq_file *s, struct nf_conn *ct)
 {
-	enum tcp_conntrack state;
-
-	spin_lock_bh(&ct->lock);
-	state = ct->proto.tcp.state;
-	spin_unlock_bh(&ct->lock);
-
-	seq_printf(s, "%s ", tcp_conntrack_names[state]);
+	seq_printf(s, "%s ", tcp_conntrack_names[ct->proto.tcp.state]);
 }
 
 static unsigned int get_conntrack_index(const struct tcphdr *tcph)
-- 
2.7.3

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

[Pablo Neira Ayuso]

On Mon, Apr 11, 2016 at 09:14:29PM +0200, Florian Westphal wrote:
> read access doesn't need any lock here.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  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?

> -	spin_unlock_bh(&ct->lock);
> -
> -	seq_printf(s, "%s ", sctp_conntrack_names[state]);
> +	seq_printf(s, "%s ", sctp_conntrack_names[ct->proto.sctp.state]);
>  }
>  
>  #define for_each_sctp_chunk(skb, sch, _sch, offset, dataoff, count)	\

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

[Florian Westphal]

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?

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

[Pablo Neira Ayuso]

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?

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

[Florian Westphal]

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.