[PATCH net-next 0/8] openvswitch: load and reference the NAT helper.

[PATCH net-next 0/8] openvswitch: load and reference the NAT helper.

[Flavio Leitner]

The request_module() is quite expensive and triggers the
usermode helper in userspace. Instead, load only if the
module is not present and keep module references to avoid
problems.

The first patch standardize the module alias which is already
there, but not in a formal way.

The second patch adds an API to point to the NAT helper.

The following patches will register each NAT helper using
the new API.

The last patch fixes openvswitch to use the new API to
load and reference the NAT helper and also report an error
if the operation fails.

Flavio Leitner (8):
  netfilter: use macros to create module aliases.
  netfilter: add API to manage NAT helpers.
  netfilter: nf_nat: register amanda NAT helper.
  netfilter: nf_nat: register ftp NAT helper.
  netfilter: nf_nat: register irc NAT helper.
  netfilter: nf_nat: register sip NAT helper.
  netfilter: nf_nat: register tftp NAT helper.
  openvswitch: load and reference the NAT helper.

 include/net/netfilter/nf_conntrack_helper.h |  23 ++++-
 net/ipv4/netfilter/nf_nat_h323.c            |   2 +-
 net/ipv4/netfilter/nf_nat_pptp.c            |   2 +-
 net/netfilter/nf_conntrack_amanda.c         |   2 +
 net/netfilter/nf_conntrack_ftp.c            |   6 +-
 net/netfilter/nf_conntrack_helper.c         | 108 +++++++++++++++++++-
 net/netfilter/nf_conntrack_irc.c            |   3 +-
 net/netfilter/nf_conntrack_sane.c           |   4 +-
 net/netfilter/nf_conntrack_sip.c            |  12 ++-
 net/netfilter/nf_conntrack_tftp.c           |   6 +-
 net/netfilter/nf_nat_amanda.c               |   9 +-
 net/netfilter/nf_nat_ftp.c                  |   8 +-
 net/netfilter/nf_nat_irc.c                  |   8 +-
 net/netfilter/nf_nat_sip.c                  |   8 +-
 net/netfilter/nf_nat_tftp.c                 |   8 +-
 net/openvswitch/conntrack.c                 |  27 +++--
 16 files changed, 209 insertions(+), 27 deletions(-)

-- 
2.20.1

[PATCH net-next 1/8] netfilter: use macros to create module aliases.

[Flavio Leitner]

Each NAT helper creates a module alias which follows a pattern.
Use macros for consistency.

Signed-off-by: Flavio Leitner <fbl@sysclose.org>
---
 include/net/netfilter/nf_conntrack_helper.h | 4 ++++
 net/ipv4/netfilter/nf_nat_h323.c            | 2 +-
 net/ipv4/netfilter/nf_nat_pptp.c            | 2 +-
 net/netfilter/nf_nat_amanda.c               | 2 +-
 net/netfilter/nf_nat_ftp.c                  | 2 +-
 net/netfilter/nf_nat_irc.c                  | 2 +-
 net/netfilter/nf_nat_sip.c                  | 2 +-
 net/netfilter/nf_nat_tftp.c                 | 2 +-
 8 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
index ec52a8dc32fd..e86fadf7e7c5 100644
--- a/include/net/netfilter/nf_conntrack_helper.h
+++ b/include/net/netfilter/nf_conntrack_helper.h
@@ -15,6 +15,10 @@
 #include <net/netfilter/nf_conntrack_extend.h>
 #include <net/netfilter/nf_conntrack_expect.h>
 
