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.
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
