Date:   Thu, 11 Apr 2019 21:26:47 +0200
From:   Pablo Neira Ayuso <pablo@...filter.org>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     netfilter-devel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH RFC 4/4] netfilter: nf_tables: add netlink description

On Fri, Mar 29, 2019 at 11:59:46AM +0100, Johannes Berg wrote:
> On Wed, 2018-02-07 at 02:37 +0100, Pablo Neira Ayuso wrote:
> > 
> > +static const struct nl_desc_attr nft_nldesc_table_attrs[NFTA_TABLE_MAX + 1] = {
> > +	NLDESC_ATTR_STRING(NFTA_TABLE_NAME, NFT_NAME_MAXLEN - 1),
> > +	NLDESC_ATTR_U32_MAX(NFTA_TABLE_FLAGS, NFT_TABLE_F_DORMANT),
> > +	NLDESC_ATTR_U32(NFTA_TABLE_USE),
> > +	NLDESC_ATTR_U64(NFTA_TABLE_HANDLE),
> > +	NLDESC_ATTR_PAD(NFTA_TABLE_PAD),
> > +};
> 
> This part I don't really like so much. Yes, it's >1yo, so we didn't have
> all the stuff at the time.

I'm glad to receive feedback, even 1 year later :-)

> But this is _lots_ of code, and most of it is already encoded in the
> nla_policy for all those attributes. I think we really need to find a
> way to unify the two sets of data.

We could probably extract this from the nla_policy if we extend it. I
can have a look...

> I'd actually be happy to *just* expose the policy with all its nested
> elements/objects in there. That'd probably be also almost enough for
> you?

Exposing commands is also important, I know of people poking for
commands to see if they are available or not.

I think we should full description to userspace, not only policies.

> And that way you don't need to maintain the same descriptions twice
> (and we *will* get it wrong if we do that).
> 
> For genl sub-families, I'd also say that the whole thing could be
> exposed within the genl family, so I think we'd want to express this as
> some kind of helper logic to export the policy data.
> 
> I'm not entirely sure what the whole story with your "objects" is
> though. Aren't those just nested attributes of sort?
> 
> I see e.g.
> 
> > 
> > +static const struct nl_desc_attr nft_nldesc_set_desc_attrs[NFTA_SET_DESC_MAX + 1] = {
> > +       NLDESC_ATTR_U32(NFTA_SET_DESC_SIZE),
> > +};
> 
> This is policy data - like I said above we should unify the two.
> 
> > +static const struct nl_desc_obj nft_nldesc_set_desc[] = {
> > +       NLDESC_OBJ(NFT_SET_DESC, nft_nldesc_set_desc_attrs, NFTA_SET_DESC_MAX),
> 
> This is some kind of "object", which I'm not sure I understand, but
> NFTA_SET_DESC_MAX is just the policy data of what the max attribute is
> for this thing.
> 
> > +static const struct nl_desc_attr nft_nldesc_set_attrs[NFTA_SET_MAX + 1] = {
> ...
> > +       NLDESC_ATTR_NESTED(NFTA_SET_DESC, nft_nldesc_set_desc),
> 
> and here this is again just a policy thing, more or less.
> 
> So it seems to me that without even having an object enum, we'd get the
> same kind of data by
> 
> struct nla_policy nft_policy_set_desc[NFTA_SET_DESC_MAX + 1] = {
> 	[NFTA_SET_DESC_SIZE] = { .type = NLA_U32 },
> };
> 
> struct nla_policy nft_policy_set[NFTA_SET_MAX + 1] = {
> 	[NFTA_SET_DESC] = NLA_POLICY_NESTED(nft_policy_set_desc),
> };
> 
> Except we don't have an "object ID" (and obviously also that we can use
> the same data to actually parse/validate messages, and so don't end up
> with bad situations of nldesc saying one thing and the policy another.)

The object ID is needed by userspace, to build a flat table, and look
up for the nested IDs, to express the graph. We can probably place
this in the policy.

> So ... Assuming I write some code to expose the policy (which I plan on
> doing), what would this still be able to expose in addition?

This would be incomplete, since commands are an essential part too,
and relation of attributes with commands are also too.

Powered by blists - more mailing lists