* [PATCH nf-next] netfilter: conntrack: don't acquire lock during seq_printf
@ 2016-04-11 19:14 Florian Westphal
[not found] ` <20160414094346.GA2056@salvia>
0 siblings, 1 reply; 5+ messages in thread
From: Florian Westphal @ 2016-04-11 19:14 UTC (permalink / raw)
To: netfilter-devel; +Cc: 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
^ permalink raw reply related [flat|nested] 5+ messages in thread[parent not found: <20160414094346.GA2056@salvia>]
* Re: [PATCH nf-next] netfilter: conntrack: don't acquire lock during seq_printf [not found] ` <20160414094346.GA2056@salvia> @ 2016-04-14 10:05 ` Pablo Neira Ayuso 2016-04-14 11:16 ` Florian Westphal 0 siblings, 1 reply; 5+ messages in thread From: Pablo Neira Ayuso @ 2016-04-14 10:05 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel 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) \ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH nf-next] netfilter: conntrack: don't acquire lock during seq_printf 2016-04-14 10:05 ` Pablo Neira Ayuso @ 2016-04-14 11:16 ` Florian Westphal 2016-04-14 12:27 ` Pablo Neira Ayuso 0 siblings, 1 reply; 5+ messages in thread From: Florian Westphal @ 2016-04-14 11:16 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel 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? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH nf-next] netfilter: conntrack: don't acquire lock during seq_printf 2016-04-14 11:16 ` Florian Westphal @ 2016-04-14 12:27 ` Pablo Neira Ayuso 2016-04-14 12:39 ` Florian Westphal 0 siblings, 1 reply; 5+ messages in thread From: Pablo Neira Ayuso @ 2016-04-14 12:27 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel 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? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH nf-next] netfilter: conntrack: don't acquire lock during seq_printf 2016-04-14 12:27 ` Pablo Neira Ayuso @ 2016-04-14 12:39 ` Florian Westphal 0 siblings, 0 replies; 5+ messages in thread From: Florian Westphal @ 2016-04-14 12:39 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-04-14 12:39 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-04-11 19:14 [PATCH nf-next] netfilter: conntrack: don't acquire lock during seq_printf Florian Westphal
[not found] ` <20160414094346.GA2056@salvia>
2016-04-14 10:05 ` Pablo Neira Ayuso
2016-04-14 11:16 ` Florian Westphal
2016-04-14 12:27 ` Pablo Neira Ayuso
2016-04-14 12:39 ` Florian Westphal
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).