[RFC,4/4] netfilter: nf_tables: add netlink description

This patch adds the netlink description for nf_tables.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/net/netfilter/nf_tables.h             |   2 +
 include/uapi/linux/netfilter/nf_tables_desc.h |  57 ++++
 net/netfilter/Makefile                        |   7 +-
 net/netfilter/nf_tables_api.c                 |   2 +
 net/netfilter/nf_tables_desc.c                | 471 ++++++++++++++++++++++++++
 5 files changed, 536 insertions(+), 3 deletions(-)
 create mode 100644 include/uapi/linux/netfilter/nf_tables_desc.h
 create mode 100644 net/netfilter/nf_tables_desc.c

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.

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.

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



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?

johannes

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.

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

On Fri, Apr 12, 2019 at 01:56:22PM +0200, Johannes Berg wrote:
> > > 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 :-)

How are you going to relate this with the policy description?

Could you post a schema of the netlink attribute hierarchy of your
policy descriptions?

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

This object ID is a policy nest ID.

The idea is to expose an array of nest IDs (object IDs), then this array
contains the attributes that this nest contains and if there are nested
attributes, then in case of nested attributed, the nest ID that describe
this attribute.

The idea is to make it easy for userspace to build this table, and then
userspace performs lookups based on this object ID (policy nest 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.

My idea was to provide a description of netlink, then a compiler tool
that would autogenerate the policy structures and the UAPI headers from
this bus description. Yes, it sounds too futuristic to autogenerate code
in the kernel, but other projects do already.

On Fri, 2019-04-26 at 18:42 +0200, Pablo Neira Ayuso wrote:
> On Fri, Apr 12, 2019 at 01:56:22PM +0200, Johannes Berg wrote:
> > > > 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 :-)
> 
> How are you going to relate this with the policy description?

Well, I'm not actually relating this at all right now.

Since generic netlink already has a list of commands like

{
  .cmd = XYZ,
  .doit = do_xyz,
  .dumpit = dump_xyz,
}

and I recently moved the policy to be for the whole *family*, not for
each command, basically all we're missing is a list of attributes used
by this command.

Ideally, we'd add this as

{
  .cmd = XYZ,
  .doit = do_xyz,
  .dumpit = dump_xyz,
  .attrs = { ATTR_A, ATTR_B, ATTR_C, ATTR_D },
}

but of course there's no good way to express this in C, you'd have to
build an out-of-line array and point to it.

If we do this though, it's easy to expose, right? Just a list of
attribute types that you can go look up in the policy description
obtained separately.

> Could you post a schema of the netlink attribute hierarchy of your
> policy descriptions?

The whole patch is in your inbox ;-)

But basically it looks like this:
FAMILY_SPECIFIC_POLICY_ATTRIBUTE = { // nested
  [policy index] = { // nested, 0 = root policy
    [attribute type] = { // nested
      [NL_POLICY_TYPE_ATTR_TYPE] = U32(NL_ATTR_TYPE_*),

      // for signed limited values:
      [NL_POLICY_TYPE_ATTR_MIN_VALUE_S] = S64(...),
      [NL_POLICY_TYPE_ATTR_MAX_VALUE_S] = S64(...),

      // for unsigned limited values:
      [NL_POLICY_TYPE_ATTR_MIN_VALUE_U] = U64(...),
      [NL_POLICY_TYPE_ATTR_MAX_VALUE_U] = U64(...),

      // for binary min/max length:
      [NL_POLICY_TYPE_ATTR_MIN_LENGTH] = U32(...),
      [NL_POLICY_TYPE_ATTR_MAX_LENGTH] = U32(...),

      // for nested policy (nested or nested array):
      [NL_POLICY_TYPE_ATTR_POLICY_IDX] = U32(...),
      [NL_POLICY_TYPE_ATTR_POLICY_MAXTYPE] = U32(...),

      // for bitfield32:
      [NL_POLICY_TYPE_ATTR_BITFIELD32_MASK] = U32(...),
    }
  }
}

> > > 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.
> 
> This object ID is a policy nest ID.

Ok. So basically I even already have the object ID, except that I don't
really specifically give it one, I just build it based on the flattening
algorithm I described in my other email..

> The idea is to expose an array of nest IDs (object IDs), then this array
> contains the attributes that this nest contains and if there are nested
> attributes, then in case of nested attributed, the nest ID that describe
> this attribute.
> 
> The idea is to make it easy for userspace to build this table, and then
> userspace performs lookups based on this object ID (policy nest ID).

Right. I think I have that?

