Re: [PATCH 1/1] netfilter: Add helper array register/unregister functions
From: Pablo Neira Ayuso
Date: Wed Jul 20 2016 - 04:40:23 EST
On Wed, Jul 20, 2016 at 08:51:17AM +0800, Liping Zhang wrote:
> 2016-07-18 11:39 GMT+08:00 <fgao@xxxxxxxxxx>:
> > From: Gao Feng <fgao@xxxxxxxxxx>
> >
> > Add nf_ct_helper_init, nf_conntrack_helpers_register/unregister
> > functions to enhance the conntrack helper codes.
>
> I think this patch is breaking something ...
>
> This irc:
> > - if (ports[i] == IRC_PORT)
> > - sprintf(irc[i].name, "irc");
> > - else
> > - sprintf(irc[i].name, "irc-%u", i);
> > -
> > - ret = nf_conntrack_helper_register(&irc[i]);
> > + nf_ct_helper_init(&irc[i], AF_INET, IPPROTO_TCP, "irc",
> > + IRC_PORT, ports[i], &irc_exp_policy, 0, 0,
> > + help, NULL, THIS_MODULE);
> > + }
>
> This sip:
> > - if (ports[i] == SIP_PORT)
> > - sprintf(sip[i][j].name, "sip");
> > - else
> > - sprintf(sip[i][j].name, "sip-%u", i);
>
> And this tftp:
> > - if (ports[i] == TFTP_PORT)
> > - sprintf(tftp[i][j].name, "tftp");
> > - else
> > - sprintf(tftp[i][j].name, "tftp-%u", i);
>
> For example, if the user install the nf_conntrack_tftp module an
> specify the ports to "69,10069",
> then the helper name is "tftp" and "tftp-1".
>
> But apply this patch, the helper name will be changed to "tftp" and
> "tftp-10069", this may break the
> existing iptables rules which used the helper match or CT target.
>
> And this was already discussed at
> https://patchwork.ozlabs.org/patch/622238/
Thanks for spotting this.
I'm going to collapse this patch that I'm attaching to Feng's work to
address this.
diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
index 778f1fc..1eaac1f 100644
--- a/include/net/netfilter/nf_conntrack_helper.h
+++ b/include/net/netfilter/nf_conntrack_helper.h
@@ -60,7 +60,7 @@ struct nf_conntrack_helper *nf_conntrack_helper_try_module_get(const char *name,
u8 protonum);
void nf_ct_helper_init(struct nf_conntrack_helper *helper,
u16 l3num, u16 protonum, const char *name,
- u16 default_port, u16 spec_port,
+ u16 default_port, u16 spec_port, u32 id,
const struct nf_conntrack_expect_policy *exp_pol,
u32 expect_class_max, u32 data_len,
int (*help)(struct sk_buff *skb, unsigned int protoff,
diff --git a/net/netfilter/nf_conntrack_ftp.c b/net/netfilter/nf_conntrack_ftp.c
index d47ada9..4314700 100644
--- a/net/netfilter/nf_conntrack_ftp.c
+++ b/net/netfilter/nf_conntrack_ftp.c
@@ -601,12 +601,12 @@ static int __init nf_conntrack_ftp_init(void)
are tracked or not - YK */
for (i = 0; i < ports_c; i++) {
nf_ct_helper_init(&ftp[2 * i], AF_INET, IPPROTO_TCP, "ftp",
- FTP_PORT, ports[i], &ftp_exp_policy, 0,
- sizeof(struct nf_ct_ftp_master), help,
+ FTP_PORT, ports[i], ports[i], &ftp_exp_policy,
+ 0, sizeof(struct nf_ct_ftp_master), help,
nf_ct_ftp_from_nlattr, THIS_MODULE);
nf_ct_helper_init(&ftp[2 * i + 1], AF_INET6, IPPROTO_TCP, "ftp",
- FTP_PORT, ports[i], &ftp_exp_policy, 0,
- sizeof(struct nf_ct_ftp_master), help,
+ FTP_PORT, ports[i], ports[i], &ftp_exp_policy,
+ 0, sizeof(struct nf_ct_ftp_master), help,
nf_ct_ftp_from_nlattr, THIS_MODULE);
}
diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index ed6c0e5..b989b81 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -467,7 +467,7 @@ EXPORT_SYMBOL_GPL(nf_conntrack_helper_unregister);
void nf_ct_helper_init(struct nf_conntrack_helper *helper,
u16 l3num, u16 protonum, const char *name,
- u16 default_port, u16 spec_port,
+ u16 default_port, u16 spec_port, u32 id,
const struct nf_conntrack_expect_policy *exp_pol,
u32 expect_class_max, u32 data_len,
int (*help)(struct sk_buff *skb, unsigned int protoff,
@@ -490,8 +490,7 @@ void nf_ct_helper_init(struct nf_conntrack_helper *helper,
if (spec_port == default_port)
snprintf(helper->name, sizeof(helper->name), "%s", name);
else
- snprintf(helper->name, sizeof(helper->name), "%s-%u", name,
- spec_port);
+ snprintf(helper->name, sizeof(helper->name), "%s-%u", name, id);
}
EXPORT_SYMBOL_GPL(nf_ct_helper_init);
diff --git a/net/netfilter/nf_conntrack_irc.c b/net/netfilter/nf_conntrack_irc.c
index b93e5e7..1972a14 100644
--- a/net/netfilter/nf_conntrack_irc.c
+++ b/net/netfilter/nf_conntrack_irc.c
@@ -256,8 +256,8 @@ static int __init nf_conntrack_irc_init(void)
for (i = 0; i < ports_c; i++) {
nf_ct_helper_init(&irc[i], AF_INET, IPPROTO_TCP, "irc",
- IRC_PORT, ports[i], &irc_exp_policy, 0, 0,
- help, NULL, THIS_MODULE);
+ IRC_PORT, ports[i], i, &irc_exp_policy,
+ 0, 0, help, NULL, THIS_MODULE);
}
ret = nf_conntrack_helpers_register(&irc[0], ports_c);
diff --git a/net/netfilter/nf_conntrack_sane.c b/net/netfilter/nf_conntrack_sane.c
index 536c208..9dcb9ee 100644
--- a/net/netfilter/nf_conntrack_sane.c
+++ b/net/netfilter/nf_conntrack_sane.c
@@ -195,11 +195,13 @@ static int __init nf_conntrack_sane_init(void)
are tracked or not - YK */
for (i = 0; i < ports_c; i++) {
nf_ct_helper_init(&sane[2 * i], AF_INET, IPPROTO_TCP, "sane",
- SANE_PORT, ports[i], &sane_exp_policy, 0,
+ SANE_PORT, ports[i], ports[i],
+ &sane_exp_policy, 0,
sizeof(struct nf_ct_sane_master), help, NULL,
THIS_MODULE);
nf_ct_helper_init(&sane[2 * i + 1], AF_INET6, IPPROTO_TCP, "sane",
- SANE_PORT, ports[i], &sane_exp_policy, 0,
+ SANE_PORT, ports[i], ports[i],
+ &sane_exp_policy, 0,
sizeof(struct nf_ct_sane_master), help, NULL,
THIS_MODULE);
}
diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
index 3feaab3..075286b 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c
@@ -1630,22 +1630,22 @@ static int __init nf_conntrack_sip_init(void)
memset(&sip[i], 0, sizeof(sip[i]));
nf_ct_helper_init(&sip[4 * i], AF_INET, IPPROTO_UDP, "sip",
- SIP_PORT, ports[i], &sip_exp_policy[0],
+ SIP_PORT, ports[i], i, &sip_exp_policy[0],
SIP_EXPECT_MAX,
sizeof(struct nf_ct_sip_master), sip_help_udp,
NULL, THIS_MODULE);
nf_ct_helper_init(&sip[4 * i + 1], AF_INET, IPPROTO_TCP, "sip",
- SIP_PORT, ports[i], &sip_exp_policy[0],
+ SIP_PORT, ports[i], i, &sip_exp_policy[0],
SIP_EXPECT_MAX,
sizeof(struct nf_ct_sip_master), sip_help_tcp,
NULL, THIS_MODULE);
nf_ct_helper_init(&sip[4 * i + 2], AF_INET6, IPPROTO_UDP, "sip",
- SIP_PORT, ports[i], &sip_exp_policy[0],
+ SIP_PORT, ports[i], i, &sip_exp_policy[0],
SIP_EXPECT_MAX,
sizeof(struct nf_ct_sip_master), sip_help_udp,
NULL, THIS_MODULE);
nf_ct_helper_init(&sip[4 * i + 3], AF_INET6, IPPROTO_TCP, "sip",
- SIP_PORT, ports[i], &sip_exp_policy[0],
+ SIP_PORT, ports[i], i, &sip_exp_policy[0],
SIP_EXPECT_MAX,
sizeof(struct nf_ct_sip_master), sip_help_tcp,
NULL, THIS_MODULE);
diff --git a/net/netfilter/nf_conntrack_tftp.c b/net/netfilter/nf_conntrack_tftp.c
index 4d65321..b1227dc 100644
--- a/net/netfilter/nf_conntrack_tftp.c
+++ b/net/netfilter/nf_conntrack_tftp.c
@@ -118,11 +118,11 @@ static int __init nf_conntrack_tftp_init(void)
for (i = 0; i < ports_c; i++) {
nf_ct_helper_init(&tftp[2 * i], AF_INET, IPPROTO_UDP, "tftp",
- TFTP_PORT, ports[i], &tftp_exp_policy, 0, 0,
- tftp_help, NULL, THIS_MODULE);
+ TFTP_PORT, ports[i], i, &tftp_exp_policy,
+ 0, 0, tftp_help, NULL, THIS_MODULE);
nf_ct_helper_init(&tftp[2 * i + 1], AF_INET6, IPPROTO_UDP, "tftp",
- TFTP_PORT, ports[i], &tftp_exp_policy, 0, 0,
- tftp_help, NULL, THIS_MODULE);
+ TFTP_PORT, ports[i], i, &tftp_exp_policy,
+ 0, 0, tftp_help, NULL, THIS_MODULE);
}
ret = nf_conntrack_helpers_register(tftp, ports_c * 2);