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.
