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

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

[Taehee Yoo]

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.

Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---
 net/netfilter/nf_conntrack_irc.c  | 2 +-
 net/netfilter/nf_conntrack_sip.c  | 2 +-
 net/netfilter/nf_conntrack_tftp.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nf_conntrack_irc.c b/net/netfilter/nf_conntrack_irc.c
index 8b6da27..20ae74f 100644
--- a/net/netfilter/nf_conntrack_irc.c
+++ b/net/netfilter/nf_conntrack_irc.c
@@ -265,7 +265,7 @@ static int __init nf_conntrack_irc_init(void)
 		if (ports[i] == IRC_PORT)
 			sprintf(irc[i].name, "irc");
 		else
-			sprintf(irc[i].name, "irc-%u", i);
+			sprintf(irc[i].name, "irc-%u", ports[i]);
 
 		ret = nf_conntrack_helper_register(&irc[i]);
 		if (ret) {
diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
index 3e06402..d523052 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c
@@ -1661,7 +1661,7 @@ static int __init nf_conntrack_sip_init(void)
 			if (ports[i] == SIP_PORT)
 				sprintf(sip[i][j].name, "sip");
 			else
-				sprintf(sip[i][j].name, "sip-%u", i);
+				sprintf(sip[i][j].name, "sip-%u", ports[i]);
 
 			pr_debug("port #%u: %u\n", i, ports[i]);
 
diff --git a/net/netfilter/nf_conntrack_tftp.c b/net/netfilter/nf_conntrack_tftp.c
index 36f9640..47239fb 100644
--- a/net/netfilter/nf_conntrack_tftp.c
+++ b/net/netfilter/nf_conntrack_tftp.c
@@ -136,7 +136,7 @@ static int __init nf_conntrack_tftp_init(void)
 			if (ports[i] == TFTP_PORT)
 				sprintf(tftp[i][j].name, "tftp");
 			else
-				sprintf(tftp[i][j].name, "tftp-%u", i);
+				sprintf(tftp[i][j].name, "tftp-%u", ports[i]);
 
 			ret = nf_conntrack_helper_register(&tftp[i][j]);
 			if (ret) {
-- 
2.7.4

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

[Pablo Neira Ayuso]

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.

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

[Taehee Yoo]

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?

Taehee Yoo

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

[Pablo Neira Ayuso]

[-- 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;

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

[Pablo Neira Ayuso]

On Tue, May 24, 2016 at 05:36:30PM +0800, Feng Gao wrote:
> Yes, right.
> 
> You mean the module forbid the user enters the duplicated ports now.

OK, if this is OK to you I'll submit this, thanks for reviewing.

BTW:

A: No, please don't do it.
Q: Is it fine to top-post in this mailing list? [1]

:)

[1] https://en.wikipedia.org/wiki/Posting_style

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

[Taehee Yoo]

2016-05-24 13:33 GMT+09:00 Feng Gao <gfree.wind@gmail.com>:
> Hi Pablo,
>
> I committed the similar fix long ago, about some month ago.
> I have changed the codes according to your advice.
> The links are http://patchwork.ozlabs.org/patch/565169/,
> http://patchwork.ozlabs.org/patch/565170,
> http://patchwork.ozlabs.org/patch/565171
>
> But they are not accepted until now.
> Is there any problem now?
>
> Best Regards
> Feng
>
> On Sun, May 22, 2016 at 11:03 PM, Taehee Yoo <ap420073@gmail.com> 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?
>>
>> Taehee Yoo
>> --
>> 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,

I Apologize for my mistake.
I didn't check patchwork before sending this patch.

Also thank you for much friendly advice

Taehee Yoo