> > 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.
> 
> My idea was to provide a description of netlink, then a compiler tool
> that would autogenerate the policy structures and the UAPI headers from
> this bus description. Yes, it sounds too futuristic to autogenerate code
> in the kernel, but other projects do already.

Yeah, ok, but I think you'll have a hard time convincing everyone to
switch to that whole-sale, and ban the other approach. What I've done
with the policy is far more agreeable to making progress without a
complete rewrite. The policy exposure for generic netlink is mostly
automatic, for example, as it could be for other netlink families I
guess.

The issue with C I noted above of course does lend itself really well to
expressing it in a DSL and then generating the C code, but even *then* I
would still argue that having all of this duplicated is a waste of
memory since we need to have the same data already.

Anyway. Please check out the patches I posted today. I do know that they
cannot currently express the "which attribute belongs to which command"
part, but I believe that we can add that in some way without rewriting
the whole thing, and that making step-by-step progress is better.

So please check out the patches and tell me whether you see any
fundamental problem with that, or if you just think that they're
incomplete in this manner - I agree and really would like to address
that, but need to also see how that feeds back into the validation (it
must, after all) etc.

johannes

On Fri, 2019-04-26 at 19:17 +0200, Johannes Berg wrote:
> 
> Ideally, we'd add this as
> 
> {
>   .cmd = XYZ,
>   .doit = do_xyz,
>   .dumpit = dump_xyz,
>   .attrs = { ATTR_A, ATTR_B, ATTR_C, ATTR_D },
> }
> 
> but of course there's no good way to express this in C, you'd have to
> build an out-of-line array and point to it.

Actually, it's possibly even more complicated. After all, it is possible
that you have an ATTR_N, that is nested, and that contains certain sub-
attributes (ATTR_N_A, ATTR_N_B, ...) of which only some are valid for
the operation X, but a different subset is valid for operation Y.

I'm sort of hoping we don't have enough of these cases to make it really
something we want to express, because that'll be really hard to express
in a way that we can validate and expose.

> The issue with C I noted above of course does lend itself really well to
> expressing it in a DSL and then generating the C code, but even *then* I
> would still argue that having all of this duplicated is a waste of
> memory since we need to have the same data already.

And also, in addition to this, I just realized that we really don't want
any sort of separate descriptions because we want to have the ability to
validate everything that we express to userspace.

IOW - if we tell userspace "this is valid" then we should at the same
time be able to use those description data structures to actually
validate that userspace isn't sending us something out of this spec.

So I guess that's just one more reason I think that we fundamentally
must provide to userspace exactly what we validate, and make the
validation data structures (or language) expressive enough to be able to
capture the real constraints.

johannes

On Fri, Apr 26, 2019 at 07:28:15PM +0200, Johannes Berg wrote:
> On Fri, 2019-04-26 at 19:17 +0200, Johannes Berg wrote:
> > 
> > Ideally, we'd add this as
> > 
> > {
> >   .cmd = XYZ,
> >   .doit = do_xyz,
> >   .dumpit = dump_xyz,
> >   .attrs = { ATTR_A, ATTR_B, ATTR_C, ATTR_D },
> > }
> > 
> > but of course there's no good way to express this in C, you'd have to
> > build an out-of-line array and point to it.
> 
> Actually, it's possibly even more complicated. After all, it is possible
> that you have an ATTR_N, that is nested, and that contains certain sub-
> attributes (ATTR_N_A, ATTR_N_B, ...) of which only some are valid for
> the operation X, but a different subset is valid for operation Y.

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.

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.

On Fri, 2019-04-26 at 20:04 +0200, Pablo Neira Ayuso wrote:
> On Fri, Apr 26, 2019 at 07:28:15PM +0200, Johannes Berg wrote:
> > On Fri, 2019-04-26 at 19:17 +0200, Johannes Berg wrote:
> > > 
> > > Ideally, we'd add this as
> > > 
> > > {
> > >   .cmd = XYZ,
> > >   .doit = do_xyz,
> > >   .dumpit = dump_xyz,
> > >   .attrs = { ATTR_A, ATTR_B, ATTR_C, ATTR_D },
> > > }
> > > 
> > > but of course there's no good way to express this in C, you'd have to
> > > build an out-of-line array and point to it.
> > 
> > Actually, it's possibly even more complicated. After all, it is possible
> > that you have an ATTR_N, that is nested, and that contains certain sub-
> > attributes (ATTR_N_A, ATTR_N_B, ...) of which only some are valid for
> > the operation X, but a different subset is valid for operation Y.
> 
> 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. I just
*removed* having a separate policy for each command in generic netlink,
as  ;-)

What really I think we should have is a common policy, but only some
attributes are valid in some commands.