+#define NF_CT_NAT_HELPER_MOD_NAME(name)	"ip_nat_" name
+#define MODULE_ALIAS_NFCT_HELPER_NAT(name) \
+	MODULE_ALIAS(NF_CT_NAT_HELPER_MOD_NAME(name))
+
 struct module;
 
 enum nf_ct_helper_flags {
diff --git a/net/ipv4/netfilter/nf_nat_h323.c b/net/ipv4/netfilter/nf_nat_h323.c
index 4e6b53ab6c33..09754e787692 100644
--- a/net/ipv4/netfilter/nf_nat_h323.c
+++ b/net/ipv4/netfilter/nf_nat_h323.c
@@ -631,4 +631,4 @@ module_exit(fini);
 MODULE_AUTHOR("Jing Min Zhao <zhaojingmin@users.sourceforge.net>");
 MODULE_DESCRIPTION("H.323 NAT helper");
 MODULE_LICENSE("GPL");
-MODULE_ALIAS("ip_nat_h323");
+MODULE_ALIAS_NFCT_HELPER_NAT("h323");
diff --git a/net/ipv4/netfilter/nf_nat_pptp.c b/net/ipv4/netfilter/nf_nat_pptp.c
index 68b4d450391b..1a984e5db88a 100644
--- a/net/ipv4/netfilter/nf_nat_pptp.c
+++ b/net/ipv4/netfilter/nf_nat_pptp.c
@@ -37,7 +37,7 @@
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Harald Welte <laforge@gnumonks.org>");
 MODULE_DESCRIPTION("Netfilter NAT helper module for PPTP");
-MODULE_ALIAS("ip_nat_pptp");
+MODULE_ALIAS_NFCT_HELPER_NAT("pptp");
 
 static void pptp_nat_expected(struct nf_conn *ct,
 			      struct nf_conntrack_expect *exp)
diff --git a/net/netfilter/nf_nat_amanda.c b/net/netfilter/nf_nat_amanda.c
index e4d61a7a5258..e87075763f73 100644
--- a/net/netfilter/nf_nat_amanda.c
+++ b/net/netfilter/nf_nat_amanda.c
@@ -22,7 +22,7 @@
 MODULE_AUTHOR("Brian J. Murrell <netfilter@interlinx.bc.ca>");
 MODULE_DESCRIPTION("Amanda NAT helper");
 MODULE_LICENSE("GPL");
-MODULE_ALIAS("ip_nat_amanda");
+MODULE_ALIAS_NFCT_HELPER_NAT("amanda");
 
 static unsigned int help(struct sk_buff *skb,
 			 enum ip_conntrack_info ctinfo,
diff --git a/net/netfilter/nf_nat_ftp.c b/net/netfilter/nf_nat_ftp.c
index 5063cbf1689c..19f5739fd5e2 100644
--- a/net/netfilter/nf_nat_ftp.c
+++ b/net/netfilter/nf_nat_ftp.c
@@ -24,7 +24,7 @@
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Rusty Russell <rusty@rustcorp.com.au>");
 MODULE_DESCRIPTION("ftp NAT helper");
-MODULE_ALIAS("ip_nat_ftp");
+MODULE_ALIAS_NFCT_HELPER_NAT("ftp");
 
 /* FIXME: Time out? --RR */
 
diff --git a/net/netfilter/nf_nat_irc.c b/net/netfilter/nf_nat_irc.c
index 3aa35a43100d..c18e3ce6589b 100644
--- a/net/netfilter/nf_nat_irc.c
+++ b/net/netfilter/nf_nat_irc.c
@@ -26,7 +26,7 @@
 MODULE_AUTHOR("Harald Welte <laforge@gnumonks.org>");
 MODULE_DESCRIPTION("IRC (DCC) NAT helper");
 MODULE_LICENSE("GPL");
-MODULE_ALIAS("ip_nat_irc");
+MODULE_ALIAS_NFCT_HELPER_NAT("irc");
 
 static unsigned int help(struct sk_buff *skb,
 			 enum ip_conntrack_info ctinfo,
diff --git a/net/netfilter/nf_nat_sip.c b/net/netfilter/nf_nat_sip.c
index aa1be643d7a0..f31c2a1b95b8 100644
--- a/net/netfilter/nf_nat_sip.c
+++ b/net/netfilter/nf_nat_sip.c
@@ -27,7 +27,7 @@
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Christian Hentschel <chentschel@arnet.com.ar>");
 MODULE_DESCRIPTION("SIP NAT helper");
-MODULE_ALIAS("ip_nat_sip");
+MODULE_ALIAS_NFCT_HELPER_NAT("sip");
 
 
 static unsigned int mangle_packet(struct sk_buff *skb, unsigned int protoff,
diff --git a/net/netfilter/nf_nat_tftp.c b/net/netfilter/nf_nat_tftp.c
index 7f67e1d5310d..51673aa6e1dc 100644
--- a/net/netfilter/nf_nat_tftp.c
+++ b/net/netfilter/nf_nat_tftp.c
@@ -16,7 +16,7 @@
 MODULE_AUTHOR("Magnus Boden <mb@ozaba.mine.nu>");
 MODULE_DESCRIPTION("TFTP NAT helper");
 MODULE_LICENSE("GPL");
-MODULE_ALIAS("ip_nat_tftp");
+MODULE_ALIAS_NFCT_HELPER_NAT("tftp");
 
 static unsigned int help(struct sk_buff *skb,
 			 enum ip_conntrack_info ctinfo,
-- 
2.20.1

[PATCH net-next 2/8] netfilter: add API to manage NAT helpers.

[Flavio Leitner]

The API allows a conntrack helper to indicate its corresponding
NAT helper which then can be loaded and reference counted.

Signed-off-by: Flavio Leitner <fbl@sysclose.org>
---
 include/net/netfilter/nf_conntrack_helper.h |  19 +++-
 net/netfilter/nf_conntrack_amanda.c         |   2 +
 net/netfilter/nf_conntrack_ftp.c            |   6 +-
 net/netfilter/nf_conntrack_helper.c         | 108 +++++++++++++++++++-
 net/netfilter/nf_conntrack_irc.c            |   3 +-
 net/netfilter/nf_conntrack_sane.c           |   4 +-
 net/netfilter/nf_conntrack_sip.c            |  12 ++-
 net/netfilter/nf_conntrack_tftp.c           |   6 +-
 8 files changed, 147 insertions(+), 13 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
index e86fadf7e7c5..0d36d6bfb522 100644
--- a/include/net/netfilter/nf_conntrack_helper.h
+++ b/include/net/netfilter/nf_conntrack_helper.h
@@ -58,6 +58,8 @@ struct nf_conntrack_helper {
 	unsigned int queue_num;
 	/* length of userspace private data stored in nf_conn_help->data */
 	u16 data_len;
+	/* name of NAT helper module */
+	char nat_mod_name[NF_CT_HELPER_NAME_LEN];
 };
 
 /* Must be kept in sync with the classes defined by helpers */
@@ -98,7 +100,8 @@ void nf_ct_helper_init(struct nf_conntrack_helper *helper,
 				   enum ip_conntrack_info ctinfo),
 		       int (*from_nlattr)(struct nlattr *attr,
 					  struct nf_conn *ct),
-		       struct module *module);
+		       struct module *module,
+		       const char *nat_mod_name);
 
 int nf_conntrack_helper_register(struct nf_conntrack_helper *);
 void nf_conntrack_helper_unregister(struct nf_conntrack_helper *);
@@ -157,4 +160,18 @@ nf_ct_helper_expectfn_find_by_symbol(const void *symbol);
 extern struct hlist_head *nf_ct_helper_hash;
 extern unsigned int nf_ct_helper_hsize;
 
+struct nf_conntrack_helper_nat {
+	struct list_head list;
+	char name[NF_CT_HELPER_NAME_LEN];
+	struct module *module;		/* pointer to self */
+};
+
+void nf_ct_helper_nat_init(struct nf_conntrack_helper_nat *nat,
+			   const char *name, struct module *module);
+
+void nf_conntrack_helper_nat_register(struct nf_conntrack_helper_nat *nat);
+void nf_conntrack_helper_nat_unregister(struct nf_conntrack_helper_nat *nat);
+int nf_conntrack_helper_nat_try_module_get(const char *name, u16 l3num,
+					   u8 protonum);
+void nf_conntrack_helper_nat_put(struct nf_conntrack_helper *helper);
 #endif /*_NF_CONNTRACK_HELPER_H*/
diff --git a/net/netfilter/nf_conntrack_amanda.c b/net/netfilter/nf_conntrack_amanda.c
index f2681ec5b5f6..b5d255897d9e 100644
--- a/net/netfilter/nf_conntrack_amanda.c
+++ b/net/netfilter/nf_conntrack_amanda.c
@@ -186,6 +186,7 @@ static struct nf_conntrack_helper amanda_helper[2] __read_mostly = {
 		.tuple.src.u.udp.port	= cpu_to_be16(10080),
 		.tuple.dst.protonum	= IPPROTO_UDP,
 		.expect_policy		= &amanda_exp_policy,
+		.nat_mod_name		= NF_CT_NAT_HELPER_MOD_NAME("amanda"),
 	},
 	{
 		.name			= "amanda",
@@ -195,6 +196,7 @@ static struct nf_conntrack_helper amanda_helper[2] __read_mostly = {
 		.tuple.src.u.udp.port	= cpu_to_be16(10080),
 		.tuple.dst.protonum	= IPPROTO_UDP,
 		.expect_policy		= &amanda_exp_policy,
+		.nat_mod_name		= NF_CT_NAT_HELPER_MOD_NAME("amanda"),
 	},
 };
 
diff --git a/net/netfilter/nf_conntrack_ftp.c b/net/netfilter/nf_conntrack_ftp.c
index a11c304fb771..fec9bb462071 100644
--- a/net/netfilter/nf_conntrack_ftp.c
+++ b/net/netfilter/nf_conntrack_ftp.c
@@ -590,10 +590,12 @@ static int __init nf_conntrack_ftp_init(void)
 	for (i = 0; i < ports_c; i++) {
 		nf_ct_helper_init(&ftp[2 * i], AF_INET, IPPROTO_TCP, "ftp",
 				  FTP_PORT, ports[i], ports[i], &ftp_exp_policy,
-				  0, help, nf_ct_ftp_from_nlattr, THIS_MODULE);
+				  0, help, nf_ct_ftp_from_nlattr, THIS_MODULE,
+				  NF_CT_NAT_HELPER_MOD_NAME("ftp"));
 		nf_ct_helper_init(&ftp[2 * i + 1], AF_INET6, IPPROTO_TCP, "ftp",
 				  FTP_PORT, ports[i], ports[i], &ftp_exp_policy,
-				  0, help, nf_ct_ftp_from_nlattr, THIS_MODULE);
+				  0, help, nf_ct_ftp_from_nlattr, THIS_MODULE,
+				  NF_CT_NAT_HELPER_MOD_NAME("ftp"));
 	}
 
 	ret = nf_conntrack_helpers_register(ftp, ports_c * 2);
diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index 274baf1dab87..883a8d438503 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -42,6 +42,9 @@ module_param_named(nf_conntrack_helper, nf_ct_auto_assign_helper, bool, 0644);
 MODULE_PARM_DESC(nf_conntrack_helper,
 		 "Enable automatic conntrack helper assignment (default 0)");
 
+static struct list_head nf_ct_nat_helpers __read_mostly;
+static DEFINE_SPINLOCK(nf_ct_nat_helpers_lock);
+
 /* Stupid hash, but collision free for the default registrations of the
  * helpers currently in the kernel. */
 static unsigned int helper_hash(const struct nf_conntrack_tuple *tuple)
@@ -130,6 +133,75 @@ void nf_conntrack_helper_put(struct nf_conntrack_helper *helper)
 }
 EXPORT_SYMBOL_GPL(nf_conntrack_helper_put);
 
