* [nf-next PATCH] netfilter: nf_tables: Report transactions' process info to user space @ 2017-05-15 14:51 Phil Sutter 2017-05-15 15:53 ` Pablo Neira Ayuso 0 siblings, 1 reply; 9+ messages in thread From: Phil Sutter @ 2017-05-15 14:51 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: netfilter-devel, Florian Westphal When committing a transaction, report PID and name of user space process which initiated it. Signed-off-by: Phil Sutter <phil@nwl.cc> --- include/uapi/linux/netfilter/nf_tables.h | 16 +++++++++++ net/netfilter/nf_tables_api.c | 49 ++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h index 683f6f88fcace..7c012690a5f02 100644 --- a/include/uapi/linux/netfilter/nf_tables.h +++ b/include/uapi/linux/netfilter/nf_tables.h @@ -90,6 +90,7 @@ enum nft_verdicts { * @NFT_MSG_GETOBJ: get a stateful object (enum nft_obj_attributes) * @NFT_MSG_DELOBJ: delete a stateful object (enum nft_obj_attributes) * @NFT_MSG_GETOBJ_RESET: get and reset a stateful object (enum nft_obj_attributes) + * @NFT_MSG_PROC_INFO: get info about user space process which initiated the transaction */ enum nf_tables_msg_types { NFT_MSG_NEWTABLE, @@ -114,6 +115,7 @@ enum nf_tables_msg_types { NFT_MSG_GETOBJ, NFT_MSG_DELOBJ, NFT_MSG_GETOBJ_RESET, + NFT_MSG_PROC_INFO, NFT_MSG_MAX, }; @@ -1375,4 +1377,18 @@ enum nft_ng_types { }; #define NFT_NG_MAX (__NFT_NG_MAX - 1) +/** + * enum nft_proc_info_attributes - nf_tables process information netlink attributes + * + * @NFTA_PROC_PID: process id (NLA_U32) + * @NFTA_PROC_NAME: process name (NLA_STRING) + */ +enum nft_proc_info_attributes { + NFTA_PROC_UNSPEC, + NFTA_PROC_PID, + NFTA_PROC_NAME, + __NFTA_PROC_MAX +}; +#define NFTA_PROC_MAX (__NFTA_PROC_MAX - 1) + #endif /* _LINUX_NF_TABLES_H */ diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index 1c6482d2c4dcf..7cdfbecbcd542 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -11,6 +11,7 @@ #include <linux/module.h> #include <linux/init.h> #include <linux/list.h> +#include <linux/sched.h> #include <linux/skbuff.h> #include <linux/netlink.h> #include <linux/netfilter.h> @@ -4748,6 +4749,49 @@ static void nf_tables_commit_release(struct nft_trans *trans) kfree(trans); } +static void nf_tables_proc_notify(const struct nft_ctx *ctx) +{ + char buf[TASK_COMM_LEN]; + struct nfgenmsg *nfmsg; + struct nlmsghdr *nlh; + struct sk_buff *skb; + int event; + + if (!ctx->report && + !nfnetlink_has_listeners(ctx->net, NFNLGRP_NFTABLES)) + return; + + skb = nlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL); + if (skb == NULL) + goto err; + + event = nfnl_msg_type(NFNL_SUBSYS_NFTABLES, NFT_MSG_PROC_INFO); + nlh = nlmsg_put(skb, ctx->portid, ctx->seq, + event, sizeof(struct nfgenmsg), 0); + if (nlh == NULL) + goto nla_put_failure; + + nfmsg = nlmsg_data(nlh); + nfmsg->nfgen_family = ctx->afi->family; + nfmsg->version = NFNETLINK_V0; + nfmsg->res_id = htons(ctx->net->nft.base_seq & 0xffff); + + if (nla_put_be32(skb, NFTA_PROC_PID, htonl(task_pid_nr(current))) || + nla_put_string(skb, NFTA_PROC_NAME, get_task_comm(buf, current))) + goto nla_put_failure; + + nlmsg_end(skb, nlh); + nfnetlink_send(skb, ctx->net, ctx->portid, + NFNLGRP_NFTABLES, ctx->report, GFP_KERNEL); + return; + +nla_put_failure: + nlmsg_trim(skb, nlh); + kfree_skb(skb); +err: + nfnetlink_set_err(ctx->net, ctx->portid, NFNLGRP_NFTABLES, -ENOBUFS); +} + static int nf_tables_commit(struct net *net, struct sk_buff *skb) { struct nft_trans *trans, *next; @@ -4764,6 +4808,11 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb) */ synchronize_rcu(); + trans = list_first_entry_or_null(&net->nft.commit_list, + struct nft_trans, list); + if (trans) + nf_tables_proc_notify(&trans->ctx); + list_for_each_entry_safe(trans, next, &net->nft.commit_list, list) { switch (trans->msg_type) { case NFT_MSG_NEWTABLE: -- 2.11.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [nf-next PATCH] netfilter: nf_tables: Report transactions' process info to user space 2017-05-15 14:51 [nf-next PATCH] netfilter: nf_tables: Report transactions' process info to user space Phil Sutter @ 2017-05-15 15:53 ` Pablo Neira Ayuso 2017-05-15 16:44 ` Phil Sutter 0 siblings, 1 reply; 9+ messages in thread From: Pablo Neira Ayuso @ 2017-05-15 15:53 UTC (permalink / raw) To: Phil Sutter; +Cc: netfilter-devel, Florian Westphal On Mon, May 15, 2017 at 04:51:49PM +0200, Phil Sutter wrote: > When committing a transaction, report PID and name of user space process > which initiated it. > > Signed-off-by: Phil Sutter <phil@nwl.cc> > --- > include/uapi/linux/netfilter/nf_tables.h | 16 +++++++++++ > net/netfilter/nf_tables_api.c | 49 ++++++++++++++++++++++++++++++++ > 2 files changed, 65 insertions(+) > > diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h > index 683f6f88fcace..7c012690a5f02 100644 > --- a/include/uapi/linux/netfilter/nf_tables.h > +++ b/include/uapi/linux/netfilter/nf_tables.h > @@ -90,6 +90,7 @@ enum nft_verdicts { > * @NFT_MSG_GETOBJ: get a stateful object (enum nft_obj_attributes) > * @NFT_MSG_DELOBJ: delete a stateful object (enum nft_obj_attributes) > * @NFT_MSG_GETOBJ_RESET: get and reset a stateful object (enum nft_obj_attributes) > + * @NFT_MSG_PROC_INFO: get info about user space process which initiated the transaction > */ > enum nf_tables_msg_types { > NFT_MSG_NEWTABLE, > @@ -114,6 +115,7 @@ enum nf_tables_msg_types { > NFT_MSG_GETOBJ, > NFT_MSG_DELOBJ, > NFT_MSG_GETOBJ_RESET, > + NFT_MSG_PROC_INFO, No need for a new message. You can place this into existing the NEWGEN messages. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [nf-next PATCH] netfilter: nf_tables: Report transactions' process info to user space 2017-05-15 15:53 ` Pablo Neira Ayuso @ 2017-05-15 16:44 ` Phil Sutter 2017-05-15 17:54 ` Pablo Neira Ayuso 0 siblings, 1 reply; 9+ messages in thread From: Phil Sutter @ 2017-05-15 16:44 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: netfilter-devel, Florian Westphal On Mon, May 15, 2017 at 05:53:31PM +0200, Pablo Neira Ayuso wrote: > On Mon, May 15, 2017 at 04:51:49PM +0200, Phil Sutter wrote: > > When committing a transaction, report PID and name of user space process > > which initiated it. > > > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > --- > > include/uapi/linux/netfilter/nf_tables.h | 16 +++++++++++ > > net/netfilter/nf_tables_api.c | 49 ++++++++++++++++++++++++++++++++ > > 2 files changed, 65 insertions(+) > > > > diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h > > index 683f6f88fcace..7c012690a5f02 100644 > > --- a/include/uapi/linux/netfilter/nf_tables.h > > +++ b/include/uapi/linux/netfilter/nf_tables.h > > @@ -90,6 +90,7 @@ enum nft_verdicts { > > * @NFT_MSG_GETOBJ: get a stateful object (enum nft_obj_attributes) > > * @NFT_MSG_DELOBJ: delete a stateful object (enum nft_obj_attributes) > > * @NFT_MSG_GETOBJ_RESET: get and reset a stateful object (enum nft_obj_attributes) > > + * @NFT_MSG_PROC_INFO: get info about user space process which initiated the transaction > > */ > > enum nf_tables_msg_types { > > NFT_MSG_NEWTABLE, > > @@ -114,6 +115,7 @@ enum nf_tables_msg_types { > > NFT_MSG_GETOBJ, > > NFT_MSG_DELOBJ, > > NFT_MSG_GETOBJ_RESET, > > + NFT_MSG_PROC_INFO, > > No need for a new message. You can place this into existing the NEWGEN > messages. But that message is sent last and so at the time nft sees it, the events will have been printed already, no? Thanks, Phil ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [nf-next PATCH] netfilter: nf_tables: Report transactions' process info to user space 2017-05-15 16:44 ` Phil Sutter @ 2017-05-15 17:54 ` Pablo Neira Ayuso 2017-05-19 10:41 ` Phil Sutter 0 siblings, 1 reply; 9+ messages in thread From: Pablo Neira Ayuso @ 2017-05-15 17:54 UTC (permalink / raw) To: Phil Sutter, netfilter-devel, Florian Westphal On Mon, May 15, 2017 at 06:44:32PM +0200, Phil Sutter wrote: > On Mon, May 15, 2017 at 05:53:31PM +0200, Pablo Neira Ayuso wrote: > > On Mon, May 15, 2017 at 04:51:49PM +0200, Phil Sutter wrote: > > > When committing a transaction, report PID and name of user space process > > > which initiated it. > > > > > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > > --- > > > include/uapi/linux/netfilter/nf_tables.h | 16 +++++++++++ > > > net/netfilter/nf_tables_api.c | 49 ++++++++++++++++++++++++++++++++ > > > 2 files changed, 65 insertions(+) > > > > > > diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h > > > index 683f6f88fcace..7c012690a5f02 100644 > > > --- a/include/uapi/linux/netfilter/nf_tables.h > > > +++ b/include/uapi/linux/netfilter/nf_tables.h > > > @@ -90,6 +90,7 @@ enum nft_verdicts { > > > * @NFT_MSG_GETOBJ: get a stateful object (enum nft_obj_attributes) > > > * @NFT_MSG_DELOBJ: delete a stateful object (enum nft_obj_attributes) > > > * @NFT_MSG_GETOBJ_RESET: get and reset a stateful object (enum nft_obj_attributes) > > > + * @NFT_MSG_PROC_INFO: get info about user space process which initiated the transaction > > > */ > > > enum nf_tables_msg_types { > > > NFT_MSG_NEWTABLE, > > > @@ -114,6 +115,7 @@ enum nf_tables_msg_types { > > > NFT_MSG_GETOBJ, > > > NFT_MSG_DELOBJ, > > > NFT_MSG_GETOBJ_RESET, > > > + NFT_MSG_PROC_INFO, > > > > No need for a new message. You can place this into existing the NEWGEN > > messages. > > But that message is sent last and so at the time nft sees it, the events > will have been printed already, no? This is an event, so it is asynchronous. From a timely perspective, we get nothing if we send it just a bit before. I suspect the problem is the lack of context, ie. access ctx->portid, ctx->seq and ctx->report, then we should take this from the original initial netlink message header coming in the batch (see nfnetlink_rcv_batch() in nfnetlink.c). Look, we send a batch from userspace that look like this: netlink header [ type = NFNL_MSG_BATCH_BEGIN ] nfnetlink header payload netlink header [ type = nfnl_msg_type(NFNL_SUBSYS_NFTABLES, NFT_MSG_NEWRULE) ] nfnetlink header payload ... netlink header [ type = NFNL_MSG_BATCH_END ] nfnetlink header There's an initial NFNL_MSG_BATCH_BEGIN and a trailing NFNL_MSG_BATCH_END. No trailing NFNL_MSG_BATCH_END means abort transaction, it provides a way to test if a ruleset applies cleanly, similar to the -C command in iptables (this doesn't exist in nft userspace though yet, it would be good to add this). I think you should extract this relevant information you need from the initial NFNL_MSG_BATCH_BEGIN netlink header, so semantically, this applies to the result of the batch, ie. you use it from the NEWGEN message. We should pass some structure to nf_tables_commit() that provides this global information, probably in an initial patch in the series. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [nf-next PATCH] netfilter: nf_tables: Report transactions' process info to user space 2017-05-15 17:54 ` Pablo Neira Ayuso @ 2017-05-19 10:41 ` Phil Sutter 2017-05-30 12:12 ` Pablo Neira Ayuso 0 siblings, 1 reply; 9+ messages in thread From: Phil Sutter @ 2017-05-19 10:41 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: netfilter-devel, Florian Westphal On Mon, May 15, 2017 at 07:54:44PM +0200, Pablo Neira Ayuso wrote: > On Mon, May 15, 2017 at 06:44:32PM +0200, Phil Sutter wrote: > > On Mon, May 15, 2017 at 05:53:31PM +0200, Pablo Neira Ayuso wrote: > > > On Mon, May 15, 2017 at 04:51:49PM +0200, Phil Sutter wrote: > > > > When committing a transaction, report PID and name of user space process > > > > which initiated it. > > > > > > > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > > > --- > > > > include/uapi/linux/netfilter/nf_tables.h | 16 +++++++++++ > > > > net/netfilter/nf_tables_api.c | 49 ++++++++++++++++++++++++++++++++ > > > > 2 files changed, 65 insertions(+) > > > > > > > > diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h > > > > index 683f6f88fcace..7c012690a5f02 100644 > > > > --- a/include/uapi/linux/netfilter/nf_tables.h > > > > +++ b/include/uapi/linux/netfilter/nf_tables.h > > > > @@ -90,6 +90,7 @@ enum nft_verdicts { > > > > * @NFT_MSG_GETOBJ: get a stateful object (enum nft_obj_attributes) > > > > * @NFT_MSG_DELOBJ: delete a stateful object (enum nft_obj_attributes) > > > > * @NFT_MSG_GETOBJ_RESET: get and reset a stateful object (enum nft_obj_attributes) > > > > + * @NFT_MSG_PROC_INFO: get info about user space process which initiated the transaction > > > > */ > > > > enum nf_tables_msg_types { > > > > NFT_MSG_NEWTABLE, > > > > @@ -114,6 +115,7 @@ enum nf_tables_msg_types { > > > > NFT_MSG_GETOBJ, > > > > NFT_MSG_DELOBJ, > > > > NFT_MSG_GETOBJ_RESET, > > > > + NFT_MSG_PROC_INFO, > > > > > > No need for a new message. You can place this into existing the NEWGEN > > > messages. > > > > But that message is sent last and so at the time nft sees it, the events > > will have been printed already, no? > > This is an event, so it is asynchronous. From a timely perspective, we > get nothing if we send it just a bit before. My concern was that it's not possible to keep the additional information in the same line as the events it is interesting for. So in order to not introduce caching in 'nft monitor', the new format would be something like this: | add chain ip t c | new generation 3 by process 2841 (nft) | add table ip t2 | add chain ip t2 c | add rule ip t2 c counter packets 0 bytes 0 | new generation 4 by process 2851 (nft) Is this fine with you, or do you have something different in mind? > I suspect the problem is the lack of context, ie. access ctx->portid, > ctx->seq and ctx->report, then we should take this from the original > initial netlink message header coming in the batch (see > nfnetlink_rcv_batch() in nfnetlink.c). That's not necessary? My current PoC looks like this: | @@ -4538,7 +4539,9 @@ static int nf_tables_fill_gen_info(struct sk_buff *skb, struct net *net, | nfmsg->version = NFNETLINK_V0; | nfmsg->res_id = htons(net->nft.base_seq & 0xffff); | | - if (nla_put_be32(skb, NFTA_GEN_ID, htonl(net->nft.base_seq))) | + if (nla_put_be32(skb, NFTA_GEN_ID, htonl(net->nft.base_seq)) || | + nla_put_be32(skb, NFTA_GEN_PROC_PID, htonl(task_pid_nr(current))) || | + nla_put_string(skb, NFTA_GEN_PROC_NAME, get_task_comm(buf, current))) | goto nla_put_failure; | | nlmsg_end(skb, nlh); Or do you think it's not safe to access 'current' at this point? Thanks, Phil ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [nf-next PATCH] netfilter: nf_tables: Report transactions' process info to user space 2017-05-19 10:41 ` Phil Sutter @ 2017-05-30 12:12 ` Pablo Neira Ayuso 2017-05-30 16:21 ` Phil Sutter 0 siblings, 1 reply; 9+ messages in thread From: Pablo Neira Ayuso @ 2017-05-30 12:12 UTC (permalink / raw) To: Phil Sutter, netfilter-devel, Florian Westphal On Fri, May 19, 2017 at 12:41:28PM +0200, Phil Sutter wrote: > On Mon, May 15, 2017 at 07:54:44PM +0200, Pablo Neira Ayuso wrote: > > On Mon, May 15, 2017 at 06:44:32PM +0200, Phil Sutter wrote: > > > On Mon, May 15, 2017 at 05:53:31PM +0200, Pablo Neira Ayuso wrote: > > > > On Mon, May 15, 2017 at 04:51:49PM +0200, Phil Sutter wrote: > > > > > When committing a transaction, report PID and name of user space process > > > > > which initiated it. > > > > > > > > > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > > > > --- > > > > > include/uapi/linux/netfilter/nf_tables.h | 16 +++++++++++ > > > > > net/netfilter/nf_tables_api.c | 49 ++++++++++++++++++++++++++++++++ > > > > > 2 files changed, 65 insertions(+) > > > > > > > > > > diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h > > > > > index 683f6f88fcace..7c012690a5f02 100644 > > > > > --- a/include/uapi/linux/netfilter/nf_tables.h > > > > > +++ b/include/uapi/linux/netfilter/nf_tables.h > > > > > @@ -90,6 +90,7 @@ enum nft_verdicts { > > > > > * @NFT_MSG_GETOBJ: get a stateful object (enum nft_obj_attributes) > > > > > * @NFT_MSG_DELOBJ: delete a stateful object (enum nft_obj_attributes) > > > > > * @NFT_MSG_GETOBJ_RESET: get and reset a stateful object (enum nft_obj_attributes) > > > > > + * @NFT_MSG_PROC_INFO: get info about user space process which initiated the transaction > > > > > */ > > > > > enum nf_tables_msg_types { > > > > > NFT_MSG_NEWTABLE, > > > > > @@ -114,6 +115,7 @@ enum nf_tables_msg_types { > > > > > NFT_MSG_GETOBJ, > > > > > NFT_MSG_DELOBJ, > > > > > NFT_MSG_GETOBJ_RESET, > > > > > + NFT_MSG_PROC_INFO, > > > > > > > > No need for a new message. You can place this into existing the NEWGEN > > > > messages. > > > > > > But that message is sent last and so at the time nft sees it, the events > > > will have been printed already, no? > > > > This is an event, so it is asynchronous. From a timely perspective, we > > get nothing if we send it just a bit before. > > My concern was that it's not possible to keep the additional information > in the same line as the events it is interesting for. So in order to not > introduce caching in 'nft monitor', the new format would be something > like this: > > | add chain ip t c > | new generation 3 by process 2841 (nft) > | add table ip t2 > | add chain ip t2 c > | add rule ip t2 c counter packets 0 bytes 0 > | new generation 4 by process 2851 (nft) > > Is this fine with you, or do you have something different in mind? This is ok, I would just print it like this: # new generation 3 by process 2841 (nft) starting with '#', since this is not a valid nft syntax, but just context information. > > I suspect the problem is the lack of context, ie. access ctx->portid, > > ctx->seq and ctx->report, then we should take this from the original > > initial netlink message header coming in the batch (see > > nfnetlink_rcv_batch() in nfnetlink.c). > > That's not necessary? My current PoC looks like this: What I mean is that that we should use the heading netlink message as netlink context (portid, flags) for the NEWGEN message. This is currently broken. I'll send a patch to fix this, so you can send a follow up of this on top of this. > | @@ -4538,7 +4539,9 @@ static int nf_tables_fill_gen_info(struct sk_buff *skb, struct net *net, > | nfmsg->version = NFNETLINK_V0; > | nfmsg->res_id = htons(net->nft.base_seq & 0xffff); > | > | - if (nla_put_be32(skb, NFTA_GEN_ID, htonl(net->nft.base_seq))) > | + if (nla_put_be32(skb, NFTA_GEN_ID, htonl(net->nft.base_seq)) || > | + nla_put_be32(skb, NFTA_GEN_PROC_PID, htonl(task_pid_nr(current))) || > | + nla_put_string(skb, NFTA_GEN_PROC_NAME, get_task_comm(buf, current))) > | goto nla_put_failure; > | > | nlmsg_end(skb, nlh); > > Or do you think it's not safe to access 'current' at this point? I'm not very familiar with the usage of 'current' in this case, so it would be good to double check on the implications of this. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [nf-next PATCH] netfilter: nf_tables: Report transactions' process info to user space 2017-05-30 12:12 ` Pablo Neira Ayuso @ 2017-05-30 16:21 ` Phil Sutter 2017-05-30 19:19 ` Pablo Neira Ayuso 2017-06-06 11:39 ` Pablo Neira Ayuso 0 siblings, 2 replies; 9+ messages in thread From: Phil Sutter @ 2017-05-30 16:21 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: netfilter-devel, Florian Westphal On Tue, May 30, 2017 at 02:12:11PM +0200, Pablo Neira Ayuso wrote: > On Fri, May 19, 2017 at 12:41:28PM +0200, Phil Sutter wrote: > > On Mon, May 15, 2017 at 07:54:44PM +0200, Pablo Neira Ayuso wrote: > > > On Mon, May 15, 2017 at 06:44:32PM +0200, Phil Sutter wrote: > > > > On Mon, May 15, 2017 at 05:53:31PM +0200, Pablo Neira Ayuso wrote: > > > > > On Mon, May 15, 2017 at 04:51:49PM +0200, Phil Sutter wrote: > > > > > > When committing a transaction, report PID and name of user space process > > > > > > which initiated it. > > > > > > > > > > > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > > > > > --- > > > > > > include/uapi/linux/netfilter/nf_tables.h | 16 +++++++++++ > > > > > > net/netfilter/nf_tables_api.c | 49 ++++++++++++++++++++++++++++++++ > > > > > > 2 files changed, 65 insertions(+) > > > > > > > > > > > > diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h > > > > > > index 683f6f88fcace..7c012690a5f02 100644 > > > > > > --- a/include/uapi/linux/netfilter/nf_tables.h > > > > > > +++ b/include/uapi/linux/netfilter/nf_tables.h > > > > > > @@ -90,6 +90,7 @@ enum nft_verdicts { > > > > > > * @NFT_MSG_GETOBJ: get a stateful object (enum nft_obj_attributes) > > > > > > * @NFT_MSG_DELOBJ: delete a stateful object (enum nft_obj_attributes) > > > > > > * @NFT_MSG_GETOBJ_RESET: get and reset a stateful object (enum nft_obj_attributes) > > > > > > + * @NFT_MSG_PROC_INFO: get info about user space process which initiated the transaction > > > > > > */ > > > > > > enum nf_tables_msg_types { > > > > > > NFT_MSG_NEWTABLE, > > > > > > @@ -114,6 +115,7 @@ enum nf_tables_msg_types { > > > > > > NFT_MSG_GETOBJ, > > > > > > NFT_MSG_DELOBJ, > > > > > > NFT_MSG_GETOBJ_RESET, > > > > > > + NFT_MSG_PROC_INFO, > > > > > > > > > > No need for a new message. You can place this into existing the NEWGEN > > > > > messages. > > > > > > > > But that message is sent last and so at the time nft sees it, the events > > > > will have been printed already, no? > > > > > > This is an event, so it is asynchronous. From a timely perspective, we > > > get nothing if we send it just a bit before. > > > > My concern was that it's not possible to keep the additional information > > in the same line as the events it is interesting for. So in order to not > > introduce caching in 'nft monitor', the new format would be something > > like this: > > > > | add chain ip t c > > | new generation 3 by process 2841 (nft) > > | add table ip t2 > > | add chain ip t2 c > > | add rule ip t2 c counter packets 0 bytes 0 > > | new generation 4 by process 2851 (nft) > > > > Is this fine with you, or do you have something different in mind? > > This is ok, I would just print it like this: > > # new generation 3 by process 2841 (nft) > > starting with '#', since this is not a valid nft syntax, but just > context information. ACK. > > > I suspect the problem is the lack of context, ie. access ctx->portid, > > > ctx->seq and ctx->report, then we should take this from the original > > > initial netlink message header coming in the batch (see > > > nfnetlink_rcv_batch() in nfnetlink.c). > > > > That's not necessary? My current PoC looks like this: > > What I mean is that that we should use the heading netlink message as > netlink context (portid, flags) for the NEWGEN message. This is > currently broken. I'll send a patch to fix this, so you can send a > follow up of this on top of this. OK, thanks! > > | @@ -4538,7 +4539,9 @@ static int nf_tables_fill_gen_info(struct sk_buff *skb, struct net *net, > > | nfmsg->version = NFNETLINK_V0; > > | nfmsg->res_id = htons(net->nft.base_seq & 0xffff); > > | > > | - if (nla_put_be32(skb, NFTA_GEN_ID, htonl(net->nft.base_seq))) > > | + if (nla_put_be32(skb, NFTA_GEN_ID, htonl(net->nft.base_seq)) || > > | + nla_put_be32(skb, NFTA_GEN_PROC_PID, htonl(task_pid_nr(current))) || > > | + nla_put_string(skb, NFTA_GEN_PROC_NAME, get_task_comm(buf, current))) > > | goto nla_put_failure; > > | > > | nlmsg_end(skb, nlh); > > > > Or do you think it's not safe to access 'current' at this point? > > I'm not very familiar with the usage of 'current' in this case, so it > would be good to double check on the implications of this. It is only valid in process context. I suspected the whole transaction to happen with rtnl lock held, so it should be safe. But I'll check this before preparing a real patch. If I was wrong, I can still cache it's value at transaction start. Thanks, Phil ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [nf-next PATCH] netfilter: nf_tables: Report transactions' process info to user space 2017-05-30 16:21 ` Phil Sutter @ 2017-05-30 19:19 ` Pablo Neira Ayuso 2017-06-06 11:39 ` Pablo Neira Ayuso 1 sibling, 0 replies; 9+ messages in thread From: Pablo Neira Ayuso @ 2017-05-30 19:19 UTC (permalink / raw) To: Phil Sutter, netfilter-devel, Florian Westphal On Tue, May 30, 2017 at 06:21:49PM +0200, Phil Sutter wrote: > On Tue, May 30, 2017 at 02:12:11PM +0200, Pablo Neira Ayuso wrote: > > On Fri, May 19, 2017 at 12:41:28PM +0200, Phil Sutter wrote: > > > On Mon, May 15, 2017 at 07:54:44PM +0200, Pablo Neira Ayuso wrote: > > > > On Mon, May 15, 2017 at 06:44:32PM +0200, Phil Sutter wrote: > > > > > On Mon, May 15, 2017 at 05:53:31PM +0200, Pablo Neira Ayuso wrote: > > > > > > On Mon, May 15, 2017 at 04:51:49PM +0200, Phil Sutter wrote: > > > > > > > When committing a transaction, report PID and name of user space process > > > > > > > which initiated it. > > > > > > > > > > > > > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > > > > > > --- > > > > > > > include/uapi/linux/netfilter/nf_tables.h | 16 +++++++++++ > > > > > > > net/netfilter/nf_tables_api.c | 49 ++++++++++++++++++++++++++++++++ > > > > > > > 2 files changed, 65 insertions(+) > > > > > > > > > > > > > > diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h > > > > > > > index 683f6f88fcace..7c012690a5f02 100644 > > > > > > > --- a/include/uapi/linux/netfilter/nf_tables.h > > > > > > > +++ b/include/uapi/linux/netfilter/nf_tables.h > > > > > > > @@ -90,6 +90,7 @@ enum nft_verdicts { > > > > > > > * @NFT_MSG_GETOBJ: get a stateful object (enum nft_obj_attributes) > > > > > > > * @NFT_MSG_DELOBJ: delete a stateful object (enum nft_obj_attributes) > > > > > > > * @NFT_MSG_GETOBJ_RESET: get and reset a stateful object (enum nft_obj_attributes) > > > > > > > + * @NFT_MSG_PROC_INFO: get info about user space process which initiated the transaction > > > > > > > */ > > > > > > > enum nf_tables_msg_types { > > > > > > > NFT_MSG_NEWTABLE, > > > > > > > @@ -114,6 +115,7 @@ enum nf_tables_msg_types { > > > > > > > NFT_MSG_GETOBJ, > > > > > > > NFT_MSG_DELOBJ, > > > > > > > NFT_MSG_GETOBJ_RESET, > > > > > > > + NFT_MSG_PROC_INFO, > > > > > > > > > > > > No need for a new message. You can place this into existing the NEWGEN > > > > > > messages. > > > > > > > > > > But that message is sent last and so at the time nft sees it, the events > > > > > will have been printed already, no? > > > > > > > > This is an event, so it is asynchronous. From a timely perspective, we > > > > get nothing if we send it just a bit before. > > > > > > My concern was that it's not possible to keep the additional information > > > in the same line as the events it is interesting for. So in order to not > > > introduce caching in 'nft monitor', the new format would be something > > > like this: > > > > > > | add chain ip t c > > > | new generation 3 by process 2841 (nft) > > > | add table ip t2 > > > | add chain ip t2 c > > > | add rule ip t2 c counter packets 0 bytes 0 > > > | new generation 4 by process 2851 (nft) > > > > > > Is this fine with you, or do you have something different in mind? > > > > This is ok, I would just print it like this: > > > > # new generation 3 by process 2841 (nft) > > > > starting with '#', since this is not a valid nft syntax, but just > > context information. > > ACK. > > > > > I suspect the problem is the lack of context, ie. access ctx->portid, > > > > ctx->seq and ctx->report, then we should take this from the original > > > > initial netlink message header coming in the batch (see > > > > nfnetlink_rcv_batch() in nfnetlink.c). > > > > > > That's not necessary? My current PoC looks like this: > > > > What I mean is that that we should use the heading netlink message as > > netlink context (portid, flags) for the NEWGEN message. This is > > currently broken. I'll send a patch to fix this, so you can send a > > follow up of this on top of this. > > OK, thanks! > > > > | @@ -4538,7 +4539,9 @@ static int nf_tables_fill_gen_info(struct sk_buff *skb, struct net *net, > > > | nfmsg->version = NFNETLINK_V0; > > > | nfmsg->res_id = htons(net->nft.base_seq & 0xffff); > > > | > > > | - if (nla_put_be32(skb, NFTA_GEN_ID, htonl(net->nft.base_seq))) > > > | + if (nla_put_be32(skb, NFTA_GEN_ID, htonl(net->nft.base_seq)) || > > > | + nla_put_be32(skb, NFTA_GEN_PROC_PID, htonl(task_pid_nr(current))) || > > > | + nla_put_string(skb, NFTA_GEN_PROC_NAME, get_task_comm(buf, current))) > > > | goto nla_put_failure; > > > | > > > | nlmsg_end(skb, nlh); > > > > > > Or do you think it's not safe to access 'current' at this point? > > > > I'm not very familiar with the usage of 'current' in this case, so it > > would be good to double check on the implications of this. > > It is only valid in process context. I suspected the whole transaction > to happen with rtnl lock held, so it should be safe. But I'll check this > before preparing a real patch. If I was wrong, I can still cache it's > value at transaction start. With nfnl_lock, it's a per-netfilter-subsystem mutex which is sort of similar to rtnl_lock. Yes, all transactions happen from process context, if that is the only restriction, then we're all good. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [nf-next PATCH] netfilter: nf_tables: Report transactions' process info to user space 2017-05-30 16:21 ` Phil Sutter 2017-05-30 19:19 ` Pablo Neira Ayuso @ 2017-06-06 11:39 ` Pablo Neira Ayuso 1 sibling, 0 replies; 9+ messages in thread From: Pablo Neira Ayuso @ 2017-06-06 11:39 UTC (permalink / raw) To: Phil Sutter, netfilter-devel, Florian Westphal On Tue, May 30, 2017 at 06:21:49PM +0200, Phil Sutter wrote: > On Tue, May 30, 2017 at 02:12:11PM +0200, Pablo Neira Ayuso wrote: > > On Fri, May 19, 2017 at 12:41:28PM +0200, Phil Sutter wrote: > > What I mean is that that we should use the heading netlink message as > > netlink context (portid, flags) for the NEWGEN message. This is > > currently broken. I'll send a patch to fix this, so you can send a > > follow up of this on top of this. > > OK, thanks! Actually the existing code for NEWGEN looks good. We pass the original skbuff to ->commit(), so we can fetch from the _BEGIN netlink message coming in first place in the batch the context we need. > > > | @@ -4538,7 +4539,9 @@ static int nf_tables_fill_gen_info(struct sk_buff *skb, struct net *net, > > > | nfmsg->version = NFNETLINK_V0; > > > | nfmsg->res_id = htons(net->nft.base_seq & 0xffff); > > > | > > > | - if (nla_put_be32(skb, NFTA_GEN_ID, htonl(net->nft.base_seq))) > > > | + if (nla_put_be32(skb, NFTA_GEN_ID, htonl(net->nft.base_seq)) || > > > | + nla_put_be32(skb, NFTA_GEN_PROC_PID, htonl(task_pid_nr(current))) || > > > | + nla_put_string(skb, NFTA_GEN_PROC_NAME, get_task_comm(buf, current))) > > > | goto nla_put_failure; So you only need to add these missing to the NEWGEN message. Thanks! ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-06-06 11:39 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-05-15 14:51 [nf-next PATCH] netfilter: nf_tables: Report transactions' process info to user space Phil Sutter 2017-05-15 15:53 ` Pablo Neira Ayuso 2017-05-15 16:44 ` Phil Sutter 2017-05-15 17:54 ` Pablo Neira Ayuso 2017-05-19 10:41 ` Phil Sutter 2017-05-30 12:12 ` Pablo Neira Ayuso 2017-05-30 16:21 ` Phil Sutter 2017-05-30 19:19 ` Pablo Neira Ayuso 2017-06-06 11:39 ` Pablo Neira Ayuso
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.