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@xxxxxxx>:
> > From: Liping Zhang <zlpnobody@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
