Message ID | 1490792404-6035-1-git-send-email-zlpnobody@163.com |
---|---|
State | Changes Requested |
Delegated to: | Pablo Neira |
Headers | show |
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? -- 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 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. -- 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
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? -- 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 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! -- 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 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! -- 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
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. -- 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/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]);