| Message ID | 1461863628-23350-6-git-send-email-fw@strlen.de |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
On Thu, Apr 28, 2016 at 07:13:44PM +0200, Florian Westphal wrote: > The iteration process is lockless, so we test if the conntrack object is > eligible for printing (e.g. is AF_INET) after obtaining the reference > count. > > Once we put all conntracks into same hash table we might see more > entries that need to be skipped. > > So add a helper and first perform the test in a lockless fashion > for fast skip. > > Once we obtain the reference count, just repeat the check. > > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > .../netfilter/nf_conntrack_l3proto_ipv4_compat.c | 24 +++++++++++++++++----- > 1 file changed, 19 insertions(+), 5 deletions(-) > > diff --git a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c > index f0dfe92..483cf79 100644 > --- a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c > +++ b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c > @@ -114,6 +114,19 @@ static inline void ct_show_secctx(struct seq_file *s, const struct nf_conn *ct) > } > #endif > > +static bool ct_seq_should_skip(const struct nf_conn *ct, > + const struct nf_conntrack_tuple_hash *hash) > +{ > + /* we only want to print DIR_ORIGINAL */ > + if (NF_CT_DIRECTION(hash)) > + return true; > + > + if (nf_ct_l3num(ct) != AF_INET) > + return true; > + > + return false; > +} > + > static int ct_seq_show(struct seq_file *s, void *v) > { > struct nf_conntrack_tuple_hash *hash = v; > @@ -123,14 +136,15 @@ static int ct_seq_show(struct seq_file *s, void *v) > int ret = 0; > > NF_CT_ASSERT(ct); > - if (unlikely(!atomic_inc_not_zero(&ct->ct_general.use))) > + if (ct_seq_should_skip(ct, hash)) > return 0; > > + if (unlikely(!atomic_inc_not_zero(&ct->ct_general.use))) > + return 0; > > - /* we only want to print DIR_ORIGINAL */ > - if (NF_CT_DIRECTION(hash)) > - goto release; > - if (nf_ct_l3num(ct) != AF_INET) > + /* check if we raced w. object reuse */ > + if (!nf_ct_is_confirmed(ct) || This refactoring includes this new check, is this intentional? > + ct_seq_should_skip(ct, hash)) > goto release; > > l3proto = __nf_ct_l3proto_find(nf_ct_l3num(ct)); > -- > 2.7.3 > -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > - if (NF_CT_DIRECTION(hash)) > > - goto release; > > - if (nf_ct_l3num(ct) != AF_INET) > > + /* check if we raced w. object reuse */ > > + if (!nf_ct_is_confirmed(ct) || > > This refactoring includes this new check, is this intentional? Hmm, yes and no. I should have put it in an extra commit :-/ Without this, we might erronously print a conntrack that is NEW and which isn't confirmed yet. We won't crash since seq_print doesn't depend on extensions being set up properly, but it seems better to only display those conntracks that are part of the conntrack hash table (i.e., have the confirmed bit set). Let me know if you want me to respin this as a separate fix, thanks! -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 03, 2016 at 08:12:50PM +0200, Pablo Neira Ayuso wrote: > On Thu, Apr 28, 2016 at 07:13:44PM +0200, Florian Westphal wrote: > > The iteration process is lockless, so we test if the conntrack object is > > eligible for printing (e.g. is AF_INET) after obtaining the reference > > count. > > > > Once we put all conntracks into same hash table we might see more > > entries that need to be skipped. > > > > So add a helper and first perform the test in a lockless fashion > > for fast skip. > > > > Once we obtain the reference count, just repeat the check. > > > > Signed-off-by: Florian Westphal <fw@strlen.de> > > --- > > .../netfilter/nf_conntrack_l3proto_ipv4_compat.c | 24 +++++++++++++++++----- > > 1 file changed, 19 insertions(+), 5 deletions(-) > > > > diff --git a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c > > index f0dfe92..483cf79 100644 > > --- a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c > > +++ b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c > > @@ -114,6 +114,19 @@ static inline void ct_show_secctx(struct seq_file *s, const struct nf_conn *ct) > > } > > #endif > > > > +static bool ct_seq_should_skip(const struct nf_conn *ct, > > + const struct nf_conntrack_tuple_hash *hash) > > +{ > > + /* we only want to print DIR_ORIGINAL */ > > + if (NF_CT_DIRECTION(hash)) > > + return true; > > + > > + if (nf_ct_l3num(ct) != AF_INET) > > + return true; > > + > > + return false; > > +} > > + > > static int ct_seq_show(struct seq_file *s, void *v) > > { > > struct nf_conntrack_tuple_hash *hash = v; > > @@ -123,14 +136,15 @@ static int ct_seq_show(struct seq_file *s, void *v) > > int ret = 0; > > > > NF_CT_ASSERT(ct); > > - if (unlikely(!atomic_inc_not_zero(&ct->ct_general.use))) > > + if (ct_seq_should_skip(ct, hash)) > > return 0; > > > > + if (unlikely(!atomic_inc_not_zero(&ct->ct_general.use))) > > + return 0; > > > > - /* we only want to print DIR_ORIGINAL */ > > - if (NF_CT_DIRECTION(hash)) > > - goto release; > > - if (nf_ct_l3num(ct) != AF_INET) > > + /* check if we raced w. object reuse */ > > + if (!nf_ct_is_confirmed(ct) || > > This refactoring includes this new check, is this intentional? It seems this check was previously missing, I can just amend the commit log with a couple of lines to document that this patch also includes this missing check. No problem. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 04, 2016 at 12:27:36AM +0200, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > - if (NF_CT_DIRECTION(hash)) > > > - goto release; > > > - if (nf_ct_l3num(ct) != AF_INET) > > > + /* check if we raced w. object reuse */ > > > + if (!nf_ct_is_confirmed(ct) || > > > > This refactoring includes this new check, is this intentional? > > Hmm, yes and no. > > I should have put it in an extra commit :-/ > > Without this, we might erronously print a conntrack that is NEW > and which isn't confirmed yet. > > We won't crash since seq_print doesn't depend on extensions being > set up properly, but it seems better to only display those conntracks > that are part of the conntrack hash table (i.e., have the confirmed bit > set). I see, a conntrack that shouldn't be printed be sneak in the listing. > Let me know if you want me to respin this as a separate fix, thanks! I will just append a notice on the commit message before applying. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c index f0dfe92..483cf79 100644 --- a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c +++ b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c @@ -114,6 +114,19 @@ static inline void ct_show_secctx(struct seq_file *s, const struct nf_conn *ct) } #endif +static bool ct_seq_should_skip(const struct nf_conn *ct, + const struct nf_conntrack_tuple_hash *hash) +{ + /* we only want to print DIR_ORIGINAL */ + if (NF_CT_DIRECTION(hash)) + return true; + + if (nf_ct_l3num(ct) != AF_INET) + return true; + + return false; +} + static int ct_seq_show(struct seq_file *s, void *v) { struct nf_conntrack_tuple_hash *hash = v; @@ -123,14 +136,15 @@ static int ct_seq_show(struct seq_file *s, void *v) int ret = 0; NF_CT_ASSERT(ct); - if (unlikely(!atomic_inc_not_zero(&ct->ct_general.use))) + if (ct_seq_should_skip(ct, hash)) return 0; + if (unlikely(!atomic_inc_not_zero(&ct->ct_general.use))) + return 0; - /* we only want to print DIR_ORIGINAL */ - if (NF_CT_DIRECTION(hash)) - goto release; - if (nf_ct_l3num(ct) != AF_INET) + /* check if we raced w. object reuse */ + if (!nf_ct_is_confirmed(ct) || + ct_seq_should_skip(ct, hash)) goto release; l3proto = __nf_ct_l3proto_find(nf_ct_l3num(ct));
The iteration process is lockless, so we test if the conntrack object is eligible for printing (e.g. is AF_INET) after obtaining the reference count. Once we put all conntracks into same hash table we might see more entries that need to be skipped. So add a helper and first perform the test in a lockless fashion for fast skip. Once we obtain the reference count, just repeat the check. Signed-off-by: Florian Westphal <fw@strlen.de> --- .../netfilter/nf_conntrack_l3proto_ipv4_compat.c | 24 +++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-)