[PATCH 2/2] netfilter: helper: Fix helper unregister count.

[PATCH 2/2] netfilter: helper: Fix helper unregister count.

[Taehee Yoo]

helpers should unregister the only registered ports.
but, helper cannot have correct registered ports value when
failed to register.

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

diff --git a/net/netfilter/nf_conntrack_ftp.c b/net/netfilter/nf_conntrack_ftp.c
index 883c691..19efeba 100644
--- a/net/netfilter/nf_conntrack_ftp.c
+++ b/net/netfilter/nf_conntrack_ftp.c
@@ -632,6 +632,7 @@ static int __init nf_conntrack_ftp_init(void)
 			if (ret) {
 				pr_err("failed to register helper for pf: %d port: %d\n",
 				       ftp[i][j].tuple.src.l3num, ports[i]);
+				ports_c = i;
 				nf_conntrack_ftp_fini();
 				return ret;
 			}
diff --git a/net/netfilter/nf_conntrack_irc.c b/net/netfilter/nf_conntrack_irc.c
index 20ae74f..9844e62 100644
--- a/net/netfilter/nf_conntrack_irc.c
+++ b/net/netfilter/nf_conntrack_irc.c
@@ -271,6 +271,7 @@ static int __init nf_conntrack_irc_init(void)
 		if (ret) {
 			pr_err("failed to register helper for pf: %u port: %u\n",
 			       irc[i].tuple.src.l3num, ports[i]);
+			ports_c = i;
 			nf_conntrack_irc_fini();
 			return ret;
 		}
diff --git a/net/netfilter/nf_conntrack_sane.c b/net/netfilter/nf_conntrack_sane.c
index 7523a57..3fcbaab 100644
--- a/net/netfilter/nf_conntrack_sane.c
+++ b/net/netfilter/nf_conntrack_sane.c
@@ -223,6 +223,7 @@ static int __init nf_conntrack_sane_init(void)
 			if (ret) {
 				pr_err("failed to register helper for pf: %d port: %d\n",
 				       sane[i][j].tuple.src.l3num, ports[i]);
+				ports_c = i;
 				nf_conntrack_sane_fini();
 				return ret;
 			}
diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
index d523052..2c748bc 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c
@@ -1669,6 +1669,7 @@ static int __init nf_conntrack_sip_init(void)
 			if (ret) {
 				pr_err("failed to register helper for pf: %u port: %u\n",
 				       sip[i][j].tuple.src.l3num, ports[i]);
+				ports_c = i;
 				nf_conntrack_sip_fini();
 				return ret;
 			}
diff --git a/net/netfilter/nf_conntrack_tftp.c b/net/netfilter/nf_conntrack_tftp.c
index 47239fb..6d9c441 100644
--- a/net/netfilter/nf_conntrack_tftp.c
+++ b/net/netfilter/nf_conntrack_tftp.c
@@ -142,6 +142,7 @@ static int __init nf_conntrack_tftp_init(void)
 			if (ret) {
 				pr_err("failed to register helper for pf: %u port: %u\n",
 				       tftp[i][j].tuple.src.l3num, ports[i]);
+				ports_c = i;
 				nf_conntrack_tftp_fini();
 				return ret;
 			}
-- 
2.7.4

Re: [PATCH 2/2] netfilter: helper: Fix helper unregister count.

[Pablo Neira Ayuso]

On Tue, May 24, 2016 at 12:18:58PM +0800, Feng Gao wrote:
> Hi,
> 
> I have committed the patches. see the following links
> http://patchwork.ozlabs.org/patch/565169/
> http://patchwork.ozlabs.org/patch/565170/
> http://patchwork.ozlabs.org/patch/565171/
> 
> But i don't know why they are not accepted yet.
> I have did all advises of Pablo.

I prefer to take these small patch fixes from Taehee Yoo, so I can
pass back this to -stable. Once they are applied, I'd suggest you
rebase your large rework to introduce these new helper functions to
register conntrack helpers.

Thanks.

Re: [PATCH 2/2] netfilter: helper: Fix helper unregister count.

[Pablo Neira Ayuso]

On Tue, May 24, 2016 at 05:32:43PM +0800, Feng Gao wrote:
> Hi Pablo,
> 
> I remember my original commit is very simple.
> Just a few lines changes.
> 
> Then I make it more complicated according to your comments

Are you refering to this?

http://patchwork.ozlabs.org/patch/522709/

I suggested this specifically: "Could you investigate if it would be
possible to add a nf_conntrack_helpers_register()?"

Then, you followed up with a patchset that was fixing and reworking in
the same go.

The usual procedure is to split the submissions in logical changes,
starting from fixes (that are always prioritized) and then follow up
with rework/improvements/new features.

I also remember that at some point you withdrew indicating that there
was a problem, then came back telling this was OK, which confused me.

Re: [PATCH 2/2] netfilter: helper: Fix helper unregister count.

[Pablo Neira Ayuso]

On Tue, May 24, 2016 at 05:47:07PM +0800, Feng Gao wrote:
> yes.
> 
> It was my fault at that time.
> I thought there may be one problem that I forget test, so I just to notify
> you.
> I was afraid that it would break the work.

That's a valid concern, and I appreciate you indicated this. But then,
it's better if you resubmit and include in the description what you
did to test it.

> But I found i had tested that case after a while, it would not be wrong.
> Because that was my first commit to the netfilter, i don't want to break
> it. So i was too upset.

Don't get upset, it can be a bit frustrating to understand the whole
process in the beginning, and keep re-submitting if you think you're
doing the right thing.

Another note: I also remember you also posted patches out of the
"merge window".  Netfilter sticks to David Miller's net-next merge
window, which is now closed BTW. Merge window (not strictly this, but
an approximation) opens by when -rc1 is released and then it closes by
when the first mainline kernel version is released.

Thanks.

Re: [PATCH 2/2] netfilter: helper: Fix helper unregister count.

[Pablo Neira Ayuso]

On Tue, May 24, 2016 at 06:03:41PM +0800, Feng Gao wrote:
> Hi Pablo,
> 
> Then could you give me some tips that how could i should to do next?
> Rebase the codes, and resubmit the commit about register helpers?
> Or Give up this and try to other commits?

Let's get the fixes in the tree first. Then, I'd suggest you follow up
with your patches once the fixes show up in nf-next.

Re: [PATCH 2/2] netfilter: helper: Fix helper unregister count.

[Pablo Neira Ayuso]

On Sat, May 14, 2016 at 10:19:53PM +0900, Taehee Yoo wrote:
> helpers should unregister the only registered ports.
> but, helper cannot have correct registered ports value when
> failed to register.

Applied, thanks.