Message-ID: <beb5d59393d8dbf466b0d728ca85842afb6cd039.camel@sipsolutions.net>
Date:   Fri, 12 Apr 2019 13:56:22 +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

Hi Pablo,

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

Please do, and please have a look at the patch(es) I sent out regarding
this. I think we've extended the policy enough that most of this is
really there, and we can certainly add more to the policy.

For example, I am now considering allowing NLA_POLICY_RANGE(NLA_BINARY,
...) to have an upper *and* lower length bound for an NLA_BINARY value,
right now you can only have either of the two and we have uses cases
where we need both.

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

Alright, well, generic netlink already exposes the commands :-)

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

I guess the way I see it, they're all pieces of the puzzle. The policy
is good for both code in the kernel and for exposing it. The command
list is already exposed.

What now we're missing, in a sense, is which commands take which
attributes. This would actually *also* be a good thing for validation in
the kernel, but I haven't really found a good way to encode it yet.
Doing a bitmap isn't really feasible, and doing a list is difficult to
type out.

Perhaps if we do end up going with your code generation idea then we can
do this more easily.

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

I don't really understand the object ID.

Say you have a top-level "filter" attribute (I'm completely making up
things here).

Now this filter "object" can have a number of attributes, and that's
captured by the nested policy there?

IOW - the code I wrote until now would expose

policy 0: attribute filter: nested with attrs from policy 1
policy 1: attribute abc: NLA_U8
policy 1: attribute xyz: NLA_U32

etc.

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

Agree. Still, I think it solves much of the problem, like I said above -
all are parts of the puzzle.

With generic netlink we have a list of commands already. That should be
easy to add elsewhere, though generic netlink has the advantage of
having the list of commands (and their handlers) as *data*, which I
think is expressed *code* in rtnetlink now? But that can either be fixed
or you just carry a separate list of commands there.

With the policy export I wrote we have the policy, and if you set it up
properly with all the nested sub-policies for each attribute etc.

So I agree - we're missing "this command uses these attributes", but
again I don't think the object description is actually the right
solution here, because you *also* want to enforce that in some way.

I feel the generic netlink model where the command handlers etc. are not
code but data is superior here because it lets us add things like that
relatively easily, though of course we haven't actually done it now.


So basically - my main concern with this bus description stuff you've
done here is that it duplicates all the data and needs to be maintained
separately from what should be the same data used for enforcement.

johannes

Powered by blists - more mailing lists