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

[Phil Sutter <phil@nwl.cc>]

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