Re: [PATCH 1/2] netfilter: helper: Fix incorrect helper name.

[Pablo Neira Ayuso <pablo@netfilter.org>]

[-- Attachment #1: Type: text/plain, Size: 1627 bytes --]

On Mon, May 23, 2016 at 12:03:55AM +0900, Taehee Yoo wrote:
> 2016-05-17 19:38 GMT+09:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> > On Sat, May 14, 2016 at 10:19:16PM +0900, Taehee Yoo wrote:
> >> when register to helper, each helper adds port to name.
> >> correct form is 'protocol name-port' but irc, sip and tftp adds
> >> a iterator value. so it fix it.
> >
> > Could you track since when this works in this way?
> >
> > This inconsistency has been probably there since long time ago, and we
> > expose this names through iptables -m helper.
> >
> > What I mean is: I understand this is inconsistent, but if we change
> > this now, we may break existing rulesets.
> 
> 
> Thank you for your review.
> And Apologize for late reply.
> 
> I agree that patch destroys so much rulesets.
> but I want to solve the issue that is helper cannot check duplicated
> helper rules.
> nf_conntrack_helper_register() checks name && l3num && protonum to
> check duplicated rules.
> but tftp, sip and irc helper always have unique helper name because
> that includes iterator value.
> (tftp-1, tftp-2, tftp-3 ...)
> helper-name is good method to check duplicated rules.
> but we need another check method to solve this issue and keep rulsets.
> so far, my idea is that using help callback function's pointer address.
> pseudo code is : "if (port && l3num && protonum && help)"
> 
> Do you have any advice?

Probably something like this?

The idea is to compare the helper name, stripping off the '-value'
from the name so we catch if the user specific duplicated ports via
module option, which is what is causing problems to you, right?

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 1095 bytes --]

diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index f703adb..5785034 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -361,9 +361,9 @@ EXPORT_SYMBOL_GPL(nf_ct_helper_log);
 
 int nf_conntrack_helper_register(struct nf_conntrack_helper *me)
 {
-	int ret = 0;
 	struct nf_conntrack_helper *cur;
 	unsigned int h = helper_hash(&me->tuple);
+	int ret = 0, len;
 
 	BUG_ON(me->expect_policy == NULL);
 	BUG_ON(me->expect_class_max >= NF_CT_MAX_EXPECT_CLASSES);
@@ -371,7 +371,13 @@ int nf_conntrack_helper_register(struct nf_conntrack_helper *me)
 
 	mutex_lock(&nf_ct_helper_mutex);
 	hlist_for_each_entry(cur, &nf_ct_helper_hash[h], hnode) {
-		if (strncmp(cur->name, me->name, NF_CT_HELPER_NAME_LEN) == 0 &&
+		slash = strchr(cur->name, '-');
+		if (slash)
+			len = slash - cur->name;
+		else
+			len = NF_CT_HELPER_NAME_LEN;
+
+		if (strncmp(cur->name, me->name, len) == 0 &&
 		    cur->tuple.src.l3num == me->tuple.src.l3num &&
 		    cur->tuple.dst.protonum == me->tuple.dst.protonum) {
 			ret = -EEXIST;