I guess you can slice this in different ways. From a "how much space
does this consume" and "can I reuse some code across different commands"
I think having the same policy is a good idea though.

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

The list of policies is just built internally when you dump out a policy
with its sub-policies for nested attributes/arrays.

johannes

On Fri, Apr 26, 2019 at 09:14:43PM +0200, Johannes Berg wrote:
> On Fri, 2019-04-26 at 20:04 +0200, Pablo Neira Ayuso wrote:
> > On Fri, Apr 26, 2019 at 07:28:15PM +0200, Johannes Berg wrote:
> > > On Fri, 2019-04-26 at 19:17 +0200, Johannes Berg wrote:
> > > > 
> > > > Ideally, we'd add this as
> > > > 
> > > > {
> > > >   .cmd = XYZ,
> > > >   .doit = do_xyz,
> > > >   .dumpit = dump_xyz,
> > > >   .attrs = { ATTR_A, ATTR_B, ATTR_C, ATTR_D },
> > > > }
> > > > 
> > > > but of course there's no good way to express this in C, you'd have to
> > > > build an out-of-line array and point to it.
> > > 
> > > Actually, it's possibly even more complicated. After all, it is possible
> > > that you have an ATTR_N, that is nested, and that contains certain sub-
> > > attributes (ATTR_N_A, ATTR_N_B, ...) of which only some are valid for
> > > the operation X, but a different subset is valid for operation Y.
> > 
> > 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".

> I just *removed* having a separate policy for each command in
> generic netlink, as  ;-)
> 
> What really I think we should have is a common policy, but only some
> attributes are valid in some commands.
> 
> I guess you can slice this in different ways. From a "how much space
> does this consume" and "can I reuse some code across different commands"
> I think having the same policy is a good idea though.
>
> > 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 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.

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

On Fri, 2019-04-26 at 21:37 +0200, Johannes Berg wrote:

> 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),
> };
> 
So if we extend this, say like this:

struct nla_policy policy[...] = {
    [MY_ATTR] = NLA_POLICY_NESTED(nested_policy),
    [MY_OTHER_ATTR] = NLA_POLICY_NESTED(nested_policy),
};

then you could perhaps argue that having an object ID makes sense, and
assigning the same object ID to MY_ATTR and MY_OTHER_ATTR would make
sense?

Of course, my could would assign this the same (temporary) policy ID,
but there can be no reliance on the policy ID beyond what's needed at
runtime to map the attribute to the nested policy.

You still see at runtime that these have the same policy (since they
have the same policy ID), but at the same time presumably there was a
reason to have MY_ATTR and MY_OTHER_ATTR, so perhaps the semantics are
different even if the attributes are the same, as could perhaps be
expected if you have a SET and a CLEAR attribute (MY_ATTR and
MY_OTHER_ATTR respectively) and the contents you give has the same
policy, but different logic?

Basically, I just didn't consider this case to be significant enough to
manually and assign stable IDs of some sort, when we already have them
in the form of the attribute type.

johannes

On Fri, Apr 26, 2019 at 09:14:43PM +0200, Johannes Berg wrote:
> Yeah, ok. Each object you had is basically its own policy. I just
> *removed* having a separate policy for each command in generic netlink,
> as  ;-)

But that only affects genetlink users who let genetlink validate and
parse messages for them. Those validating/parsing the messages
themselves can still have different policy (and completely different set
of attributes) for each command.

Michal

On Fri, 2019-04-26 at 22:47 +0200, Michal Kubecek wrote:
> On Fri, Apr 26, 2019 at 09:14:43PM +0200, Johannes Berg wrote:
> > Yeah, ok. Each object you had is basically its own policy. I just
> > *removed* having a separate policy for each command in generic netlink,
> > as  ;-)
> 
> But that only affects genetlink users who let genetlink validate and
> parse messages for them. Those validating/parsing the messages
> themselves can still have different policy (and completely different set
> of attributes) for each command.

Sure. I already argued elsewhere though that you should only have one
policy for each set of attributes, but if you do in fact have different
attributes then that's perfectly valid.

Not sure whether I think it all that reasonable, since you then burden
userspace with having to know even more intricate detail and not being
able to share code well between commands, but hey :-)

johannes

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!

