Message-ID: <20190427112511.fqage4zlfapfj2td@salvia>
Date:   Sat, 27 Apr 2019 13:25:11 +0200
From:   Pablo Neira Ayuso <pablo@...filter.org>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     netdev@...r.kernel.org, David Ahern <dsa@...ulusnetworks.com>
Subject: Re: [PATCH 6/6] netlink: add infrastructure to expose policies to
 userspace

On Fri, Apr 26, 2019 at 09:22:20PM +0200, Johannes Berg wrote:
> On Fri, 2019-04-26 at 20:21 +0200, Pablo Neira Ayuso wrote:
> > On Fri, Apr 26, 2019 at 02:13:06PM +0200, Johannes Berg wrote:
> > > diff --git a/include/uapi/linux/genetlink.h b/include/uapi/linux/genetlink.h
> > > index 877f7fa95466..9c0636ec2286 100644
> > > --- a/include/uapi/linux/genetlink.h
> > > +++ b/include/uapi/linux/genetlink.h
> > > @@ -48,6 +48,7 @@ enum {
> > >  	CTRL_CMD_NEWMCAST_GRP,
> > >  	CTRL_CMD_DELMCAST_GRP,
> > >  	CTRL_CMD_GETMCAST_GRP, /* unused */
> > > +	CTRL_CMD_GETPOLICY,
> > 
> > It would be good to single entry point to request descriptions, ie.
> > have a NETLINK_DESC family for this. Thus, we could use the same
> > program to pull for policy/command descriptions without updating an
> > array that includes the command to get the policy _for each
> > subsystem_.
> > 
> > The program to inquire for policy/command descriptions would be very
> > much the same along time, no need for updates to include new command
> > type for each subsystem.
> > 
> > It would just spin over NETLINK_DESC discovering subsystems ID that we
> > support.
> 
> No objection to that. The only problem I think is that there's no
> natural point to "hang" the policy data, mostly it's just used in the
> nla_parse() or nla_validate() calls in the code. Basically, it's code-
> driven, not data-driven like generic netlink.
> 
> Take IFLA_AF_SPEC for example. To validate that, we end up calling into
> validate_link_af() which is defined in IPv4 and IPv6, rather than having
> the inet_af_policy/inet6_af_policy available and doing it in the caller
> (also, validate_link_af() does some additional validation, though for
> IFLA_INET_CONF that can actually now be expressed as a nested policy
> inside inet_af_policy, I believe).
> 
> So to really generalize that you'd have change this - at least as far as
> the netlink attribute validation is concerned, not the extra code - to
> be data driven, rather than coded.
> 
> Then you could use and expose that data pretty easily.

I see, agreed, we would need to rework this to make data driven, so
this nest:

IFLA_AF_SPEC (nest)
  family = AF_INET6 (nest)
    IFLA_INET6_...

IFLA_AF_SPEC (nest)
  family = AF_INET4 (nest)
    IFLA_INET_...

needs a nla_policy definition for each family.

We can do this rework progressively, as we start exposing description
though the list of nla_policy structure for each subsystem.

> > In genetlink, I understand this can be exception if you prefer so, ie.
> > I'll be fine with this CTRL_CMD_GETPOLICY if that makes it look nicer
> > in terms of integration with the existing infrastructure. But for
> > other netlink subsystems, NETLINK_DESC allows you to pull the
> > description for genetlink itself, not the internal subsystems.
> 
> I think genetlink it really makes more sense this way - the genetlink
> family it basically the introspection point already: it lets you
> discover which families there are, which commands they support,
> multicast groups they have etc.
> 
> It's also easier to deal with in userspace because you already need to
> deal with the genetlink family itself (to get your family ID), so
> interacting with another NETLINK_DESC family would be annoying.
> 
> However, if we wanted to generalize that I guess we could make
> NETLINK_DESC able to cover generic netlink as well, providing two entry
> points to the same information? Very small amount of code, I guess.

Makes sense to me.

Powered by blists - more mailing lists