Message-ID: <20190427105208.xlqv77pmjvkpyxr4@salvia>
Date:   Sat, 27 Apr 2019 12:52:08 +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, Apr 26, 2019 at 09:37:43PM +0200, Johannes Berg wrote:
> On Fri, 2019-04-26 at 21:20 +0200, Pablo Neira Ayuso wrote:
> 
[...]
> So ... I guess I need to think about this a bit more.
> 
> My approach to this was that as long as you have a single enum
> (namespace) for attributes (e.g. enum nft_chain_attributes to pick a
> random example), you'd also want to have a single policy for these,
> describing the possibilities.
> 
> I do admit though that this doesn't cover the "only some of these
> attributes are valid for a given command" part, and I also admit that I
> had sort of punted that part.
> 
> We can solve that by having separate policies, so if you have two
> operations using NTFA_CHAIN_*, then perhaps an operation deletes a chain
> can only take a NFTA_CHAIN_HANDLE or NFTA_CHAIN_NAME, but an operations
> that sets things inside might take all the other attributes.
>
> HOWEVER, having separate policies then opens up an easy avenue for
> (mistakenly or not) having, well, separate policies! Even for the same
> attribute type. And that's something we really *don't* want.
>
> So I actually think that separating "type description", which is the
> policy today, from the "type validity description" is valuable, because
> ideally we do want each type (e.g. NFTA_CHAIN_HANDLE) to always be the
> same regardless of command, so that a hypothetical GET_STATS command
> won't take a U64 CHAIN_HANDLE while a (similarly hypothetical)
> DELETE_CHAIN command takes a U32 CHAIN_HANDLE!

If we want to avoid the extra code for the netlink description, and
reuse the existing policy objects, then we'll need policies for each
command that express what attributes make sense per command as you
describe above.

> Hence my thought of separating "this is the policy for the attributes
> (types)" and "this is the list of allowed types for this command". I do
> realize now that the latter becomes difficult with nested attributes
> though, but we're probably better off finding ways to express that than
> having entirely different policies (also, the data would be smaller).

The netlink description is telling what attributes are available for
each command and policies, so policy definitions are a subset of the
netlink description.

If I understand well, your concern is that you would like that we
use reuse / extend the existing policy structures to be used for the
netlink description. That's very reasonable indeed and a good idea.

> [[[ total aside: we could do something like the "list of allowed types"
> being an array of
> union valid_type {
>    u16 attr;
>    struct nested *nested;
> };
> 
> with
> 
> struct nested {
>   u16 attr;
>   union valid_type *valid_nested_types;
> };
>
> I think we can probably safely assume that pointers are always >>MAX_U16
> but if not we can either make that larger or find other ways of
> disambiguating, e.g. by alignment (and making ATTR_TO_PTR(attr)
> something like "(attr << 1) | 0x1" since "struct nested" requires 2-byte 
> alignment.
> 
> > > > If we use the list policies that you propose, then it's just an extra
> > > > enumeration to maintain for each command. And many commands will
> > > > likely reuse the same object ID.
> > > 
> > > A policy pointer, really.
> > 
> > Sort of, but it has to be stable along time for userspace, right?
> > Actually, it's an ID.
> 
> The ID in my policy export is not stable in any way. The only thing that
> it's used for is to identify which sub-policy is used while you're
> dumping it. It will be stable by virtue of the algorithm, but will
> change when you add different sub-policy links etc. You cannot rely on
> it being stable, but you also do not need to since it's only relevant
> that you are able to identify the nested policy while dumping.
>
> > > The list of policies is just built internally when you dump out a policy
> > > with its sub-policies for nested attributes/arrays.
> > 
> > If we expose these to userspace, we need that these object IDs are
> > stable, hence the enum. So userspace results in a simple program that
> > just makes look ups for the object ID that contains the description of
> > the attributes that are available.
> 
> Well, no.
> 
> You're now thinking of the "policy ID" I assigned for the wire format as
> the object ID, but really that's not what it is. The object ID that
> you're looking for is the attribute type of the nested attribute.

I'm attaching userspace code for the netlink description
infrastructure for libmnl. It should be easy to rework it to build a
flat table with these IDs. The IDs are helping to provide a easy way
to express this graph in a data structure that is easy to navigate
from userspace.

My usecase is this: I don't want to probe the kernel to guess if a
command / attribute is available for this kernel version or not.
I just want to dump the netlink description, then navigate this table
to search if that command / attribute is there.

> So if you have
> 
> struct nla_policy nested_policy[...] = { ... };
> 
> struct nla_policy policy[...] = {
>     [MY_ATTR] = NLA_POLICY_NESTED(nested_policy),
> };
> 
> and you dump out starting from "policy" then yes, "policy" will have ID
> 0, and "nested_policy" will have ID 1, but those are only temporary
> identifiers for the dump. What's really relevant is the attribute type
> "MY_ATTR".

Do you have userspace I can have a look? With runtime / dynamic ID
generation, I cannot see yet how to navigate such a table or how the
userspace representation would look like.

By using MY_ATTR as ID, I think you will have to build a graph
structure in userspace, I was trying to provide a simple flat table
representation for userspace.

Thanks!

View attachment "nl-desc-libmnl.patch" of type "text/x-diff" (11834 bytes)

Powered by blists - more mailing lists