On Fri, Apr 26, 2019 at 09:46:33PM +0200, Johannes Berg wrote:
> On Fri, 2019-04-26 at 21:37 +0200, Johannes Berg wrote:
> 
> > 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),
> > };
> > 
> So if we extend this, say like this:
> 
> struct nla_policy policy[...] = {
>     [MY_ATTR] = NLA_POLICY_NESTED(nested_policy),
>     [MY_OTHER_ATTR] = NLA_POLICY_NESTED(nested_policy),
> };
> 
> then you could perhaps argue that having an object ID makes sense, and
> assigning the same object ID to MY_ATTR and MY_OTHER_ATTR would make
> sense?
> 
> Of course, my could would assign this the same (temporary) policy ID,
> but there can be no reliance on the policy ID beyond what's needed at
> runtime to map the attribute to the nested policy.
> 
> You still see at runtime that these have the same policy (since they
> have the same policy ID), but at the same time presumably there was a
> reason to have MY_ATTR and MY_OTHER_ATTR, so perhaps the semantics are
> different even if the attributes are the same, as could perhaps be
> expected if you have a SET and a CLEAR attribute (MY_ATTR and
> MY_OTHER_ATTR respectively) and the contents you give has the same
> policy, but different logic?
> 
> Basically, I just didn't consider this case to be significant enough to
> manually and assign stable IDs of some sort, when we already have them
> in the form of the attribute type.

But they all point to the same nested_policy, ie. these nested
atributes represent instances of the same object class.

I think this is meaningful to userspace in terms of providing a
description of the interface, rather than making it look.

Without the ID, it is not possible from userspace to see that MY_ATTR
and MY_OTHER_ATTR refer to the same object, right?

On Sat, 2019-04-27 at 12:52 +0200, Pablo Neira Ayuso wrote:

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

Yes. Although when you say "policies", I'd argue that's not an
"nla_policy" but rather some sort of "list of permitted attributes".

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

Yes, mostly. My bigger concern isn't really that we reuse it, my bigger
concern is that if we *don't* we'll eventually end up with something
like:

struct nla_policy policy_for_one_command[] = {
   [NLA_MY_ATTR] = { .type = NLA_U32 },
   ...
};

struct nla_policy policy_for_another_command[] = {
   [NLA_MY_ATTR] = { .type = NLA_U16 },
   ...
};

and then we just have a big mess. So I'd like to separate the "type"
policy (what type is each attribute in a given netlink type enum) from
the "permissible" policy (which attributes are valid in a given context
like a command).

I've not actually started working on the second part at all yet, but
because I strongly believe it makes sense to separate them, I don't see
a need to interleave it with the rest of the patch series.

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

Right.

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

Sure.

The only difference between the "object ID" that you manually assign and
the "policy ID" that my code automatically assigns is that the latter is
somewhat ephemeral - a new kernel version may change it.

So, let's say you have
struct nla_policy policy[...] = {
  [NLA_MY_OBJECT] = NLA_POLICY_NESTED(sub_policy),
  [NLA_ANOTHER_OBJ] = NLA_POLICY_NESTED_ARRAY(sub_policy),
};

Then in one kernel version you might get reported to userspace:

NLA_MY_OBJECT: nested - policy 7
NLA_ANOTHER_OBJ: nested array - policy 7

and in a new kernel version the value "7" there might change to be 8 or
6 or whatever depending on changes in any possible nested policy for
other attributes, but it would still always be the same number (unless
you actually changed the fact that both link to "sub_policy" in the C
code description), and the contents of that referenced policy would
still all be the same as before. Just the ID that links them together at
runtime would be more ephemeral than the object ID you proposed.

I don't, however, think that this is a problem as I don't see any value
in the object ID itself. It's just an ID to tell you what kind of sub-
attributes are allowed.

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

I posted a very hacky patch before, let me see if I can find it...

https://p.sipsolutions.net/4c674acaf8d6ca71.txt

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

I'm not using MY_ATTR as ID. I'm just making up an (ephemeral) object ID
at runtime based on the policy pointer. See above though.

johannes

On Sat, 2019-04-27 at 12:57 +0200, Pablo Neira Ayuso wrote:

> But they all point to the same nested_policy, ie. these nested
> atributes represent instances of the same object class.

To some extent, yes.

> I think this is meaningful to userspace in terms of providing a
> description of the interface, rather than making it look.

Sure.

> Without the ID, it is not possible from userspace to see that MY_ATTR
> and MY_OTHER_ATTR refer to the same object, right?

There is an ID, and if you reference the same sub-policy multiple times
for nested / nested array attribute types (even at different levels of
nesting btw) then this sub-policy will only be dumped to userspace
multiple times, given an ID, and be referenced by that ID from the
appropriate attribute types in other root/sub-policies.

The only thing is that between kernel versions that ID may change as
it's computed while walking the policy graph, and that graph may change
and thus the walk may reach nodes in the graph in a different order and
thereby label them differently.

johannes