[nf-next PATCH] netfilter: nf_tables: Report transactions' process info to user space

[nf-next PATCH] netfilter: nf_tables: Report transactions' process info to user space

[Phil Sutter]

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

Re: [nf-next PATCH] netfilter: nf_tables: Report transactions' process info to user space

[Pablo Neira Ayuso]

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.

Re: [nf-next PATCH] netfilter: nf_tables: Report transactions' process info to user space

[Phil Sutter]

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

Re: [nf-next PATCH] netfilter: nf_tables: Report transactions' process info to user space

[Pablo Neira Ayuso]

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.

Re: [nf-next PATCH] netfilter: nf_tables: Report transactions' process info to user space

[Phil Sutter]

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

Re: [nf-next PATCH] netfilter: nf_tables: Report transactions' process info to user space

[Pablo Neira Ayuso]

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.

Re: [nf-next PATCH] netfilter: nf_tables: Report transactions' process info to user space

[Phil Sutter]

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

Re: [nf-next PATCH] netfilter: nf_tables: Report transactions' process info to user space

[Pablo Neira Ayuso]

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.

Re: [nf-next PATCH] netfilter: nf_tables: Report transactions' process info to user space

[Pablo Neira Ayuso]

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!