+static struct nf_conntrack_helper_nat *
+nf_conntrack_helper_nat_find(const char *name)
+{
+	struct nf_conntrack_helper_nat *cur;
+	bool found = false;
+
+	list_for_each_entry_rcu(cur, &nf_ct_nat_helpers, list) {
+		if (!strcmp(cur->name, name)) {
+			found = true;
+			break;
+		}
+	}
+	return found ? cur : NULL;
+}
+
+int
+nf_conntrack_helper_nat_try_module_get(const char *name, u16 l3num, u8 protonum)
+{
+	struct nf_conntrack_helper *h;
+	struct nf_conntrack_helper_nat *nat;
+	char mod_name[NF_CT_HELPER_NAME_LEN];
+	int ret = 0;
+
+	rcu_read_lock();
+	h = __nf_conntrack_helper_find(name, l3num, protonum);
+	if (h == NULL) {
+		rcu_read_unlock();
+		return -EINVAL;
+	}
+
+	if (!strlen(h->nat_mod_name)) {
+		rcu_read_unlock();
+		return -EOPNOTSUPP;
+	}
+
+	nat = nf_conntrack_helper_nat_find(h->nat_mod_name);
+	if (nat == NULL) {
+		snprintf(mod_name, sizeof(mod_name), "%s", h->nat_mod_name);
+		rcu_read_unlock();
+		ret = request_module(mod_name);
+		if (ret != 0)
+			return ret;
+
+		rcu_read_lock();
+		nat = nf_conntrack_helper_nat_find(mod_name);
+		if (nat == NULL) {
+			rcu_read_unlock();
+			return -EINVAL;
+		}
+	}
+
+	if (!try_module_get(nat->module))
+		ret = -EINVAL;
+
+	rcu_read_unlock();
+	return ret;
+}
+EXPORT_SYMBOL_GPL(nf_conntrack_helper_nat_try_module_get);
+
+void nf_conntrack_helper_nat_put(struct nf_conntrack_helper *helper)
+{
+	struct nf_conntrack_helper_nat *nat;
+
+	nat = nf_conntrack_helper_nat_find(helper->nat_mod_name);
+	BUG_ON(nat == NULL);
+	module_put(nat->module);
+}
+EXPORT_SYMBOL_GPL(nf_conntrack_helper_nat_put);
+
 struct nf_conn_help *
 nf_ct_helper_ext_add(struct nf_conn *ct, gfp_t gfp)
 {
@@ -420,7 +492,8 @@ void nf_ct_helper_init(struct nf_conntrack_helper *helper,
 				   enum ip_conntrack_info ctinfo),
 		       int (*from_nlattr)(struct nlattr *attr,
 					  struct nf_conn *ct),
-		       struct module *module)
+		       struct module *module,
+		       const char *nat_mod_name)
 {
 	helper->tuple.src.l3num = l3num;
 	helper->tuple.dst.protonum = protonum;
@@ -430,6 +503,10 @@ void nf_ct_helper_init(struct nf_conntrack_helper *helper,
 	helper->help = help;
 	helper->from_nlattr = from_nlattr;
 	helper->me = module;
+	helper->nat_mod_name[0] = '\0';
+	if (nat_mod_name)
+		snprintf(helper->nat_mod_name, sizeof(helper->nat_mod_name),
+			 "%s", nat_mod_name);
 
 	if (spec_port == default_port)
 		snprintf(helper->name, sizeof(helper->name), "%s", name);
@@ -466,6 +543,34 @@ void nf_conntrack_helpers_unregister(struct nf_conntrack_helper *helper,
 }
 EXPORT_SYMBOL_GPL(nf_conntrack_helpers_unregister);
 
+void nf_conntrack_helper_nat_register(struct nf_conntrack_helper_nat *nat)
+{
+	BUG_ON(nat->module == NULL);
+
+	spin_lock(&nf_ct_nat_helpers_lock);
+	list_add_rcu(&nat->list, &nf_ct_nat_helpers);
+	spin_unlock(&nf_ct_nat_helpers_lock);
+}
+EXPORT_SYMBOL_GPL(nf_conntrack_helper_nat_register);
+
+void nf_conntrack_helper_nat_unregister(struct nf_conntrack_helper_nat *nat)
+{
+	BUG_ON(nat->module == NULL);
+
+	spin_lock(&nf_ct_nat_helpers_lock);
+	list_del_rcu(&nat->list);
+	spin_unlock(&nf_ct_nat_helpers_lock);
+}
+EXPORT_SYMBOL_GPL(nf_conntrack_helper_nat_unregister);
+
+void nf_ct_helper_nat_init(struct nf_conntrack_helper_nat *nat,
+			   const char *name, struct module *module)
+{
+	nat->module = module;
+	snprintf(nat->name, sizeof(nat->name), "%s", name);
+}
+EXPORT_SYMBOL_GPL(nf_ct_helper_nat_init);
+
 static const struct nf_ct_ext_type helper_extend = {
 	.len	= sizeof(struct nf_conn_help),
 	.align	= __alignof__(struct nf_conn_help),
@@ -493,6 +598,7 @@ int nf_conntrack_helper_init(void)
 		goto out_extend;
 	}
 
+	INIT_LIST_HEAD(&nf_ct_nat_helpers);
 	return 0;
 out_extend:
 	kvfree(nf_ct_helper_hash);
diff --git a/net/netfilter/nf_conntrack_irc.c b/net/netfilter/nf_conntrack_irc.c
index 4099f4d79bae..659aa2cb5493 100644
--- a/net/netfilter/nf_conntrack_irc.c
+++ b/net/netfilter/nf_conntrack_irc.c
@@ -261,7 +261,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], i, &irc_exp_policy,
-				  0, help, NULL, THIS_MODULE);
+				  0, help, NULL, THIS_MODULE,
+				  NF_CT_NAT_HELPER_MOD_NAME("irc"));
 	}
 
 	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 5072ff96ab33..b08724b8754c 100644
