netfilter: nfnetlink_queue: get rid of nfnetlink_queue_ct.c
Subsystems (2)
Pablo Neira Ayuso
maintainerPATCH v3 1/2 netfilter: nfnetlink_queue: get rid of nfnetlink_queue_ct.c
The original intention was to avoid dependencies between nfnetlink_queue and conntrack without ifdef pollution. However, we can achieve this by moving the conntrack dependent code into ctnetlink and keep some glue code to access the nfq_ct indirection from nfqueue.
After this patch, the nfq_ct indirection is always compiled in the netfilter core to avoid polluting nfqueue with ifdefs. Thus, if nf_conntrack is not compiled this results in only 8-bytes of memory waste in x86_64.
This patch also adds ctnetlink_nfqueue_seqadj() to avoid that the nf_conn structure layout if exposed to nf_queue, which creates another dependency with nf_conntrack at compilation time.
---
v3: fix typo when rebasing without the patch to consolidate the netlink message
size calculation.
include/linux/netfilter.h | 12 ++-- include/net/netfilter/nfnetlink_queue.h | 51 -------------- net/netfilter/Makefile | 1 - net/netfilter/core.c | 9 ++- net/netfilter/nf_conntrack_netlink.c | 52 ++++++++++++++- net/netfilter/nfnetlink_queue_core.c | 52 +++++++++++---- net/netfilter/nfnetlink_queue_ct.c | 113 -------------------------------- 7 files changed, 103 insertions(+), 187 deletions(-) delete mode 100644 include/net/netfilter/nfnetlink_queue.h delete mode 100644 net/netfilter/nfnetlink_queue_ct.c
include/linux/netfilter.h +8 -4
@@ -369,14 +369,21 @@ nf_nat_decode_session(struct sk_buff *skb, struct flowi *fl, u_int8_t family)extern void (*ip_ct_attach)(struct sk_buff *,const struct sk_buff *)__rcu ;void nf_ct_attach (struct sk_buff *,const struct sk_buff *);extern void (*nf_ct_destroy)(struct nf_conntrack *)__rcu ; +#else +static inline void nf_ct_attach (struct sk_buff *new,struct sk_buff *skb) {} +#endif struct nf_conn ;enum ip_conntrack_info ;struct nlattr ;struct nfq_ct_hook { +struct nf_conn *(*get_ct)(struct sk_buff *skb, +enum ip_conntrack_info *ctinfo);size_t (*build_size)(const struct nf_conn *ct); -int (*build)(struct sk_buff *skb,struct nf_conn *ct); +int (*build)(struct sk_buff *skb,struct nf_conn *ct, +enum ip_conntrack_info ctinfo, +u_int16_t ct_attr,u_int16_t ct_info_attr);int (*parse)(const struct nlattr *attr,struct nf_conn *ct);int (*attach_expect)(const struct nlattr *attr,struct nf_conn *ct,u32 portid,u32 report);
@@ -384,9 +391,6 @@ struct nfq_ct_hook { enum ip_conntrack_info ctinfo, s32 off); };extern struct nfq_ct_hook __rcu *nfq_ct_hook; -#else -static inline void nf_ct_attach (struct sk_buff *new,struct sk_buff *skb) {} -#endif /** * nf_skb_duplicated - TEE target has sent a packet
include/net/netfilter/nfnetlink_queue.h deleted +0 -51
51 lines not on this page · raw message
net/netfilter/Makefile +0 -1
@@ -11,7 +11,6 @@ obj-$(CONFIG_NETFILTER) = netfilter.o obj-$(CONFIG_NETFILTER_NETLINK) += nfnetlink.o obj-$(CONFIG_NETFILTER_NETLINK_ACCT) += nfnetlink_acct.o nfnetlink_queue-y := nfnetlink_queue_core.o -nfnetlink_queue-$(CONFIG_NETFILTER_NETLINK_QUEUE_CT) += nfnetlink_queue_ct.o obj-$(CONFIG_NETFILTER_NETLINK_QUEUE) += nfnetlink_queue.o obj-$(CONFIG_NETFILTER_NETLINK_LOG) += nfnetlink_log.o
net/netfilter/core.c +6 -3
@@ -348,6 +348,12 @@ int skb_make_writable(struct sk_buff *skb, unsigned int writable_len) }EXPORT_SYMBOL (skb_make_writable); +/* This needs to be compiled in any case to avoid dependencies between the +* nfnetlink_queue code and nf_conntrack. +*/ +struct nfq_ct_hook __rcu *nfq_ct_hook__read_mostly ; +EXPORT_SYMBOL_GPL (nfq_ct_hook); +#if IS_ENABLED (CONFIG_NF_CONNTRACK )/* This does not belong here, but locally generated errors need it if connection tracking in use: without this, connection may not be in hash table, and hence
@@ -385,9 +391,6 @@ void nf_conntrack_destroy(struct nf_conntrack *nfct) }EXPORT_SYMBOL (nf_conntrack_destroy); -struct nfq_ct_hook __rcu *nfq_ct_hook__read_mostly ; -EXPORT_SYMBOL_GPL (nfq_ct_hook); -/* Built-in default zone used e.g. by modules. */ const struct nf_conntrack_zone nf_ct_zone_dflt = { .id =NF_CT_DEFAULT_ZONE_ID ,
net/netfilter/nf_conntrack_netlink.c +49 -3
75 lines not on this page · raw message
net/netfilter/nfnetlink_queue_core.c +40 -12
105 lines not on this page · raw message
net/netfilter/nfnetlink_queue_ct.c deleted +0 -113
113 lines not on this page · raw message
Signature · 1 line
2.1.4
Pablo Neira Ayuso
maintainerPATCH v3 2/2 netfilter: rename nfnetlink_queue_core.c to nfnetlink_queue.c
Now that we have integrated the ct glue code into nfnetlink_queue without introducing dependencies with the conntrack code.
---
v3: no changes
net/netfilter/Makefile | 1 - net/netfilter/nfnetlink_queue.c | 1443 ++++++++++++++++++++++++++++++++++ net/netfilter/nfnetlink_queue_core.c | 1443 ---------------------------------- 3 files changed, 1443 insertions(+), 1444 deletions(-) create mode 100644 net/netfilter/nfnetlink_queue.c delete mode 100644 net/netfilter/nfnetlink_queue_core.c
net/netfilter/Makefile +0 -1
@@ -10,7 +10,6 @@ obj-$(CONFIG_NETFILTER) = netfilter.o obj-$(CONFIG_NETFILTER_NETLINK) += nfnetlink.o obj-$(CONFIG_NETFILTER_NETLINK_ACCT) += nfnetlink_acct.o -nfnetlink_queue-y := nfnetlink_queue_core.o obj-$(CONFIG_NETFILTER_NETLINK_QUEUE) += nfnetlink_queue.o obj-$(CONFIG_NETFILTER_NETLINK_LOG) += nfnetlink_log.o
net/netfilter/nfnetlink_queue.c new file +1443 -0
1,443 lines not on this page · raw message
net/netfilter/nfnetlink_queue_core.c deleted +0 -1443
1,443 lines not on this page · raw message
Signature · 1 line
2.1.4
v2 cover netfilter: nfnetlink_log attach conntrack information
Hi Pablo,
I rebased on top of your patches, would you review these?
You suggested it might be better to divide ct attaching Kconfig to each of nfqueue and nflog, but I think it makes less sense now because of your rework. is it right?
Thanks,
PATCH v2 1/4 netfilter: nfnetlink_queue: rename related to nfqueue attaching conntrack info
The idea of this series of patch is to attach conntrack information to nflog like nfqueue has already done. nfqueue conntrack info attaching basis is generic, rename those names to generic one, glue.
--- include/linux/netfilter.h | 4 +-- net/netfilter/core.c | 4 +-- net/netfilter/nf_conntrack_netlink.c | 60 +++++++++++++++++----------------- net/netfilter/nfnetlink_queue.c | 30 ++++++++--------- 4 files changed, 49 insertions(+), 49 deletions(-)
include/linux/netfilter.h +2 -2
@@ -377,7 +377,7 @@ struct nf_conn;enum ip_conntrack_info ;struct nlattr ; -struct nfq_ct_hook { +struct nfnl_ct_hook {struct nf_conn *(*get_ct)(struct sk_buff *skb,enum ip_conntrack_info *ctinfo);size_t (*build_size)(const struct nf_conn *ct);
@@ -390,7 +390,7 @@ struct nfq_ct_hook {void (*seq_adjust)(struct sk_buff *skb,struct nf_conn *ct,enum ip_conntrack_info ctinfo,s32 off); }; -extern struct nfq_ct_hook __rcu *nfq_ct_hook; +extern struct nfnl_ct_hook __rcu *nfnl_ct_hook;/** * nf_skb_duplicated - TEE target has sent a packet
net/netfilter/core.c +2 -2
@@ -351,8 +351,8 @@ EXPORT_SYMBOL(skb_make_writable);/* This needs to be compiled in any case to avoid dependencies between the * nfnetlink_queue code and nf_conntrack. */ -struct nfq_ct_hook __rcu *nfq_ct_hook__read_mostly ; -EXPORT_SYMBOL_GPL (nfq_ct_hook); +struct nfnl_ct_hook __rcu *nfnl_ct_hook__read_mostly ; +EXPORT_SYMBOL_GPL (nfnl_ct_hook);#if IS_ENABLED (CONFIG_NF_CONNTRACK )/* This does not belong here, but locally generated errors need it if connection
net/netfilter/nf_conntrack_netlink.c +30 -30
149 lines not on this page · raw message
net/netfilter/nfnetlink_queue.c +15 -15
@@ -313,7 +313,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,struct net_device *outdev;struct nf_conn *ct =NULL ;enum ip_conntrack_info uninitialized_var (ctinfo ); -struct nfq_ct_hook *nfq_ct; +struct nfnl_ct_hook *nfnl_ct;bool csum_verify;char *secdata =NULL ;u32 seclen =0 ;
@@ -366,11 +366,11 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue, }if (queue->flags &NFQA_CFG_F_CONNTRACK ) { - nfq_ct =rcu_dereference (nfq_ct_hook); -if (nfq_ct !=NULL ) { - ct = nfq_ct->get_ct (entskb, &ctinfo); + nfnl_ct =rcu_dereference (nfnl_ct_hook); +if (nfnl_ct !=NULL ) { + ct = nfnl_ct->get_ct (entskb, &ctinfo);if (ct !=NULL ) - size += nfq_ct->build_size (ct); + size += nfnl_ct->build_size (ct); } }
@@ -515,7 +515,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,if (seclen &&nla_put (skb,NFQA_SECCTX , seclen, secdata)) goto nla_put_failure; -if (ct && nfq_ct->build (skb, ct, ctinfo,NFQA_CT ,NFQA_CT_INFO ) <0 ) +if (ct && nfnl_ct->build (skb, ct, ctinfo,NFQA_CT ,NFQA_CT_INFO ) <0 ) goto nla_put_failure; if (cap_len > data_len &&
@@ -1008,7 +1008,7 @@ nfqnl_recv_verdict_batch(struct sock *ctnl, struct sk_buff *skb,return 0 ; } -static struct nf_conn *nfqnl_ct_parse(struct nfq_ct_hook *nfq_ct, +static struct nf_conn *nfqnl_ct_parse(struct nfnl_ct_hook *nfnl_ct, const struct nlmsghdr *nlh, const struct nlattr * const nfqa[], struct nf_queue_entry *entry,
@@ -1016,15 +1016,15 @@ static struct nf_conn *nfqnl_ct_parse(struct nfq_ct_hook *nfq_ct, {struct nf_conn *ct; - ct = nfq_ct->get_ct (entry->skb, ctinfo); + ct = nfnl_ct->get_ct (entry->skb, ctinfo);if (ct ==NULL )return NULL ; -if (nfq_ct->parse (nfqa[NFQA_CT ], ct) <0 ) +if (nfnl_ct->parse (nfqa[NFQA_CT ], ct) <0 )return NULL ;if (nfqa[NFQA_EXP ]) - nfq_ct->attach_expect (nfqa[NFQA_EXP ], ct, + nfnl_ct->attach_expect (nfqa[NFQA_EXP ], ct,NETLINK_CB (entry->skb).portid,nlmsg_report (nlh));return ct;
@@ -1043,7 +1043,7 @@ nfqnl_recv_verdict(struct sock *ctnl, struct sk_buff *skb,unsigned int verdict;struct nf_queue_entry *entry;enum ip_conntrack_info uninitialized_var (ctinfo ); -struct nfq_ct_hook *nfq_ct; +struct nfnl_ct_hook *nfnl_ct;struct nf_conn *ct =NULL ;struct net *net =sock_net (ctnl);
@@ -1068,9 +1068,9 @@ nfqnl_recv_verdict(struct sock *ctnl, struct sk_buff *skb,if (nfqa[NFQA_CT ]) {/* rcu lock already held from nfnl->call_rcu. */ - nfq_ct =rcu_dereference (nfq_ct_hook); -if (nfq_ct !=NULL ) - ct =nfqnl_ct_parse (nfq_ct, nlh, nfqa, entry, &ctinfo); + nfnl_ct =rcu_dereference (nfnl_ct_hook); +if (nfnl_ct !=NULL ) + ct =nfqnl_ct_parse (nfnl_ct, nlh, nfqa, entry, &ctinfo); }if (nfqa[NFQA_PAYLOAD ]) {
@@ -1082,7 +1082,7 @@ nfqnl_recv_verdict(struct sock *ctnl, struct sk_buff *skb, verdict =NF_DROP ;if (ct && diff) - nfq_ct->seq_adjust (entry->skb, ct, ctinfo, diff); + nfnl_ct->seq_adjust (entry->skb, ct, ctinfo, diff); }if (nfqa[NFQA_MARK ])
Signature · 1 line
1.7.10.4
PATCH v2 2/4 netfilter: Kconfig rename QUEUE_CT to GLUE_CT
In reply to Ken-ichirou MATSUZAWAConntrack information attaching infrastructure is now generic and update it's name to use `glue' in previous patch. This patch updates Kconfig symbol name and adding NF_CT_NETLINK dependency.
--- net/netfilter/Kconfig | 10 +++++----- net/netfilter/nf_conntrack_netlink.c | 8 ++++---- 2 files changed, 9 insertions(+), 9 deletions(-)
net/netfilter/Kconfig +5 -5
@@ -354,7 +354,7 @@ config NF_CT_NETLINK_HELPER select NETFILTER_NETLINK depends on NF_CT_NETLINK depends on NETFILTER_NETLINK_QUEUE - depends on NETFILTER_NETLINK_QUEUE_CT + depends on NETFILTER_NETLINK_GLUE_CT depends on NETFILTER_ADVANCED help This option enables the user-space connection tracking helpers
@@ -362,10 +362,10 @@ config NF_CT_NETLINK_HELPER If unsure, say `N'. -config NETFILTER_NETLINK_QUEUE_CT - bool "NFQUEUE integration with Connection Tracking" - default n - depends on NETFILTER_NETLINK_QUEUE +config NETFILTER_NETLINK_GLUE_CT + bool "NFQUEUE integration with Connection Tracking" + default n + depends on NETFILTER_NETLINK_QUEUE && NF_CT_NETLINK help If this option is enabled, NFQUEUE can include Connection Tracking information together with the packet is the enqueued via NFNETLINK.
net/netfilter/nf_conntrack_netlink.c +4 -4
@@ -2133,7 +2133,7 @@ ctnetlink_alloc_expect(const struct nlattr *const cda[], struct nf_conn *ct,struct nf_conntrack_tuple *tuple, struct nf_conntrack_tuple *mask); -#ifdef CONFIG_NETFILTER_NETLINK_QUEUE_CT +#ifdef CONFIG_NETFILTER_NETLINK_GLUE_CT static size_t ctnetlink_glue_build_size (const struct nf_conn *ct) {
@@ -2403,7 +2403,7 @@ static struct nfnl_ct_hook ctnetlink_glue_hook = { .attach_expect = ctnetlink_glue_attach_expect, .seq_adjust = ctnetlink_glue_seqadj, }; -#endif /* CONFIG_NETFILTER_NETLINK_QUEUE_CT */ +#endif /* CONFIG_NETFILTER_NETLINK_GLUE_CT */ /*********************************************************************** * EXPECT
@@ -3387,7 +3387,7 @@ static int __init ctnetlink_init(void)pr_err ("ctnetlink_init: cannot register pernet operations\n" ); goto err_unreg_exp_subsys; } -#ifdef CONFIG_NETFILTER_NETLINK_QUEUE_CT +#ifdef CONFIG_NETFILTER_NETLINK_GLUE_CT /* setup interaction between nf_queue and nf_conntrack_netlink. */ RCU_INIT_POINTER (nfnl_ct_hook, &ctnetlink_glue_hook);#endif
@@ -3408,7 +3408,7 @@ static void __exit ctnetlink_exit(void)unregister_pernet_subsys (&ctnetlink_net_ops);nfnetlink_subsys_unregister (&ctnl_exp_subsys);nfnetlink_subsys_unregister (&ctnl_subsys); -#ifdef CONFIG_NETFILTER_NETLINK_QUEUE_CT +#ifdef CONFIG_NETFILTER_NETLINK_GLUE_CT RCU_INIT_POINTER (nfnl_ct_hook,NULL );#endif }
Signature · 1 line
1.7.10.4
PATCH v2 3/4 netfilter: nf_conntrack_netlink: add const qualifier to nfnl_hook.get_ct
In reply to Ken-ichirou MATSUZAWAget_ct as is and will not update its skb argument, and users of nfnl_ct_hook is currently only nfqueue, we can add const qualifier.
--- include/linux/netfilter.h | 2 +- net/netfilter/nf_conntrack_netlink.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
include/linux/netfilter.h +1 -1
@@ -378,7 +378,7 @@ enum ip_conntrack_info;struct nlattr ;struct nfnl_ct_hook { -struct nf_conn *(*get_ct)(struct sk_buff *skb, +struct nf_conn *(*get_ct)(const struct sk_buff *skb,enum ip_conntrack_info *ctinfo);size_t (*build_size)(const struct nf_conn *ct); int (*build)(struct sk_buff *skb, struct nf_conn *ct,
net/netfilter/nf_conntrack_netlink.c +1 -1
@@ -2162,7 +2162,7 @@ ctnetlink_glue_build_size(const struct nf_conn *ct) ; } -static struct nf_conn *ctnetlink_glue_get_ct (struct sk_buff *skb, +static struct nf_conn *ctnetlink_glue_get_ct (const struct sk_buff *skb,enum ip_conntrack_info *ctinfo) {struct nf_conn *ct;
Signature · 1 line
1.7.10.4
PATCH v2 4/4 netfilter: nfnetlink_log: allow to attach conntrack
In reply to Ken-ichirou MATSUZAWAThis patch enables to include the conntrack information together with the packet that is sent to user-space via NFLOG, then a user-space program can acquire NATed information by this NFULA_CT attribute.
Including the conntrack information is optional, you can set it via NFULNL_CFG_F_CONNTRACK flag with the NFULA_CFG_FLAGS attribute like NFQUEUE.
--- include/uapi/linux/netfilter/nfnetlink_log.h | 3 ++ net/netfilter/Kconfig | 9 +++--- net/netfilter/nfnetlink_log.c | 40 ++++++++++++++++++++++---- 3 files changed, 42 insertions(+), 10 deletions(-)
include/uapi/linux/netfilter/nfnetlink_log.h +3 -0
@@ -51,6 +51,8 @@ enum nfulnl_attr_type {NFULA_HWTYPE ,/* hardware type */ NFULA_HWHEADER ,/* hardware header */ NFULA_HWLEN ,/* hardware header length */ +NFULA_CT ,/* nf_conntrack_netlink.h */ +NFULA_CT_INFO ,/* enum ip_conntrack_info */ __NFULA_MAX };
@@ -93,5 +95,6 @@ enum nfulnl_attr_config {#define NFULNL_CFG_F_SEQ 0x0001#define NFULNL_CFG_F_SEQ_GLOBAL 0x0002 +#define NFULNL_CFG_F_CONNTRACK 0x0004#endif /* _NFNETLINK_LOG_H */
net/netfilter/Kconfig +5 -4
@@ -363,12 +363,13 @@ config NF_CT_NETLINK_HELPER If unsure, say `N'. config NETFILTER_NETLINK_GLUE_CT - bool "NFQUEUE integration with Connection Tracking" + bool "NFQUEUE and NFLOG integration with Connection Tracking" default n - depends on NETFILTER_NETLINK_QUEUE && NF_CT_NETLINK + depends on (NETFILTER_NETLINK_QUEUE || NETFILTER_NETLINK_LOG) && NF_CT_NETLINK help - If this option is enabled, NFQUEUE can include Connection Tracking - information together with the packet is the enqueued via NFNETLINK. + If this option is enabled, NFQUEUE and NFLOG can include + Connection Tracking information together with the packet is + the enqueued via NFNETLINK. config NF_NAT tristate
net/netfilter/nfnetlink_log.c +34 -6
@@ -27,6 +27,7 @@#include <net/netlink.h> #include <linux/netfilter/nfnetlink.h> #include <linux/netfilter/nfnetlink_log.h> +#include <linux/netfilter/nf_conntrack_common.h> #include <linux/spinlock.h> #include <linux/sysctl.h> #include <linux/proc_fs.h>
@@ -401,7 +402,9 @@ __build_packet_message(struct nfnl_log_net *log,unsigned int hooknum, const struct net_device *indev, const struct net_device *outdev, - const char *prefix, unsigned int plen) + const char *prefix, unsigned int plen, + const struct nfnl_ct_hook *nfnl_ct, + struct nf_conn *ct, enum ip_conntrack_info ctinfo) { struct nfulnl_msg_packet_hdr pmsg;struct nlmsghdr *nlh;
@@ -575,6 +578,10 @@ __build_packet_message(struct nfnl_log_net *log, htonl(atomic_inc_return(&log->global_seq)))) goto nla_put_failure; +if (ct && nfnl_ct->build (inst->skb, ct, ctinfo, +NFULA_CT ,NFULA_CT_INFO ) <0 ) + goto nla_put_failure; +if (data_len) {struct nlattr *nla;int size =nla_attr_size (data_len);
@@ -620,12 +627,16 @@ nfulnl_log_packet(struct net *net,const struct nf_loginfo *li_user, const char *prefix) { - unsigned int size, data_len; + size_t size; +unsigned int data_len;struct nfulnl_instance *inst;const struct nf_loginfo *li;unsigned int qthreshold;unsigned int plen;struct nfnl_log_net *log =nfnl_log_pernet (net); +const struct nfnl_ct_hook *nfnl_ct =NULL ; +struct nf_conn *ct =NULL ; +enum ip_conntrack_info uninitialized_var (ctinfo );if (li_user && li_user->type ==NF_LOG_TYPE_ULOG ) li = li_user;
@@ -671,7 +682,15 @@ nfulnl_log_packet(struct net *net, size +=nla_total_size (sizeof (u_int32_t));if (inst->flags &NFULNL_CFG_F_SEQ_GLOBAL ) size +=nla_total_size (sizeof (u_int32_t)); - +if (inst->flags &NFULNL_CFG_F_CONNTRACK ) { + nfnl_ct =rcu_dereference (nfnl_ct_hook); +if (nfnl_ct !=NULL ) { + ct = nfnl_ct->get_ct (skb, &ctinfo); +if (ct !=NULL ) + size += nfnl_ct->build_size (ct); + } + } + qthreshold = inst->qthreshold;/* per-rule qthreshold overrides per-instance */ if (li->u.ulog.qthreshold)
@@ -715,7 +734,8 @@ nfulnl_log_packet(struct net *net, inst->qlen++;__build_packet_message (log, inst, skb, data_len, pf, - hooknum, in, out, prefix, plen); + hooknum, in, out, prefix, plen, + nfnl_ct, ct, ctinfo);if (inst->qlen >= qthreshold)__nfulnl_flush (inst);
@@ -899,13 +919,21 @@ nfulnl_recv_config(struct sock *ctnl, struct sk_buff *skb, }if (nfula[NFULA_CFG_FLAGS ]) { -__be16 flags =nla_get_be16 (nfula[NFULA_CFG_FLAGS ]); +u16 flags =ntohs (nla_get_be16 (nfula[NFULA_CFG_FLAGS ]));if (!inst) { ret = -ENODEV ; goto out; } -nfulnl_set_flags (inst,ntohs (flags)); + +if (flags &NFULNL_CFG_F_CONNTRACK && +rcu_dereference (nfnl_ct_hook) ==NULL ) { + ret = -EOPNOTSUPP ; + goto out; + + } + +nfulnl_set_flags (inst, flags); } out_put:
Signature · 1 line
1.7.10.4
Pablo Neira Ayuso
maintainerRe: [PATCHv2 nf-next 4/4] netfilter: nfnetlink_log: allow to attach conntrack
Quoted 4 lines · Ken-ichirou MATSUZAWA
This patch enables to include the conntrack information together with the packet that is sent to user-space via NFLOG, then a user-space program can acquire NATed information by this
NFULA_CTattribute.
WARNING: suspect code indent for conditional statements (8, 24) #99: FILE: net/netfilter/nfnetlink_log.c:581:
+ if (ct && nfnl_ct->build(inst->skb, ct, ctinfo,
[...]
+ goto nla_put_failure;
Please, make sure you use tabs next time.
ERROR: trailing whitespace
#102: FILE: net/netfilter/nfnetlink_log.c:584:
ERROR: trailing whitespace
#137: FILE: net/netfilter/nfnetlink_log.c:693:
Please, run checkpatch.pl on your patches next time to address whitespace errors and wrong indent.
And regarding this:
...
rcu_dereference(nfnl_ct_hook) == NULL) {it should be:
rcu_access_pointer(nfnl_ct_hook) == NULL) {I have fixed these issues here this time, no need to resend.
But, would you please resubmit this:
http://patchwork.ozlabs.org/patch/516185/
but using rcu_access_pointer() instead?
BTW, another thing that would be good to investigate is to autoload nf_conntrack_netlink if the process passes the _F_CONNTRACK flag is set. Would you have a look into that?
Thanks.
v2 cover netfilter: nfnetlink_queue: check NFQA_CFG_F_CONNTRACK config flag
Thank you for advices.
I try to fix rcu_dereference() and to autoload not only nf_conntrack_netlink but also l3ct module if nfgen family is specified. Is it unnecessary?
I will make same thing to nfnetlink_log after this round.
Thanks,
PATCH v2 1/1 netfilter: nfnetlink_queue: check NFQA_CFG_F_CONNTRACK config flag
This patch enables to check GLUE_CT is enabled or not when NFQA_CFG_F_CONNTRACK config flag is received. And try to load nf_conntrack_netlink module, and l3proto module if family is specified. Then returns error either case is failed.
--- net/netfilter/nfnetlink_queue.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
net/netfilter/nfnetlink_queue.c +19 -0
@@ -34,6 +34,7 @@#include <net/tcp_states.h> #include <net/netfilter/nf_queue.h> #include <net/netns/generic.h> +#include <net/netfilter/nf_conntrack.h> #include <linux/atomic.h>
@@ -1225,6 +1226,24 @@ nfqnl_recv_config(struct sock *ctnl, struct sk_buff *skb, goto err_out_unlock; }#endif +if (flags & mask &NFQA_CFG_F_CONNTRACK ) { + if (!rcu_access_pointer(nfnl_ct_hook)) +#ifdef CONFIG_MODULES +if (request_module ("ip_conntrack_netlink" ) || + !rcu_access_pointer(nfnl_ct_hook)) +#endif + { + ret = -EOPNOTSUPP ; + goto err_out_unlock; + } + +if (nfmsg->nfgen_family && +nf_ct_l3proto_try_module_get (nfmsg->nfgen_family)) { + ret = -EPROTONOSUPPORT ; + goto err_out_unlock; + } + } +spin_lock_bh (&queue->lock); queue->flags &= ~mask; queue->flags |= flags & mask;
Signature · 1 line
1.7.10.4
Pablo Neira Ayuso
maintainerRe: [PATCHv2 nf-next 1/1] netfilter: nfnetlink_queue: check NFQA_CFG_F_CONNTRACK config flag
Quoted 23 lines · Ken-ichirou MATSUZAWA
This patch enables to check
GLUE_CTis enabled or not whenNFQA_CFG_F_CONNTRACKconfig flag is received. And try to loadnf_conntrack_netlinkmodule, and l3proto module if family is specified. Then returns error either case is failed.Signed-off-by Ken-ichirou MATSUZAWA <[email protected]>--- net/netfilter/nfnetlink_queue.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)net/netfilter/nfnetlink_queue.c +1
-0@@ -34,6 +34,7 @@#include <net/tcp_states.h> #include <net/netfilter/nf_queue.h> #include <net/netns/generic.h> +#include <net/netfilter/nf_conntrack.h> #include <linux/atomic.h>
net/netfilter/nfnetlink_queue.c nfqnl_recv_config(struct sock *ctnl, struct sk_buff *skb, · line 1232
Quoted 7 lines
net/netfilter/nfnetlink_queue.c +4
-0@@ -1225,6 +1226,24 @@ nfqnl_recv_config(struct sock *ctnl, struct sk_buff *skb, goto err_out_unlock; }#endif +if (flags & mask &NFQA_CFG_F_CONNTRACK ) { + if (!rcu_access_pointer(nfnl_ct_hook)) +#ifdef CONFIG_MODULES + if (request_module("ip_conntrack_netlink") ||
nfnl_lock is held while requesting a module, which is something that we should avoid. Please, abort the operation and return -EAGAIN to retry, this is the usual procedure when requesting other modules.
Quoted 4 lines
net/netfilter/nfnetlink_queue.c +4
-0+ !rcu_access_pointer(nfnl_ct_hook)) +#endif + { + ret = -EOPNOTSUPP ;
net/netfilter/nfnetlink_queue.c nfqnl_recv_config(struct sock *ctnl, struct sk_buff *skb, · line 1243
Quoted 10 lines
net/netfilter/nfnetlink_queue.c +7
-0+ goto err_out_unlock; + } + +if (nfmsg->nfgen_family && +nf_ct_l3proto_try_module_get (nfmsg->nfgen_family)) { + ret = -EPROTONOSUPPORT ; + goto err_out_unlock;
I think this chunk belongs to nf_conntrack_netlink, the nf_conntrack_{ipv4,ipv6} modules gets loaded when the user tries to create a conntrack/expectation.
Otherwise we create a hard dependency between nfnetlink_queue and nf_conntrack, which is what we're trying to avoid ;-)
Quoted 9 lines
net/netfilter/nfnetlink_queue.c +3
-0+ } + } +net/netfilter/nfnetlink_queue.c +0
-0spin_lock_bh (&queue->lock); queue->flags &= ~mask; queue->flags |= flags & mask;Signature · 1 line
1.7.10.4
Re: [PATCHv2 nf-next 1/1] netfilter: nfnetlink_queue: check NFQA_CFG_F_CONNTRACK config flag
Quoted 9 lines · Pablo Neira Ayuso
net/netfilter/nfnetlink_queue.c nfqnl_recv_config(struct sock *ctnl, struct sk_buff *skb, · line 1232
Quoted 4 lines · Ken-ichirou MATSUZAWA
net/netfilter/nfnetlink_queue.c +4
-0+if (flags & mask &NFQA_CFG_F_CONNTRACK ) { + if (!rcu_access_pointer(nfnl_ct_hook)) +#ifdef CONFIG_MODULES + if (request_module("ip_conntrack_netlink") ||
nfnl_lockis held while requesting a module, which is something that we should avoid. Please, abort the operation and return-EAGAINto retry, this is the usual procedure when requesting other modules.
Thank you as always.
I read another request_module() part and I think I can understand, would you review it again? I changed rcu_access_pointer() since rcu_read_lock() is called in nfqnl_recv_config().
I think a few fix seems to be needed at that time, and try to fix it in the following patches. Would you confirm these too?
Thanks,
PATCH v3 1/1 netfilter: nfnetlink_queue: check NFQA_CFG_F_CONNTRACK config flag
This patch enables to check if GLUE_CT is enabled and try to load dependent, nf_conntrack_netlink module when NFQA_CFG_F_CONNTRACK config flag is received. Then returns error either case is failed.
--- net/netfilter/nfnetlink_queue.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
net/netfilter/nfnetlink_queue.c +17 -0
@@ -1225,6 +1225,23 @@ nfqnl_recv_config(struct sock *ctnl, struct sk_buff *skb, goto err_out_unlock; }#endif +if (flags & mask &NFQA_CFG_F_CONNTRACK && + !rcu_dereference (nfnl_ct_hook)) { +#ifdef CONFIG_MODULES +rcu_read_unlock (); +nfnl_unlock (NFNL_SUBSYS_QUEUE ); +request_module ("ip_conntrack_netlink" ); +nfnl_lock (NFNL_SUBSYS_QUEUE ); +rcu_read_lock (); +if (rcu_dereference (nfnl_ct_hook)) { + ret = -EAGAIN ; + goto err_out_unlock; + } +#endif + ret = -EOPNOTSUPP ; + goto err_out_unlock; + } +spin_lock_bh (&queue->lock); queue->flags &= ~mask; queue->flags |= flags & mask;
Signature · 1 line
1.7.10.4
PATCH 1/1 netfilter: nfnetlink_log: autoload nf_conntrack_netlink module NFQA_CFG_F_CONNTRACK config flag
In reply to Ken-ichirou MATSUZAWAThis patch enables to load nf_conntrack_netlink module if NFULNL_CFG_F_CONNTRACK config flag is specified.
--- net/netfilter/nfnetlink_log.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
net/netfilter/nfnetlink_log.c +10 -1
@@ -927,7 +927,16 @@ nfulnl_recv_config(struct sock *ctnl, struct sk_buff *skb, }if (flags &NFULNL_CFG_F_CONNTRACK && -rcu_access_pointer (nfnl_ct_hook) ==NULL ) { + !rcu_access_pointer (nfnl_ct_hook)) { +#ifdef CONFIG_MODULES +nfnl_unlock (NFNL_SUBSYS_ULOG ); +request_module ("ip_conntrack_netlink" ); +nfnl_lock (NFNL_SUBSYS_ULOG ); +if (rcu_access_pointer (nfnl_ct_hook)) { + ret = -EAGAIN ; + goto out; + } +#endif ret = -EOPNOTSUPP ; goto out; }
Signature · 1 line
1.7.10.4
Re: [PATCH nf-next] netfilter: nfnetlink_log: autoload nf_conntrack_netlink module NFQA_CFG_F_CONNTRACK config flag
Quoted 2 lines · Ken-ichirou MATSUZAWA
This patch enables to load
nf_conntrack_netlinkmodule ifNFULNL_CFG_F_CONNTRACKconfig flag is specified.
Applied, thanks.
Pablo Neira Ayuso
maintainerRe: [PATCH nf-next] netfilter: nfnetlink_log: autoload nf_conntrack_netlink module NFQA_CFG_F_CONNTRACK config flag
Quoted 5 lines · Pablo Neira Ayuso
Quoted 2 lines · Ken-ichirou MATSUZAWA
This patch enables to load
nf_conntrack_netlinkmodule ifNFULNL_CFG_F_CONNTRACKconfig flag is specified.Applied, thanks.
I just noticed we're breaking atomicity when configuring things.
I mean, if we fail to configure the flags, we return an error but it's too late, we just create the instance and we have configured everything.
I quickly made a couple of patches here, we'll be posting them tomorrow.
Pablo Neira Ayuso
maintainerRe: [PATCH nf-next] netfilter: nfnetlink_log: autoload nf_conntrack_netlink module NFQA_CFG_F_CONNTRACK config flag
In reply to Ken-ichirou MATSUZAWAQuoted 17 lines · Ken-ichirou MATSUZAWA
This patch enables to load
nf_conntrack_netlinkmodule ifNFULNL_CFG_F_CONNTRACKconfig flag is specified.Signed-off-by Ken-ichirou MATSUZAWA <[email protected]>--- net/netfilter/nfnetlink_log.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)net/netfilter/nfnetlink_log.c +0
-1@@ -927,7 +927,16 @@ nfulnl_recv_config(struct sock *ctnl, struct sk_buff *skb, } if (flags & NFULNL_CFG_F_CONNTRACK && -rcu_access_pointer (nfnl_ct_hook) ==NULL ) {
net/netfilter/nfnetlink_log.c nfulnl_recv_config(struct sock *ctnl, struct sk_buff *skb, · line 939
Quoted 10 lines
net/netfilter/nfnetlink_log.c +10
-0+ !rcu_access_pointer(nfnl_ct_hook)) { +#ifdef CONFIG_MODULES +nfnl_unlock (NFNL_SUBSYS_ULOG ); +request_module ("ip_conntrack_netlink" ); +nfnl_lock (NFNL_SUBSYS_ULOG ); +if (rcu_access_pointer (nfnl_ct_hook)) { + ret = -EAGAIN ; + goto out; + } +#endif
I know this is not your fault, but could you have a look to see if we can get resolved the broken atomicity problems in first place when applying configuration updates to nfnetlink_queue in a similar way to what I did for nfnetlink_log?
I mean, move code that check for things right up the beginning of the functions, then we pass the preparation phase and we're good, we apply the configuration changes.
Again not your fault, but there's another nasty thing in that code: We can destroy a queue in NFQNL_CFG_CMD_UNBIND, then we can keep going on configuring it when it is actually destroyed. This is working only by luck because we're using call_rcu so the object is still there, but this is sloppy and it should be fixed.
After having that resolved, we can go back to adding module autoload.
Let me know,
Thanks.
Re: [PATCH nf-next] netfilter: nfnetlink_log: autoload nf_conntrack_netlink module NFQA_CFG_F_CONNTRACK config flag
Sorry for being late.
Quoted 3 lines · Pablo Neira Ayuso
can get resolved the broken atomicity problems in first place when applying configuration updates to
nfnetlink_queuein a similar way to what I did fornfnetlink_log?
I tried it, would you review the following patches?
Quoted 2 lines
can destroy a queue in
NFQNL_CFG_CMD_UNBIND, then we can keep going on configuring it when it is actually destroyed. This is working only by
I'm wondering how we handle obsolete commands NFQNL_CFG_CMD_PF_BIND / UNBIND, especially with a valid queue_num and configurations. These patches follows current behavior, do nothing, ignores another nlas and just returns success. Should I update patches to handle configuraion nlas with obsolute commands?
Thanks,
PATCH 1/3 netfilter: nfnetlink_queue: remove duplicated obsolete commands handling
--- net/netfilter/nfnetlink_queue.c | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-)
net/netfilter/nfnetlink_queue.c +5 -13
@@ -1116,21 +1116,10 @@ nfqnl_recv_config(struct sock *ctnl, struct sk_buff *skb,struct nfgenmsg *nfmsg =nlmsg_data (nlh);u_int16_t queue_num =ntohs (nfmsg->res_id);struct nfqnl_instance *queue; -struct nfqnl_msg_config_cmd *cmd =NULL ;struct net *net =sock_net (ctnl);struct nfnl_queue_net *q =nfnl_queue_pernet (net);int ret =0 ; -if (nfqa[NFQA_CFG_CMD ]) { - cmd =nla_data (nfqa[NFQA_CFG_CMD ]); - -/* Obsolete commands without queue context */ -switch (cmd->command) { -case NFQNL_CFG_CMD_PF_BIND :return 0 ; -case NFQNL_CFG_CMD_PF_UNBIND :return 0 ; - } - } -rcu_read_lock (); queue =instance_lookup (q, queue_num);if (queue && queue->peer_portid !=NETLINK_CB (skb).portid) {
@@ -1138,7 +1127,9 @@ nfqnl_recv_config(struct sock *ctnl, struct sk_buff *skb, goto err_out_unlock; } -if (cmd !=NULL ) { +if (nfqa[NFQA_CFG_CMD ]) { +struct nfqnl_msg_config_cmd *cmd =nla_data (nfqa[NFQA_CFG_CMD ]); +switch (cmd->command) {case NFQNL_CFG_CMD_BIND :if (queue) {
@@ -1161,7 +1152,8 @@ nfqnl_recv_config(struct sock *ctnl, struct sk_buff *skb,break ;case NFQNL_CFG_CMD_PF_BIND :case NFQNL_CFG_CMD_PF_UNBIND : -break ; +/* Obsolete commands without queue context */ + goto err_out_unlock;default : ret = -ENOTSUPP ;break ;
Signature · 1 line
1.7.10.4
Pablo Neira Ayuso
maintainerRe: [PATCH nf-next 1/3] netfilter: nfnetlink_queue: remove duplicated obsolete commands handling
Quoted 18 lines · Ken-ichirou MATSUZAWA
Signed-off-by Ken-ichirou MATSUZAWA <[email protected]>--- net/netfilter/nfnetlink_queue.c | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-)net/netfilter/nfnetlink_queue.c +0
-1@@ -1116,21 +1116,10 @@ nfqnl_recv_config(struct sock *ctnl, struct sk_buff *skb,struct nfgenmsg *nfmsg =nlmsg_data (nlh);u_int16_t queue_num =ntohs (nfmsg->res_id);struct nfqnl_instance *queue; -struct nfqnl_msg_config_cmd *cmd =NULL ;struct net *net =sock_net (ctnl);struct nfnl_queue_net *q =nfnl_queue_pernet (net);int ret =0 ;
net/netfilter/nfnetlink_queue.c nfqnl_recv_config(struct sock *ctnl, struct sk_buff *skb, · line 1133
Quoted 10 lines
net/netfilter/nfnetlink_queue.c +0
-10-if (nfqa[NFQA_CFG_CMD ]) { - cmd =nla_data (nfqa[NFQA_CFG_CMD ]); - -/* Obsolete commands without queue context */ -switch (cmd->command) { -case NFQNL_CFG_CMD_PF_BIND :return 0 ; -case NFQNL_CFG_CMD_PF_UNBIND :return 0 ; - } - } -
Thanks for following up on this Ken-ichirou.
However, I suspect this is wrong:
commit 0360ae412d09bc6f4864c801effcb20bfd84520e
Author: Florian Westphal <[email protected]> Date: Fri Nov 23 06:22:21 2012 +0000
netfilter: kill support for per-af queue backends
The former behaviour before Florian's patch is that NFQNL_CFG_CMD_PF_BIND and NFQNL_CFG_CMD_PF_UNBIND are terminal.
This code is there not to break backward compatibility, ie. old userspace that rely on these commands.
After this patch, we inspect if there is an existing queue for this, which was not happening before.
As I said, my concerns go in the direction of possible breakage of old code, we shouldn't break backward.
Let me know, thanks.
Quoted 3 lines
net/netfilter/nfnetlink_queue.c +0
-0rcu_read_lock (); queue =instance_lookup (q, queue_num);if (queue && queue->peer_portid !=NETLINK_CB (skb).portid) {
Re: [PATCH nf-next 1/3] netfilter: nfnetlink_queue: remove duplicated obsolete commands handling
Sorry for being late often.
Quoted 6 lines · Pablo Neira Ayuso
commit
0360ae412d09bc6f4864c801effcb20bfd84520eAuthor: Florian Westphal <[email protected]> Date: Fri Nov 23 06:22:21 2012 +0000netfilter: kill support for per-af queue backends
Quoted 8 lines
The former behaviour before Florian's patch is that
NFQNL_CFG_CMD_PF_BINDandNFQNL_CFG_CMD_PF_UNBINDare terminal.This code is there not to break backward compatibility, ie. old userspace that rely on these commands.
After this patch, we inspect if there is an existing queue for this, which was not happening before.
I tried to follow your advice, would you see again?
Quoted 2 lines
As I said, my concerns go in the direction of possible breakage of old code, we shouldn't break backward.
I'm worry about appended patches 3/5 and 5/5 may not follow it. After these patches applied, userspace can not update instance's options with invalid command. is it acceptable?
Thanks,
PATCH 1/5 netfilter: nfnetlink_queue: validate dependencies to avoid breaking atomicity
Check that dependencies are fulfilled before updating the queue instance, otherwise we can leave things in intermediate state on errors in nfqnl_recv_config().
--- net/netfilter/nfnetlink_queue.c | 74 +++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 42 deletions(-)
net/netfilter/nfnetlink_queue.c +32 -42
@@ -1113,6 +1113,7 @@ static int nfqnl_recv_config(struct net *net, struct sock *ctnl,struct nfqnl_instance *queue;struct nfqnl_msg_config_cmd *cmd =NULL ;struct nfnl_queue_net *q =nfnl_queue_pernet (net); +__u32 flags =0 , mask =0 ;int ret =0 ;if (nfqa[NFQA_CFG_CMD ]) {
@@ -1125,6 +1126,29 @@ static int nfqnl_recv_config(struct net *net, struct sock *ctnl, } } +/* Check if we support these flags in first place, dependencies should +* be there too not to break atomicity. +*/ +if (nfqa[NFQA_CFG_FLAGS ]) { +if (!nfqa[NFQA_CFG_MASK ]) { +/* A mask is needed to specify which flags are being +* changed. +*/ +return -EINVAL ; + } + + flags =ntohl (nla_get_be32 (nfqa[NFQA_CFG_FLAGS ])); + mask =ntohl (nla_get_be32 (nfqa[NFQA_CFG_MASK ])); + +if (flags >=NFQA_CFG_F_MAX ) +return -EOPNOTSUPP ; + +#if !IS_ENABLED (CONFIG_NETWORK_SECMARK ) +if (flags & mask &NFQA_CFG_F_SECCTX ) +return -EOPNOTSUPP ; +#endif + } +rcu_read_lock (); queue =instance_lookup (q, queue_num);if (queue && queue->peer_portid !=NETLINK_CB (skb).portid) {
@@ -1162,60 +1186,26 @@ static int nfqnl_recv_config(struct net *net, struct sock *ctnl, } } -if (nfqa[NFQA_CFG_PARAMS ]) { -struct nfqnl_msg_config_params *params; +if (!queue) { + ret = -ENODEV ; + goto err_out_unlock; + } -if (!queue) { - ret = -ENODEV ; - goto err_out_unlock; - } - params =nla_data (nfqa[NFQA_CFG_PARAMS ]); +if (nfqa[NFQA_CFG_PARAMS ]) { +struct nfqnl_msg_config_params *params + =nla_data (nfqa[NFQA_CFG_PARAMS ]);nfqnl_set_mode (queue, params->copy_mode,ntohl (params->copy_range)); }if (nfqa[NFQA_CFG_QUEUE_MAXLEN ]) { -__be32 *queue_maxlen; - -if (!queue) { - ret = -ENODEV ; - goto err_out_unlock; - } - queue_maxlen =nla_data (nfqa[NFQA_CFG_QUEUE_MAXLEN ]); +__be32 *queue_maxlen =nla_data (nfqa[NFQA_CFG_QUEUE_MAXLEN ]);spin_lock_bh (&queue->lock); queue->queue_maxlen =ntohl (*queue_maxlen);spin_unlock_bh (&queue->lock); }if (nfqa[NFQA_CFG_FLAGS ]) { -__u32 flags, mask; - -if (!queue) { - ret = -ENODEV ; - goto err_out_unlock; - } - -if (!nfqa[NFQA_CFG_MASK ]) { -/* A mask is needed to specify which flags are being -* changed. -*/ - ret = -EINVAL ; - goto err_out_unlock; - } - - flags =ntohl (nla_get_be32 (nfqa[NFQA_CFG_FLAGS ])); - mask =ntohl (nla_get_be32 (nfqa[NFQA_CFG_MASK ])); - -if (flags >=NFQA_CFG_F_MAX ) { - ret = -EOPNOTSUPP ; - goto err_out_unlock; - } -#if !IS_ENABLED (CONFIG_NETWORK_SECMARK ) -if (flags & mask &NFQA_CFG_F_SECCTX ) { - ret = -EOPNOTSUPP ; - goto err_out_unlock; - } -#endif spin_lock_bh (&queue->lock); queue->flags &= ~mask; queue->flags |= flags & mask;
Signature · 1 line
1.7.10.4
PATCH 2/5 netfilter: nfnetlink_queue: not handle options after unbind
In reply to Ken-ichirou MATSUZAWAThis patch stops processing after destroying a queue instance.
--- net/netfilter/nfnetlink_queue.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
net/netfilter/nfnetlink_queue.c +1 -1
@@ -1176,7 +1176,7 @@ static int nfqnl_recv_config(struct net *net, struct sock *ctnl, goto err_out_unlock; }instance_destroy (q, queue); -break ; + goto err_out_unlock;case NFQNL_CFG_CMD_PF_BIND :case NFQNL_CFG_CMD_PF_UNBIND :break ;
Signature · 1 line
1.7.10.4
PATCH 3/5 netfilter: nfnetlink_queue: just returns error for unknown command
In reply to Ken-ichirou MATSUZAWAThis patch stops processing options for unknown command.
--- net/netfilter/nfnetlink_queue.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
net/netfilter/nfnetlink_queue.c +1 -1
@@ -1182,7 +1182,7 @@ static int nfqnl_recv_config(struct net *net, struct sock *ctnl,break ;default : ret = -ENOTSUPP ; -break ; + goto err_out_unlock; } }
Signature · 1 line
1.7.10.4
PATCH 4/5 netfilter: nfnetlink_queue: autoload nf_conntrack_netlink module NFQA_CFG_F_CONNTRACK config flag
In reply to Ken-ichirou MATSUZAWAThis patch enables to load nf_conntrack_netlink module if NFQA_CFG_F_CONNTRACK config flag is specified.
--- net/netfilter/nfnetlink_queue.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
net/netfilter/nfnetlink_queue.c +11 -0
@@ -1147,6 +1147,17 @@ static int nfqnl_recv_config(struct net *net, struct sock *ctnl,if (flags & mask &NFQA_CFG_F_SECCTX )return -EOPNOTSUPP ;#endif +if ((flags & mask &NFQA_CFG_F_CONNTRACK ) && + !rcu_access_pointer (nfnl_ct_hook)) { +#ifdef CONFIG_MODULES +nfnl_unlock (NFNL_SUBSYS_QUEUE ); +request_module ("ip_conntrack_netlink" ); +nfnl_lock (NFNL_SUBSYS_QUEUE ); +if (rcu_access_pointer (nfnl_ct_hook)) +return -EAGAIN ; +#endif +return -EOPNOTSUPP ; + } }rcu_read_lock ();
Signature · 1 line
1.7.10.4
PATCH 5/5 netfilter: nfnetlink_log: just returns error for unknown command
In reply to Ken-ichirou MATSUZAWAThis patch stops processing options for unknown command.
--- net/netfilter/nfnetlink_log.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
net/netfilter/nfnetlink_log.c +3 -1
@@ -888,7 +888,9 @@ static int nfulnl_recv_config(struct net *net, struct sock *ctnl, goto out_put;default : ret = -ENOTSUPP ; -break ; +if (inst) + goto out_put; + goto out; } }else if (!inst) { ret = -ENODEV ;
Signature · 1 line
1.7.10.4
Pablo Neira Ayuso
maintainerRe: [PATCH nf-next 5/5] netfilter: nfnetlink_log: just returns error for unknown command
Quoted 12 lines · Ken-ichirou MATSUZAWA
This patch stops processing options for unknown command.
Signed-off-by Ken-ichirou MATSUZAWA <[email protected]>--- net/netfilter/nfnetlink_log.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)net/netfilter/nfnetlink_log.c +0
-0
net/netfilter/nfnetlink_log.c static int nfulnl_recv_config(struct net *net, struct sock *ctnl, · line 893
Quoted 7 lines
net/netfilter/nfnetlink_log.c +3
-1@@ -888,7 +888,9 @@ static int nfulnl_recv_config(struct net *net, struct sock *ctnl, goto out_put;default : ret = -ENOTSUPP ; -break ; +if (inst) + goto out_put; + goto out;
We can simplify this to:
goto out_put;
since instance_put() already checks if inst is non-null.
No need to resend, I'll fix this here.
Thanks a lot for following up on this series Ken-ichirou :-).
PATCH 2/3 netfilter: nfnetlink_queue: validate dependencies to avoid breaking atomicity
In reply to Ken-ichirou MATSUZAWACheck that dependencies are fulfilled before updating the queue instance, otherwise we can leave things in intermediate state on errors in nfqnl_recv_config(). This patch also fixes unknown and destroy command handling.
--- net/netfilter/nfnetlink_queue.c | 74 +++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 42 deletions(-)
net/netfilter/nfnetlink_queue.c +32 -42
@@ -1118,8 +1118,30 @@ nfqnl_recv_config(struct sock *ctnl, struct sk_buff *skb,struct nfqnl_instance *queue;struct net *net =sock_net (ctnl);struct nfnl_queue_net *q =nfnl_queue_pernet (net); +__u32 flags, mask;int ret =0 ; +/* Check if we support these flags in first place, dependencies should +* be there too not to break atomicity. +*/ +if (nfqa[NFQA_CFG_FLAGS ]) { +if (!nfqa[NFQA_CFG_MASK ]) +/* A mask is needed to specify which flags are being +* changed. +*/ +return -EINVAL ; + + flags =ntohl (nla_get_be32 (nfqa[NFQA_CFG_FLAGS ])); + mask =ntohl (nla_get_be32 (nfqa[NFQA_CFG_MASK ])); + +if (flags >=NFQA_CFG_F_MAX ) +return -EOPNOTSUPP ; +#if !IS_ENABLED (CONFIG_NETWORK_SECMARK ) +if (flags & mask &NFQA_CFG_F_SECCTX ) +return -EOPNOTSUPP ; +#endif + } +rcu_read_lock (); queue =instance_lookup (q, queue_num);if (queue && queue->peer_portid !=NETLINK_CB (skb).portid) {
@@ -1149,71 +1171,39 @@ nfqnl_recv_config(struct sock *ctnl, struct sk_buff *skb, goto err_out_unlock; }instance_destroy (q, queue); -break ; + goto err_out_unlock;case NFQNL_CFG_CMD_PF_BIND :case NFQNL_CFG_CMD_PF_UNBIND :/* Obsolete commands without queue context */ goto err_out_unlock;default : ret = -ENOTSUPP ; -break ; + goto err_out_unlock; } } +if (!queue) { + ret = -ENODEV ; + goto err_out_unlock; + } +if (nfqa[NFQA_CFG_PARAMS ]) { -struct nfqnl_msg_config_params *params; +struct nfqnl_msg_config_params *params + =nla_data (nfqa[NFQA_CFG_PARAMS ]); -if (!queue) { - ret = -ENODEV ; - goto err_out_unlock; - } - params =nla_data (nfqa[NFQA_CFG_PARAMS ]);nfqnl_set_mode (queue, params->copy_mode,ntohl (params->copy_range)); }if (nfqa[NFQA_CFG_QUEUE_MAXLEN ]) { -__be32 *queue_maxlen; +__be32 *queue_maxlen =nla_data (nfqa[NFQA_CFG_QUEUE_MAXLEN ]); -if (!queue) { - ret = -ENODEV ; - goto err_out_unlock; - } - queue_maxlen =nla_data (nfqa[NFQA_CFG_QUEUE_MAXLEN ]);spin_lock_bh (&queue->lock); queue->queue_maxlen =ntohl (*queue_maxlen);spin_unlock_bh (&queue->lock); }if (nfqa[NFQA_CFG_FLAGS ]) { -__u32 flags, mask; - -if (!queue) { - ret = -ENODEV ; - goto err_out_unlock; - } - -if (!nfqa[NFQA_CFG_MASK ]) { -/* A mask is needed to specify which flags are being -* changed. -*/ - ret = -EINVAL ; - goto err_out_unlock; - } - - flags =ntohl (nla_get_be32 (nfqa[NFQA_CFG_FLAGS ])); - mask =ntohl (nla_get_be32 (nfqa[NFQA_CFG_MASK ])); - -if (flags >=NFQA_CFG_F_MAX ) { - ret = -EOPNOTSUPP ; - goto err_out_unlock; - } -#if !IS_ENABLED (CONFIG_NETWORK_SECMARK ) -if (flags & mask &NFQA_CFG_F_SECCTX ) { - ret = -EOPNOTSUPP ; - goto err_out_unlock; - } -#endif spin_lock_bh (&queue->lock); queue->flags &= ~mask; queue->flags |= flags & mask;
Signature · 1 line
1.7.10.4
PATCH 3/3 netfilter: nfnetlink_queue: autoload nf_conntrack_netlink module NFQA_CFG_F_CONNTRACK config flag
In reply to Ken-ichirou MATSUZAWAThis patch enables to load nf_conntrack_netlink module if NFQA_CFG_F_CONNTRACK config flag is specified.
--- net/netfilter/nfnetlink_queue.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
net/netfilter/nfnetlink_queue.c +11 -0
@@ -1140,6 +1140,17 @@ nfqnl_recv_config(struct sock *ctnl, struct sk_buff *skb,if (flags & mask &NFQA_CFG_F_SECCTX )return -EOPNOTSUPP ;#endif +if ((flags & mask &NFQA_CFG_F_CONNTRACK ) && + !rcu_access_pointer (nfnl_ct_hook)) { +#ifdef CONFIG_MODULES +nfnl_unlock (NFNL_SUBSYS_QUEUE ); +request_module ("ip_conntrack_netlink" ); +nfnl_lock (NFNL_SUBSYS_QUEUE ); +if (rcu_access_pointer (nfnl_ct_hook)) +return -EAGAIN ; +#endif +return -EOPNOTSUPP ; + } }rcu_read_lock ();
Signature · 1 line
1.7.10.4
PATCH 1/1 netfilter: nf_conntrack_netlink: fix nf-nat module loaded checking
In reply to Ken-ichirou MATSUZAWA--- net/netfilter/nf_conntrack_netlink.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
net/netfilter/nf_conntrack_netlink.c +1 -1
@@ -1389,7 +1389,7 @@ ctnetlink_parse_nat_setup(struct nf_conn *ct, }nfnl_lock (NFNL_SUBSYS_CTNETLINK );rcu_read_lock (); -if (nfnetlink_parse_nat_setup_hook) +if (rcu_dereference (nfnetlink_parse_nat_setup_hook))return -EAGAIN ;#endif return -EOPNOTSUPP ;
Signature · 1 line
1.7.10.4
PATCH 1/1 netfilter: nf_conntrack_netlink: fix locks around helper module loading
In reply to Ken-ichirou MATSUZAWAThere are two paths for calling ctnetlink_change_helper which may call request_module() which requires nfnl lock, and may also call __nf_conntrack_helper_find() which needs rcu read lock.
- nfqnl_cb[NFQNL_MSG_VERDICT].call_rcu / nfqnl_recv_verdict()
- ctnl_cb[IPCTNL_MSG_CT_NEW].call / ctnetlink_new_conntrack()
These uses different lock mechanism, it seems we need to adjust it.
--- net/netfilter/nf_conntrack_netlink.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
net/netfilter/nf_conntrack_netlink.c +10 -1
@@ -1497,12 +1497,17 @@ ctnetlink_change_helper(struct nf_conn *ct, const struct nlattr * const cda[])if (helper ==NULL ) {#ifdef CONFIG_MODULES spin_unlock_bh (&nf_conntrack_expect_lock); - +rcu_read_unlock (); +nfnl_unlock (NFNL_SUBSYS_CTNETLINK );if (request_module ("nfct-helper-%s" , helpname) <0 ) { +nfnl_lock (NFNL_SUBSYS_CTNETLINK ); +rcu_read_lock ();spin_lock_bh (&nf_conntrack_expect_lock);return -EOPNOTSUPP ; } +nfnl_lock (NFNL_SUBSYS_CTNETLINK ); +rcu_read_lock ();spin_lock_bh (&nf_conntrack_expect_lock); helper =__nf_conntrack_helper_find (helpname,nf_ct_l3num (ct),nf_ct_protonum (ct));
@@ -1943,9 +1948,11 @@ ctnetlink_new_conntrack(struct sock *ctnl, struct sk_buff *skb, err = -EEXIST ; ct =nf_ct_tuplehash_to_ctrack (h);if (!(nlh->nlmsg_flags &NLM_F_EXCL )) { +rcu_read_lock ();spin_lock_bh (&nf_conntrack_expect_lock); err =ctnetlink_change_conntrack (ct, cda);spin_unlock_bh (&nf_conntrack_expect_lock); +rcu_read_unlock ();if (err ==0 ) { nf_conntrack_eventmask_report((1 << IPCT_REPLY) | (1 << IPCT_ASSURED) |
@@ -2287,7 +2294,9 @@ ctnetlink_glue_parse_ct(const struct nlattr *cda[], struct nf_conn *ct)return err; }if (cda[CTA_HELP ]) { +nfnl_lock (NFNL_SUBSYS_CTNETLINK ); err =ctnetlink_change_helper (ct, cda); +nfnl_unlock (NFNL_SUBSYS_CTNETLINK );if (err <0 )return err; }
Signature · 1 line
1.7.10.4
Pablo Neira Ayuso
maintainerRe: [PATCHv2 nf-next 0/4] netfilter: nfnetlink_log attach conntrack information
In reply to Ken-ichirou MATSUZAWAQuoted 7 lines · Ken-ichirou MATSUZAWA
Hi Pablo,
I rebased on top of your patches, would you review these?
You suggested it might be better to divide ct attaching
Kconfigto each of nfqueue and nflog, but I think it makes less sense now because of your rework. is it right?
Series applied, thanks for your patience.
Please, follow up with fixes in case of problems.