Re: [nf-next PATCH v2 2/5] netfilter: nf_tables: Unlimit table name length

[Phil Sutter <phil@nwl.cc>]

Hi Pablo,

On Tue, Jul 25, 2017 at 06:10:02PM +0200, Pablo Neira Ayuso wrote:
> I think you can rename patch title to "allow table names up to 256
> chars". This is not unlimited anymore since v2.

OK.

> A couple more comments below.
> 
> On Mon, Jul 24, 2017 at 08:56:48PM +0200, Phil Sutter wrote:
[...]
> > --- a/include/uapi/linux/netfilter/nf_tables.h
> > +++ b/include/uapi/linux/netfilter/nf_tables.h
> > @@ -1,7 +1,7 @@
> >  #ifndef _LINUX_NF_TABLES_H
> >  #define _LINUX_NF_TABLES_H
> >  
> > -#define NFT_TABLE_MAXNAMELEN	32
> > +#define NFT_NAME_MAXLEN		256
> 
> I understand NFT_*_MAXNAMELEN per object is probably too much, but
> given this is uapi, I think we have to keep it around, ie.
> 
> #define NFT_NAME_MAXLEN		256
> #define NFT_TABLE_MAXNAMELEN    NFT_NAME_MAXLEN
> 
> And so on.

OK, I will change it. But using NFT_NAME_MAXLEN throughout kernel code
is OK? Or should I revert the policy struct changes?

> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index 7843efa33c598..cf12f63606aaf 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -428,7 +428,7 @@ nf_tables_chain_type_lookup(const struct nft_af_info *afi,
> >  
> >  static const struct nla_policy nft_table_policy[NFTA_TABLE_MAX + 1] = {
> >  	[NFTA_TABLE_NAME]	= { .type = NLA_STRING,
> > -				    .len = NFT_TABLE_MAXNAMELEN - 1 },
> > +				    .len = NFT_NAME_MAXLEN - 1 },
> >  	[NFTA_TABLE_FLAGS]	= { .type = NLA_U32 },
> >  };

This one, e.g.: Keeping the old NFT_TABLE_MAXNAMELEN around makes that
change needless in theory.

[...]
> > diff --git a/net/netfilter/nf_tables_trace.c b/net/netfilter/nf_tables_trace.c
> > index e1b15e7a5793f..e95098c1faaf0 100644
> > --- a/net/netfilter/nf_tables_trace.c
> > +++ b/net/netfilter/nf_tables_trace.c
> > @@ -175,7 +175,6 @@ void nft_trace_notify(struct nft_traceinfo *info)
> >  		return;
> >  
> >  	size = nlmsg_total_size(sizeof(struct nfgenmsg)) +
> > -		nla_total_size(NFT_TABLE_MAXNAMELEN) +
> >  		nla_total_size(NFT_CHAIN_MAXNAMELEN) +
> >  		nla_total_size_64bit(sizeof(__be64)) +	/* rule handle */
> >  		nla_total_size(sizeof(__be32)) +	/* trace type */
> > @@ -194,6 +193,9 @@ void nft_trace_notify(struct nft_traceinfo *info)
> >  		nla_total_size(sizeof(u32)) +		/* nfproto */
> >  		nla_total_size(sizeof(u32));		/* policy */
> >  
> > +	if (info->chain)
> > +		size += nla_total_size(strlen(info->chain->table->name));
> 
> Do we need a branch here? I think info->chain is always set in traces,
> right?

I wasn't sure, so I stuck to how nft_trace_notify() handles it later:

| if (info->chain) {
|         if (nla_put_string(skb, NFTA_TRACE_CHAIN,
|                            info->chain->name))
|                 goto nla_put_failure;
|         if (nla_put_string(skb, NFTA_TRACE_TABLE,
|                            info->chain->table->name))
|                 goto nla_put_failure;
| }

This made me believe there is a case where info->chain is not set.
Though looking at nft_do_chain() which is the only caller of
nft_trace_packet(), it seems like there is indeed always a chain (it is
dereferenced right at the top).

So probably nft_trace_notify() can unconditionally put NFTA_TRACE_CHAIN
and NFTA_TRACE_TABLE attributes. Maybe Florian knows more?

Another questionable part (in the 'Unlimit chain name length' patch) is
the existence check for info->verdict->chain: The relevant attribute is
created only if info->type is either NFT_TRACETYPE_RETURN or
NFT_TRACETYPE_RULE *and* info->verdict->code is either NFT_JUMP or
NFT_GOTO. Hard to tell whether this can be assumed to always exist.

Thanks, Phil