--- a/net/netfilter/nf_conntrack_sane.c
+++ b/net/netfilter/nf_conntrack_sane.c
@@ -198,11 +198,11 @@ static int __init nf_conntrack_sane_init(void)
 		nf_ct_helper_init(&sane[2 * i], AF_INET, IPPROTO_TCP, "sane",
 				  SANE_PORT, ports[i], ports[i],
 				  &sane_exp_policy, 0, help, NULL,
-				  THIS_MODULE);
+				  THIS_MODULE, NULL);
 		nf_ct_helper_init(&sane[2 * i + 1], AF_INET6, IPPROTO_TCP, "sane",
 				  SANE_PORT, ports[i], ports[i],
 				  &sane_exp_policy, 0, help, NULL,
-				  THIS_MODULE);
+				  THIS_MODULE, NULL);
 	}
 
 	ret = nf_conntrack_helpers_register(sane, ports_c * 2);
diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
index f067c6b50857..0d4fca4a329f 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c
@@ -1677,19 +1677,23 @@ static int __init nf_conntrack_sip_init(void)
 		nf_ct_helper_init(&sip[4 * i], AF_INET, IPPROTO_UDP, "sip",
 				  SIP_PORT, ports[i], i, sip_exp_policy,
 				  SIP_EXPECT_MAX, sip_help_udp,
-				  NULL, THIS_MODULE);
+				  NULL, THIS_MODULE,
+				  NF_CT_NAT_HELPER_MOD_NAME("sip"));
 		nf_ct_helper_init(&sip[4 * i + 1], AF_INET, IPPROTO_TCP, "sip",
 				  SIP_PORT, ports[i], i, sip_exp_policy,
 				  SIP_EXPECT_MAX, sip_help_tcp,
-				  NULL, THIS_MODULE);
+				  NULL, THIS_MODULE,
+				  NF_CT_NAT_HELPER_MOD_NAME("sip"));
 		nf_ct_helper_init(&sip[4 * i + 2], AF_INET6, IPPROTO_UDP, "sip",
 				  SIP_PORT, ports[i], i, sip_exp_policy,
 				  SIP_EXPECT_MAX, sip_help_udp,
-				  NULL, THIS_MODULE);
+				  NULL, THIS_MODULE,
+				  NF_CT_NAT_HELPER_MOD_NAME("sip"));
 		nf_ct_helper_init(&sip[4 * i + 3], AF_INET6, IPPROTO_TCP, "sip",
 				  SIP_PORT, ports[i], i, sip_exp_policy,
 				  SIP_EXPECT_MAX, sip_help_tcp,
-				  NULL, THIS_MODULE);
+				  NULL, THIS_MODULE,
+				  NF_CT_NAT_HELPER_MOD_NAME("sip"));
 	}
 
 	ret = nf_conntrack_helpers_register(sip, ports_c * 4);
diff --git a/net/netfilter/nf_conntrack_tftp.c b/net/netfilter/nf_conntrack_tftp.c
index 548b673b3625..e1fbf892db70 100644
--- a/net/netfilter/nf_conntrack_tftp.c
+++ b/net/netfilter/nf_conntrack_tftp.c
@@ -121,10 +121,12 @@ 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], i, &tftp_exp_policy,
-				  0, tftp_help, NULL, THIS_MODULE);
+				  0, tftp_help, NULL, THIS_MODULE,
+				  NF_CT_NAT_HELPER_MOD_NAME("tftp"));
 		nf_ct_helper_init(&tftp[2 * i + 1], AF_INET6, IPPROTO_UDP, "tftp",
 				  TFTP_PORT, ports[i], i, &tftp_exp_policy,
-				  0, tftp_help, NULL, THIS_MODULE);
+				  0, tftp_help, NULL, THIS_MODULE,
+				  NF_CT_NAT_HELPER_MOD_NAME("tftp"));
 	}
 
 	ret = nf_conntrack_helpers_register(tftp, ports_c * 2);
-- 
2.20.1

[PATCH net-next 3/8] netfilter: nf_nat: register amanda NAT helper.

[Flavio Leitner]

Signed-off-by: Flavio Leitner <fbl@sysclose.org>
---
 net/netfilter/nf_nat_amanda.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/netfilter/nf_nat_amanda.c b/net/netfilter/nf_nat_amanda.c
index e87075763f73..344096418224 100644
--- a/net/netfilter/nf_nat_amanda.c
+++ b/net/netfilter/nf_nat_amanda.c
@@ -24,6 +24,8 @@ MODULE_DESCRIPTION("Amanda NAT helper");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS_NFCT_HELPER_NAT("amanda");
 
