[PATCH nf V2] netfilter: nf_ct_helper: permit cthelpers with different names via nfnetlink

[PATCH nf V2] netfilter: nf_ct_helper: permit cthelpers with different names via nfnetlink

[Liping Zhang]

From: Liping Zhang <zlpnobody@gmail.com>

cthelpers added via nfnetlink may have the same tuple, i.e. except for
the l3proto and l4proto, other fields are all zero. So even with the
different names, we will also fail to add them:
  # nfct helper add ssdp inet udp
  # nfct helper add tftp inet udp
  nfct v1.4.3: netlink error: File exists

So in order to avoid unpredictable behaviour, we should:
1. cthelpers can be selected by nft ct helper obj or xt_CT target, so
report error if duplicated { name, l3proto, l4proto } tuple exist.
2. cthelpers can be selected by nf_ct_tuple_src_mask_cmp when
nf_ct_auto_assign_helper is enabled, so also report error if duplicated
{ l3proto, l4proto, src-port } tuple exist.

Also note, if the cthelper is added from userspace, then the src-port will
always be zero, it's invalid for nf_ct_auto_assign_helper, so there's no
need to check the second point listed above.

Fixes: 893e093c786c ("netfilter: nf_ct_helper: bail out on duplicated helpers")
Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
---
 V2: drop to use __nf_conntrack_helper_find which may cause annoying
     rcu warning when debug is enabled, spotted by Pablo.

 net/netfilter/nf_conntrack_helper.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index 6dc44d9..92e69af 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -379,17 +379,33 @@ int nf_conntrack_helper_register(struct nf_conntrack_helper *me)
 	struct nf_conntrack_tuple_mask mask = { .src.u.all = htons(0xFFFF) };
 	unsigned int h = helper_hash(&me->tuple);
 	struct nf_conntrack_helper *cur;
-	int ret = 0;
+	int ret = 0, i;
 
 	BUG_ON(me->expect_policy == NULL);
 	BUG_ON(me->expect_class_max >= NF_CT_MAX_EXPECT_CLASSES);
 	BUG_ON(strlen(me->name) > NF_CT_HELPER_NAME_LEN - 1);
 
 	mutex_lock(&nf_ct_helper_mutex);
