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

On Fri, 2019-04-26 at 21:20 +0200, Pablo Neira Ayuso wrote:

> > > I solved this in my patchset through the object ID. So each command
> > > points to an object ID, then such object ID comes with a list of
> > > attributes.
> > 
> > Yeah, ok. Each object you had is basically its own policy.
> 
> Hm, not necessarily.
> 
> The object id is matching to the "root policy". Several commands may
> have the same "root policy" (or as I call it "object id"). For
> example, in netfilter we have commands to add and to delete rules,
> both would have the same "root policy".

Right, I didn't mean they necessarily had a different policy, just that
they could.

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!

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).

[[[ 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.

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".

johannes

Powered by blists - more mailing lists