+static struct nf_conntrack_helper_nat helper_nat_amanda;
+
 static unsigned int help(struct sk_buff *skb,
 			 enum ip_conntrack_info ctinfo,
 			 unsigned int protoff,
@@ -74,6 +76,7 @@ static unsigned int help(struct sk_buff *skb,
 
 static void __exit nf_nat_amanda_fini(void)
 {
+	nf_conntrack_helper_nat_unregister(&helper_nat_amanda);
 	RCU_INIT_POINTER(nf_nat_amanda_hook, NULL);
 	synchronize_rcu();
 }
@@ -81,6 +84,10 @@ static void __exit nf_nat_amanda_fini(void)
 static int __init nf_nat_amanda_init(void)
 {
 	BUG_ON(nf_nat_amanda_hook != NULL);
+	nf_ct_helper_nat_init(&helper_nat_amanda,
+			      NF_CT_NAT_HELPER_MOD_NAME("amanda"),
+			      THIS_MODULE);
+	nf_conntrack_helper_nat_register(&helper_nat_amanda);
 	RCU_INIT_POINTER(nf_nat_amanda_hook, help);
 	return 0;
 }
-- 
2.20.1

[PATCH net-next 4/8] netfilter: nf_nat: register ftp NAT helper.

[Flavio Leitner]

Signed-off-by: Flavio Leitner <fbl@sysclose.org>
---
 net/netfilter/nf_nat_ftp.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/netfilter/nf_nat_ftp.c b/net/netfilter/nf_nat_ftp.c
index 19f5739fd5e2..70fddcddad54 100644
--- a/net/netfilter/nf_nat_ftp.c
+++ b/net/netfilter/nf_nat_ftp.c
@@ -28,6 +28,8 @@ MODULE_ALIAS_NFCT_HELPER_NAT("ftp");
 
 /* FIXME: Time out? --RR */
 
+static struct nf_conntrack_helper_nat helper_nat_ftp;
+
 static int nf_nat_ftp_fmt_cmd(struct nf_conn *ct, enum nf_ct_ftp_type type,
 			      char *buffer, size_t buflen,
 			      union nf_inet_addr *addr, u16 port)
@@ -124,6 +126,7 @@ static unsigned int nf_nat_ftp(struct sk_buff *skb,
 
 static void __exit nf_nat_ftp_fini(void)
 {
+	nf_conntrack_helper_nat_unregister(&helper_nat_ftp);
 	RCU_INIT_POINTER(nf_nat_ftp_hook, NULL);
 	synchronize_rcu();
 }
@@ -131,6 +134,9 @@ static void __exit nf_nat_ftp_fini(void)
 static int __init nf_nat_ftp_init(void)
 {
 	BUG_ON(nf_nat_ftp_hook != NULL);
+	nf_ct_helper_nat_init(&helper_nat_ftp,
+			      NF_CT_NAT_HELPER_MOD_NAME("ftp"), THIS_MODULE);
+	nf_conntrack_helper_nat_register(&helper_nat_ftp);
 	RCU_INIT_POINTER(nf_nat_ftp_hook, nf_nat_ftp);
 	return 0;
 }
-- 
2.20.1

[PATCH net-next 5/8] netfilter: nf_nat: register irc NAT helper.

[Flavio Leitner]

Signed-off-by: Flavio Leitner <fbl@sysclose.org>
---
 net/netfilter/nf_nat_irc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/netfilter/nf_nat_irc.c b/net/netfilter/nf_nat_irc.c
index c18e3ce6589b..853e91c1cea5 100644
--- a/net/netfilter/nf_nat_irc.c
+++ b/net/netfilter/nf_nat_irc.c
@@ -28,6 +28,8 @@ MODULE_DESCRIPTION("IRC (DCC) NAT helper");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS_NFCT_HELPER_NAT("irc");
 
+static struct nf_conntrack_helper_nat helper_nat_irc;
+
 static unsigned int help(struct sk_buff *skb,
 			 enum ip_conntrack_info ctinfo,
 			 unsigned int protoff,
@@ -96,6 +98,7 @@ static unsigned int help(struct sk_buff *skb,
 
 static void __exit nf_nat_irc_fini(void)
 {
+	nf_conntrack_helper_nat_unregister(&helper_nat_irc);
 	RCU_INIT_POINTER(nf_nat_irc_hook, NULL);
 	synchronize_rcu();
 }
@@ -103,6 +106,9 @@ static void __exit nf_nat_irc_fini(void)
 static int __init nf_nat_irc_init(void)
 {
 	BUG_ON(nf_nat_irc_hook != NULL);
+	nf_ct_helper_nat_init(&helper_nat_irc,
+			      NF_CT_NAT_HELPER_MOD_NAME("irc"), THIS_MODULE);
+	nf_conntrack_helper_nat_register(&helper_nat_irc);
 	RCU_INIT_POINTER(nf_nat_irc_hook, help);
 	return 0;
 }
-- 
2.20.1

[PATCH net-next 6/8] netfilter: nf_nat: register sip NAT helper.

[Flavio Leitner]

Signed-off-by: Flavio Leitner <fbl@sysclose.org>
---
 net/netfilter/nf_nat_sip.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nf_nat_sip.c b/net/netfilter/nf_nat_sip.c
index f31c2a1b95b8..42b3d2e7ecbd 100644
--- a/net/netfilter/nf_nat_sip.c
+++ b/net/netfilter/nf_nat_sip.c
@@ -29,6 +29,7 @@ MODULE_AUTHOR("Christian Hentschel <chentschel@arnet.com.ar>");
 MODULE_DESCRIPTION("SIP NAT helper");
 MODULE_ALIAS_NFCT_HELPER_NAT("sip");
 
+static struct nf_conntrack_helper_nat helper_nat_sip;
 
 static unsigned int mangle_packet(struct sk_buff *skb, unsigned int protoff,
 				  unsigned int dataoff,
@@ -656,8 +657,8 @@ static struct nf_ct_helper_expectfn sip_nat = {
 
 static void __exit nf_nat_sip_fini(void)
 {
+	nf_conntrack_helper_nat_unregister(&helper_nat_sip);
 	RCU_INIT_POINTER(nf_nat_sip_hooks, NULL);
-
 	nf_ct_helper_expectfn_unregister(&sip_nat);
 	synchronize_rcu();
 }
@@ -675,6 +676,9 @@ static const struct nf_nat_sip_hooks sip_hooks = {
 static int __init nf_nat_sip_init(void)
 {
 	BUG_ON(nf_nat_sip_hooks != NULL);
+	nf_ct_helper_nat_init(&helper_nat_sip,
+			      NF_CT_NAT_HELPER_MOD_NAME("sip"), THIS_MODULE);
+	nf_conntrack_helper_nat_register(&helper_nat_sip);
 	RCU_INIT_POINTER(nf_nat_sip_hooks, &sip_hooks);
 	nf_ct_helper_expectfn_register(&sip_nat);
 	return 0;
-- 
2.20.1

[PATCH net-next 7/8] netfilter: nf_nat: register tftp NAT helper.

[Flavio Leitner]

Signed-off-by: Flavio Leitner <fbl@sysclose.org>
---
 net/netfilter/nf_nat_tftp.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/netfilter/nf_nat_tftp.c b/net/netfilter/nf_nat_tftp.c
index 51673aa6e1dc..5a7af30e3e02 100644
--- a/net/netfilter/nf_nat_tftp.c
+++ b/net/netfilter/nf_nat_tftp.c
@@ -18,6 +18,8 @@ MODULE_DESCRIPTION("TFTP NAT helper");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS_NFCT_HELPER_NAT("tftp");
 
+static struct nf_conntrack_helper_nat helper_nat_tftp;
+
 static unsigned int help(struct sk_buff *skb,
 			 enum ip_conntrack_info ctinfo,
 			 struct nf_conntrack_expect *exp)
@@ -37,6 +39,7 @@ static unsigned int help(struct sk_buff *skb,
 
 static void __exit nf_nat_tftp_fini(void)
 {
+	nf_conntrack_helper_nat_unregister(&helper_nat_tftp);
 	RCU_INIT_POINTER(nf_nat_tftp_hook, NULL);
 	synchronize_rcu();
 }
@@ -44,6 +47,9 @@ static void __exit nf_nat_tftp_fini(void)
 static int __init nf_nat_tftp_init(void)
 {
 	BUG_ON(nf_nat_tftp_hook != NULL);
+	nf_ct_helper_nat_init(&helper_nat_tftp,
+			      NF_CT_NAT_HELPER_MOD_NAME("tftp"), THIS_MODULE);
+	nf_conntrack_helper_nat_register(&helper_nat_tftp);
 	RCU_INIT_POINTER(nf_nat_tftp_hook, help);
 	return 0;
 }
-- 
2.20.1

[PATCH net-next 8/8] openvswitch: load and reference the NAT helper.

[Flavio Leitner]

This improves the original commit 17c357efe5ec ("openvswitch: load
NAT helper") where it unconditionally tries to load the module for
every flow using NAT, so not efficient when loading multiple flows.
It also doesn't hold any references to the NAT module while the
flow is active.

This change fixes those problems. It will try to load the module
only if it's not present. It grabs a reference to the NAT module
and holds it while the flow is active. Finally, an error message
shows up if either actions above fails.

Fixes: 17c357efe5ec ("openvswitch: load NAT helper")
Signed-off-by: Flavio Leitner <fbl@sysclose.org>
---
 net/openvswitch/conntrack.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 845b83598e0d..fb58637a27c9 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -1305,6 +1305,7 @@ static int ovs_ct_add_helper(struct ovs_conntrack_info *info, const char *name,
 {
 	struct nf_conntrack_helper *helper;
 	struct nf_conn_help *help;
+	int ret = 0;
 
 	helper = nf_conntrack_helper_try_module_get(name, info->family,
 						    key->ip.proto);
@@ -1319,13 +1320,22 @@ static int ovs_ct_add_helper(struct ovs_conntrack_info *info, const char *name,
 		return -ENOMEM;
 	}
 
+#ifdef CONFIG_NF_NAT_NEEDED
+	if (info->nat) {
+		ret = nf_conntrack_helper_nat_try_module_get(name,
+							     info->family,
+							     key->ip.proto);
+		if (ret) {
+			nf_conntrack_helper_put(helper);
+			OVS_NLERR(log, "Failed to load \"%s\" NAT helper, err: %d",
+				  name, ret);
+			return ret;
+		}
+	}
+#endif
 	rcu_assign_pointer(help->helper, helper);
 	info->helper = helper;
-
-	if (info->nat)
-		request_module("ip_nat_%s", name);
-
-	return 0;
+	return ret;
 }
 
 #ifdef CONFIG_NF_NAT_NEEDED
@@ -1776,8 +1786,13 @@ void ovs_ct_free_action(const struct nlattr *a)
 
 static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info)
 {
-	if (ct_info->helper)
+	if (ct_info->helper) {
+#ifdef CONFIG_NF_NAT_NEEDED
+		if (ct_info->nat)
+			nf_conntrack_helper_nat_put(ct_info->helper);
+#endif
 		nf_conntrack_helper_put(ct_info->helper);
+	}
 	if (ct_info->ct)
 		nf_ct_tmpl_free(ct_info->ct);
 }
-- 
2.20.1

Re: [PATCH net-next 0/8] openvswitch: load and reference the NAT helper.

[David Miller]

From: Flavio Leitner <fbl@sysclose.org>
Date: Tue, 26 Mar 2019 17:57:07 -0300

> The request_module() is quite expensive and triggers the
> usermode helper in userspace. Instead, load only if the
> module is not present and keep module references to avoid
> problems.
> 
> The first patch standardize the module alias which is already
> there, but not in a formal way.
> 
> The second patch adds an API to point to the NAT helper.
> 
> The following patches will register each NAT helper using
> the new API.
> 
> The last patch fixes openvswitch to use the new API to
> load and reference the NAT helper and also report an error
> if the operation fails.

These are mostly netfilter changes so I would like to see some
reviews/ACKs from netfilter folks.

Thanks.

Re: [PATCH net-next 0/8] openvswitch: load and reference the NAT helper.

[David Miller]

From: Flavio Leitner <fbl@sysclose.org>
Date: Tue, 26 Mar 2019 17:57:07 -0300

> The request_module() is quite expensive and triggers the
> usermode helper in userspace. Instead, load only if the
> module is not present and keep module references to avoid
> problems.
> 
> The first patch standardize the module alias which is already
> there, but not in a formal way.
> 
> The second patch adds an API to point to the NAT helper.
> 
> The following patches will register each NAT helper using
> the new API.
> 
> The last patch fixes openvswitch to use the new API to
> load and reference the NAT helper and also report an error
> if the operation fails.

I haven't seen any netfilter reviews of this series, so I'm dropping
it.

Re: [PATCH net-next 1/8] netfilter: use macros to create module aliases.

[Pablo Neira Ayuso]

On Tue, Mar 26, 2019 at 05:57:08PM -0300, Flavio Leitner wrote:
> Each NAT helper creates a module alias which follows a pattern.
> Use macros for consistency.
> 
> Signed-off-by: Flavio Leitner <fbl@sysclose.org>
> ---
>  include/net/netfilter/nf_conntrack_helper.h | 4 ++++
>  net/ipv4/netfilter/nf_nat_h323.c            | 2 +-
>  net/ipv4/netfilter/nf_nat_pptp.c            | 2 +-
>  net/netfilter/nf_nat_amanda.c               | 2 +-
>  net/netfilter/nf_nat_ftp.c                  | 2 +-
>  net/netfilter/nf_nat_irc.c                  | 2 +-
>  net/netfilter/nf_nat_sip.c                  | 2 +-
>  net/netfilter/nf_nat_tftp.c                 | 2 +-
>  8 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
> index ec52a8dc32fd..e86fadf7e7c5 100644
> --- a/include/net/netfilter/nf_conntrack_helper.h
> +++ b/include/net/netfilter/nf_conntrack_helper.h
> @@ -15,6 +15,10 @@
>  #include <net/netfilter/nf_conntrack_extend.h>
>  #include <net/netfilter/nf_conntrack_expect.h>
>  
> +#define NF_CT_NAT_HELPER_MOD_NAME(name)	"ip_nat_" name

I'd suggest a rename from NF_CT_NAT_HELPER_MOD_NAME() to
NF_NAT_HELPER_NAME().

Please, also use "nf_nat_" prefix instead, "ip_nat" is legacy stuff.

> +#define MODULE_ALIAS_NFCT_HELPER_NAT(name) \
> +	MODULE_ALIAS(NF_CT_NAT_HELPER_MOD_NAME(name))

Probably:

        MODULE_ALIAS_NF_NAT_HELPER

instead of MODULE_ALIAS_NFCT_HELPER_NAT.

Re: [PATCH net-next 2/8] netfilter: add API to manage NAT helpers.

[Pablo Neira Ayuso]

On Tue, Mar 26, 2019 at 05:57:09PM -0300, Flavio Leitner wrote:
> The API allows a conntrack helper to indicate its corresponding
> NAT helper which then can be loaded and reference counted.
> 
> Signed-off-by: Flavio Leitner <fbl@sysclose.org>
> ---
>  include/net/netfilter/nf_conntrack_helper.h |  19 +++-
>  net/netfilter/nf_conntrack_amanda.c         |   2 +
>  net/netfilter/nf_conntrack_ftp.c            |   6 +-
>  net/netfilter/nf_conntrack_helper.c         | 108 +++++++++++++++++++-
>  net/netfilter/nf_conntrack_irc.c            |   3 +-
>  net/netfilter/nf_conntrack_sane.c           |   4 +-
>  net/netfilter/nf_conntrack_sip.c            |  12 ++-
>  net/netfilter/nf_conntrack_tftp.c           |   6 +-
>  8 files changed, 147 insertions(+), 13 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
> index e86fadf7e7c5..0d36d6bfb522 100644
> --- a/include/net/netfilter/nf_conntrack_helper.h
> +++ b/include/net/netfilter/nf_conntrack_helper.h
> @@ -58,6 +58,8 @@ struct nf_conntrack_helper {
>  	unsigned int queue_num;
>  	/* length of userspace private data stored in nf_conn_help->data */
>  	u16 data_len;
> +	/* name of NAT helper module */
> +	char nat_mod_name[NF_CT_HELPER_NAME_LEN];
>  };
>  
>  /* Must be kept in sync with the classes defined by helpers */
> @@ -98,7 +100,8 @@ void nf_ct_helper_init(struct nf_conntrack_helper *helper,
>  				   enum ip_conntrack_info ctinfo),
>  		       int (*from_nlattr)(struct nlattr *attr,
>  					  struct nf_conn *ct),
> -		       struct module *module);
> +		       struct module *module,
> +		       const char *nat_mod_name);
>  
>  int nf_conntrack_helper_register(struct nf_conntrack_helper *);
>  void nf_conntrack_helper_unregister(struct nf_conntrack_helper *);
> @@ -157,4 +160,18 @@ nf_ct_helper_expectfn_find_by_symbol(const void *symbol);
>  extern struct hlist_head *nf_ct_helper_hash;
>  extern unsigned int nf_ct_helper_hsize;
>  
> +struct nf_conntrack_helper_nat {
> +	struct list_head list;
> +	char name[NF_CT_HELPER_NAME_LEN];
> +	struct module *module;		/* pointer to self */
> +};
> +
> +void nf_ct_helper_nat_init(struct nf_conntrack_helper_nat *nat,
> +			   const char *name, struct module *module);

Instead of this nf_ct_helper_nat_init() runtime initializer, define
the structure in C99 as static in the NAT helper module?

Telling this because we can probably also extend this structure to
remove the RCU hook between ct helper and nat helper at some point
through this new definition.

> +void nf_conntrack_helper_nat_register(struct nf_conntrack_helper_nat *nat);

Shorter name suggestion:

        nf_nat_helper_register()

> +void nf_conntrack_helper_nat_unregister(struct nf_conntrack_helper_nat *nat);

        nf_nat_helper_unregister()

> +int nf_conntrack_helper_nat_try_module_get(const char *name, u16 l3num,
> +					   u8 protonum);

        nf_nat_helper_try_module_get()

> +void nf_conntrack_helper_nat_put(struct nf_conntrack_helper *helper);

        nf_nat_helper_nat_put()

Re: [PATCH net-next 2/8] netfilter: add API to manage NAT helpers.

[Pablo Neira Ayuso]

On Tue, Mar 26, 2019 at 05:57:09PM -0300, Flavio Leitner wrote:
[...]
> diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
> index 274baf1dab87..883a8d438503 100644
> --- a/net/netfilter/nf_conntrack_helper.c
> +++ b/net/netfilter/nf_conntrack_helper.c
> @@ -42,6 +42,9 @@ module_param_named(nf_conntrack_helper, nf_ct_auto_assign_helper, bool, 0644);
>  MODULE_PARM_DESC(nf_conntrack_helper,
>  		 "Enable automatic conntrack helper assignment (default 0)");
>  
> +static struct list_head nf_ct_nat_helpers __read_mostly;
> +static DEFINE_SPINLOCK(nf_ct_nat_helpers_lock);

Why a spinlock here? Use mutex instead I'd suggest.

Thanks.

Re: [PATCH net-next 1/8] netfilter: use macros to create module aliases.

[Flavio Leitner]

On Mon, Apr 01, 2019 at 12:07:43AM +0200, Pablo Neira Ayuso wrote:
> On Tue, Mar 26, 2019 at 05:57:08PM -0300, Flavio Leitner wrote:
> > Each NAT helper creates a module alias which follows a pattern.
> > Use macros for consistency.
> > 
> > Signed-off-by: Flavio Leitner <fbl@sysclose.org>
> > ---
> >  include/net/netfilter/nf_conntrack_helper.h | 4 ++++
> >  net/ipv4/netfilter/nf_nat_h323.c            | 2 +-
> >  net/ipv4/netfilter/nf_nat_pptp.c            | 2 +-
> >  net/netfilter/nf_nat_amanda.c               | 2 +-
> >  net/netfilter/nf_nat_ftp.c                  | 2 +-
> >  net/netfilter/nf_nat_irc.c                  | 2 +-
> >  net/netfilter/nf_nat_sip.c                  | 2 +-
> >  net/netfilter/nf_nat_tftp.c                 | 2 +-
> >  8 files changed, 11 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
> > index ec52a8dc32fd..e86fadf7e7c5 100644
> > --- a/include/net/netfilter/nf_conntrack_helper.h
> > +++ b/include/net/netfilter/nf_conntrack_helper.h
> > @@ -15,6 +15,10 @@
> >  #include <net/netfilter/nf_conntrack_extend.h>
> >  #include <net/netfilter/nf_conntrack_expect.h>
> >  
> > +#define NF_CT_NAT_HELPER_MOD_NAME(name)	"ip_nat_" name
> 
> I'd suggest a rename from NF_CT_NAT_HELPER_MOD_NAME() to
> NF_NAT_HELPER_NAME().

OK.


> Please, also use "nf_nat_" prefix instead, "ip_nat" is legacy stuff.

I don't think we can, because people might be using
the current alias which is ip_nat_something. This patch
is just making it more robust.

> > +#define MODULE_ALIAS_NFCT_HELPER_NAT(name) \
> > +	MODULE_ALIAS(NF_CT_NAT_HELPER_MOD_NAME(name))
> 
> Probably:
> 
>         MODULE_ALIAS_NF_NAT_HELPER
> 
> instead of MODULE_ALIAS_NFCT_HELPER_NAT.

OK.

fbl

Re: [PATCH net-next 2/8] netfilter: add API to manage NAT helpers.

[Flavio Leitner]

On Mon, Apr 01, 2019 at 12:10:32AM +0200, Pablo Neira Ayuso wrote:
> On Tue, Mar 26, 2019 at 05:57:09PM -0300, Flavio Leitner wrote:
> > The API allows a conntrack helper to indicate its corresponding
> > NAT helper which then can be loaded and reference counted.
> > 
> > Signed-off-by: Flavio Leitner <fbl@sysclose.org>
> > ---
> >  include/net/netfilter/nf_conntrack_helper.h |  19 +++-
> >  net/netfilter/nf_conntrack_amanda.c         |   2 +
> >  net/netfilter/nf_conntrack_ftp.c            |   6 +-
> >  net/netfilter/nf_conntrack_helper.c         | 108 +++++++++++++++++++-
> >  net/netfilter/nf_conntrack_irc.c            |   3 +-
> >  net/netfilter/nf_conntrack_sane.c           |   4 +-
> >  net/netfilter/nf_conntrack_sip.c            |  12 ++-
> >  net/netfilter/nf_conntrack_tftp.c           |   6 +-
> >  8 files changed, 147 insertions(+), 13 deletions(-)
> > 
> > diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
> > index e86fadf7e7c5..0d36d6bfb522 100644
> > --- a/include/net/netfilter/nf_conntrack_helper.h
> > +++ b/include/net/netfilter/nf_conntrack_helper.h
> > @@ -58,6 +58,8 @@ struct nf_conntrack_helper {
> >  	unsigned int queue_num;
> >  	/* length of userspace private data stored in nf_conn_help->data */
> >  	u16 data_len;
> > +	/* name of NAT helper module */
> > +	char nat_mod_name[NF_CT_HELPER_NAME_LEN];
> >  };
> >  
> >  /* Must be kept in sync with the classes defined by helpers */
> > @@ -98,7 +100,8 @@ void nf_ct_helper_init(struct nf_conntrack_helper *helper,
> >  				   enum ip_conntrack_info ctinfo),
> >  		       int (*from_nlattr)(struct nlattr *attr,
> >  					  struct nf_conn *ct),
> > -		       struct module *module);
> > +		       struct module *module,
> > +		       const char *nat_mod_name);
> >  
> >  int nf_conntrack_helper_register(struct nf_conntrack_helper *);
> >  void nf_conntrack_helper_unregister(struct nf_conntrack_helper *);
> > @@ -157,4 +160,18 @@ nf_ct_helper_expectfn_find_by_symbol(const void *symbol);
> >  extern struct hlist_head *nf_ct_helper_hash;
> >  extern unsigned int nf_ct_helper_hsize;
> >  
> > +struct nf_conntrack_helper_nat {
> > +	struct list_head list;
> > +	char name[NF_CT_HELPER_NAME_LEN];
> > +	struct module *module;		/* pointer to self */
> > +};
> > +
> > +void nf_ct_helper_nat_init(struct nf_conntrack_helper_nat *nat,
> > +			   const char *name, struct module *module);
> 
> Instead of this nf_ct_helper_nat_init() runtime initializer, define
> the structure in C99 as static in the NAT helper module?
> 
> Telling this because we can probably also extend this structure to
> remove the RCU hook between ct helper and nat helper at some point
> through this new definition.

Sounds good, let me try that.


> > +void nf_conntrack_helper_nat_register(struct nf_conntrack_helper_nat *nat);
> 
> Shorter name suggestion:
> 
>         nf_nat_helper_register()
> 
> > +void nf_conntrack_helper_nat_unregister(struct nf_conntrack_helper_nat *nat);
> 
>         nf_nat_helper_unregister()
> 
> > +int nf_conntrack_helper_nat_try_module_get(const char *name, u16 l3num,
> > +					   u8 protonum);
> 
>         nf_nat_helper_try_module_get()
> 
> > +void nf_conntrack_helper_nat_put(struct nf_conntrack_helper *helper);
> 
>         nf_nat_helper_nat_put()

Ok to all the above.

Thanks,
fbl