-	hlist_for_each_entry(cur, &nf_ct_helper_hash[h], hnode) {
-		if (nf_ct_tuple_src_mask_cmp(&cur->tuple, &me->tuple, &mask)) {
-			ret = -EEXIST;
-			goto out;
+	for (i = 0; i < nf_ct_helper_hsize; i++) {
+		hlist_for_each_entry(cur, &nf_ct_helper_hash[i], hnode) {
+			if (!strcmp(cur->name, me->name) &&
+			    (cur->tuple.src.l3num == NFPROTO_UNSPEC ||
+			     cur->tuple.src.l3num == me->tuple.src.l3num) &&
+			    cur->tuple.dst.protonum == me->tuple.dst.protonum) {
+				ret = -EEXIST;
+				goto out;
+			}
+		}
+	}
+
+	/* avoid unpredictable behaviour for auto_assign_helper */
+	if (!(me->flags & NF_CT_HELPER_F_USERSPACE)) {
+		hlist_for_each_entry(cur, &nf_ct_helper_hash[h], hnode) {
+			if (nf_ct_tuple_src_mask_cmp(&cur->tuple, &me->tuple,
+						     &mask)) {
+				ret = -EEXIST;
+				goto out;
+			}
 		}
 	}
 	hlist_add_head_rcu(&me->hnode, &nf_ct_helper_hash[h]);
-- 
2.5.5

Re: [PATCH nf V2] netfilter: nf_ct_helper: permit cthelpers with different names via nfnetlink

[Liping Zhang]

Hi Pablo,

2017-03-29 21:00 GMT+08:00 Liping Zhang <zlpnobody@163.com>:
> From: Liping Zhang <zlpnobody@gmail.com>
>
> cthelpers added via nfnetlink may have the same tuple, i.e. except for
> the l3proto and l4proto, other fields are all zero. So even with the
> different names, we will also fail to add them:
>   # nfct helper add ssdp inet udp
>   # nfct helper add tftp inet udp
>   nfct v1.4.3: netlink error: File exists
>
> So in order to avoid unpredictable behaviour, we should:
> 1. cthelpers can be selected by nft ct helper obj or xt_CT target, so
> report error if duplicated { name, l3proto, l4proto } tuple exist.
> 2. cthelpers can be selected by nf_ct_tuple_src_mask_cmp when
> nf_ct_auto_assign_helper is enabled, so also report error if duplicated
> { l3proto, l4proto, src-port } tuple exist.
>
> Also note, if the cthelper is added from userspace, then the src-port will
> always be zero, it's invalid for nf_ct_auto_assign_helper, so there's no
> need to check the second point listed above.
>
> Fixes: 893e093c786c ("netfilter: nf_ct_helper: bail out on duplicated helpers")
> Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
> ---
>  V2: drop to use __nf_conntrack_helper_find which may cause annoying
>      rcu warning when debug is enabled, spotted by Pablo.

I think this patch should be ignored.

>  net/netfilter/nf_conntrack_helper.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
[...]
> +       for (i = 0; i < nf_ct_helper_hsize; i++) {
> +               hlist_for_each_entry(cur, &nf_ct_helper_hash[i], hnode) {
> +                       if (!strcmp(cur->name, me->name) &&
> +                           (cur->tuple.src.l3num == NFPROTO_UNSPEC ||
> +                            cur->tuple.src.l3num == me->tuple.src.l3num) &&
> +                           cur->tuple.dst.protonum == me->tuple.dst.protonum) {
> +                               ret = -EEXIST;
> +                               goto out;
> +                       }
> +               }
> +       }

After I have a closer look, inside hlist_for_each_entry_rcu, we use the
rcu_dereference_raw() to get the pointer, and this will not generate warning:

#define hlist_for_each_entry_rcu(pos, head, member) \
    for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
                         typeof(*(pos)), member);
   ....

Then "This is likely going to spot false positives with the RCU
debugging instrumentation"
will not happen.

So I think the "https://patchwork.ozlabs.org/patch/743472/" looks better?

Re: [PATCH nf V2] netfilter: nf_ct_helper: permit cthelpers with different names via nfnetlink

[Pablo Neira Ayuso]

On Thu, Mar 30, 2017 at 10:12:00AM +0800, Liping Zhang wrote:
> Hi Pablo,
> 
> 2017-03-29 21:00 GMT+08:00 Liping Zhang <zlpnobody@163.com>:
> > From: Liping Zhang <zlpnobody@gmail.com>
> >
> > cthelpers added via nfnetlink may have the same tuple, i.e. except for
> > the l3proto and l4proto, other fields are all zero. So even with the
> > different names, we will also fail to add them:
> >   # nfct helper add ssdp inet udp
> >   # nfct helper add tftp inet udp
> >   nfct v1.4.3: netlink error: File exists
> >
> > So in order to avoid unpredictable behaviour, we should:
> > 1. cthelpers can be selected by nft ct helper obj or xt_CT target, so
> > report error if duplicated { name, l3proto, l4proto } tuple exist.
> > 2. cthelpers can be selected by nf_ct_tuple_src_mask_cmp when
> > nf_ct_auto_assign_helper is enabled, so also report error if duplicated
> > { l3proto, l4proto, src-port } tuple exist.
> >
> > Also note, if the cthelper is added from userspace, then the src-port will
> > always be zero, it's invalid for nf_ct_auto_assign_helper, so there's no
> > need to check the second point listed above.
> >
> > Fixes: 893e093c786c ("netfilter: nf_ct_helper: bail out on duplicated helpers")
> > Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
> > ---
> >  V2: drop to use __nf_conntrack_helper_find which may cause annoying
> >      rcu warning when debug is enabled, spotted by Pablo.
> 
> I think this patch should be ignored.
> 
> >  net/netfilter/nf_conntrack_helper.c | 26 +++++++++++++++++++++-----
> >  1 file changed, 21 insertions(+), 5 deletions(-)
> [...]
> > +       for (i = 0; i < nf_ct_helper_hsize; i++) {
> > +               hlist_for_each_entry(cur, &nf_ct_helper_hash[i], hnode) {
> > +                       if (!strcmp(cur->name, me->name) &&
> > +                           (cur->tuple.src.l3num == NFPROTO_UNSPEC ||
> > +                            cur->tuple.src.l3num == me->tuple.src.l3num) &&
> > +                           cur->tuple.dst.protonum == me->tuple.dst.protonum) {
> > +                               ret = -EEXIST;
> > +                               goto out;
> > +                       }
> > +               }
> > +       }
> 
> After I have a closer look, inside hlist_for_each_entry_rcu, we use the
> rcu_dereference_raw() to get the pointer, and this will not generate warning:
> 
> #define hlist_for_each_entry_rcu(pos, head, member) \
>     for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
>                          typeof(*(pos)), member);
>    ....
> 
> Then "This is likely going to spot false positives with the RCU
> debugging instrumentation"
> will not happen.

Right, instrumentation will not trigger any problem.

But even if instrumention is not a problem, I just would like to avoid
people sending me "obvious" fixes afterwards, by removing _rcu since
they see this code runs under mutex or how knows what.

Re: [PATCH nf V2] netfilter: nf_ct_helper: permit cthelpers with different names via nfnetlink

[Liping Zhang]

Hi Pablo,

2017-04-14 6:29 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
[...]
>> After I have a closer look, inside hlist_for_each_entry_rcu, we use the
>> rcu_dereference_raw() to get the pointer, and this will not generate warning:
>>
>> #define hlist_for_each_entry_rcu(pos, head, member) \
>>     for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
>>                          typeof(*(pos)), member);
>>    ....
>>
>> Then "This is likely going to spot false positives with the RCU
>> debugging instrumentation"
>> will not happen.
>
> Right, instrumentation will not trigger any problem.
>
> But even if instrumention is not a problem, I just would like to avoid
> people sending me "obvious" fixes afterwards, by removing _rcu since
> they see this code runs under mutex or how knows what.

I'm a little confusing about this one.

I found "http://patchwork.ozlabs.org/patch/744786/" and
"http://patchwork.ozlabs.org/patch/743472/" were both set
to "Changes Requested".

So which one is you prefer to :)? What's next step should I do?

Re: [PATCH nf V2] netfilter: nf_ct_helper: permit cthelpers with different names via nfnetlink

[Pablo Neira Ayuso]

On Fri, Apr 14, 2017 at 07:50:26AM +0800, Liping Zhang wrote:
> Hi Pablo,
> 
> 2017-04-14 6:29 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> [...]
> >> After I have a closer look, inside hlist_for_each_entry_rcu, we use the
> >> rcu_dereference_raw() to get the pointer, and this will not generate warning:
> >>
> >> #define hlist_for_each_entry_rcu(pos, head, member) \
> >>     for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
> >>                          typeof(*(pos)), member);
> >>    ....
> >>
> >> Then "This is likely going to spot false positives with the RCU
> >> debugging instrumentation"
> >> will not happen.
> >
> > Right, instrumentation will not trigger any problem.
> >
> > But even if instrumention is not a problem, I just would like to avoid
> > people sending me "obvious" fixes afterwards, by removing _rcu since
> > they see this code runs under mutex or how knows what.
> 
> I'm a little confusing about this one.
> 
> I found "http://patchwork.ozlabs.org/patch/744786/" and
> "http://patchwork.ozlabs.org/patch/743472/" were both set
> to "Changes Requested".
> 
> So which one is you prefer to :)? What's next step should I do?

The latter, please resubmit bumping your version number and log.

That makes things easier for me than going back and forth trying to
figure out what I should do, thanks!

Re: [PATCH nf V2] netfilter: nf_ct_helper: permit cthelpers with different names via nfnetlink

[Pablo Neira Ayuso]

On Fri, Apr 14, 2017 at 01:57:02AM +0200, Pablo Neira Ayuso wrote:
> On Fri, Apr 14, 2017 at 07:50:26AM +0800, Liping Zhang wrote:
> > Hi Pablo,
> > 
> > 2017-04-14 6:29 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> > [...]
> > >> After I have a closer look, inside hlist_for_each_entry_rcu, we use the
> > >> rcu_dereference_raw() to get the pointer, and this will not generate warning:
> > >>
> > >> #define hlist_for_each_entry_rcu(pos, head, member) \
> > >>     for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
> > >>                          typeof(*(pos)), member);
> > >>    ....
> > >>
> > >> Then "This is likely going to spot false positives with the RCU
> > >> debugging instrumentation"
> > >> will not happen.
> > >
> > > Right, instrumentation will not trigger any problem.
> > >
> > > But even if instrumention is not a problem, I just would like to avoid
> > > people sending me "obvious" fixes afterwards, by removing _rcu since
> > > they see this code runs under mutex or how knows what.
> > 
> > I'm a little confusing about this one.
> > 
> > I found "http://patchwork.ozlabs.org/patch/744786/" and
> > "http://patchwork.ozlabs.org/patch/743472/" were both set
> > to "Changes Requested".
> > 
> > So which one is you prefer to :)? What's next step should I do?
> 
> The latter, please resubmit bumping your version number and log.
> 
> That makes things easier for me than going back and forth trying to
> figure out what I should do, thanks!

Hm, this patch requires no changes actually. Now I understand why
you're confused there.

So let me know if you I should just take this or wait for you to
resubmit.

In case of doubt, resubmitting is just fine. Thanks!

Re: [PATCH nf V2] netfilter: nf_ct_helper: permit cthelpers with different names via nfnetlink

[Liping Zhang]

Hi Pablo,

2017-04-15 17:28 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> Hm, this patch requires no changes actually. Now I understand why
> you're confused there.
>
> So let me know if you I should just take this or wait for you to
> resubmit.
>
> In case of doubt, resubmitting is just fine. Thanks!

I will resubmit it :). Actually,I find that this patch conflicts with
the latest nf tree now. Just wait a few minutes.