Re: [PATCH net-next 1/3] genetlink: do not validate dump requests if there is no policy

[Johannes Berg]

On Thu, 2019-05-02 at 07:36 -0600, David Ahern wrote:
> On 5/2/19 7:32 AM, Michal Kubecek wrote:
> > Wouldn't it mean effecitvely ending up with only one command (in
> > genetlink sense) and having to distinguish actual commands with
> > atributes? Even if I wanted to have just "get" and "set" command, common
> > policy wouldn't allow me to say which attributes are allowed for each of
> > them.
> 
> yes, I have been stuck on that as well.
> 
> There are a number of RTA attributes that are only valid for GET
> requests or only used in the response or only valid in NEW requests.
> Right now there is no discriminator when validating policies and the
> patch set to expose the policies to userspace

Yeah. As I've been discussing with Pablo in various threads recently,
this is definitely something we're missing.

As I said there though, I think it's something we should treat as
orthogonal to the policies.

I haven't looked at your ethtool patches really now (if you have a git
tree that'd be nice), but I saw e.g.

+When appropriate, network device is identified by a nested attribute named
+ETHA_*_DEV. This attribute can contain
+
+    ETHA_DEV_INDEX	(u32)		device ifindex
+    ETHA_DEV_NAME	(string)	device name

Presumably, this is valid for each and every command, right?

I'm not sure I understand the "ETHA_*_DEV" part, but splitting the
policy per command means that things like this that are available/valid
for each command need to be stated over and over again. This opens up
the very easy possibility that you have one command that takes an
ETHA_DEV_INDEX as u32, and another that - for some reason - takes a u64
for example, or similar confusion between the same attribute stated in
different policies.

This is why I believe that when we have a flat namespace for attributes,
like ETHA_*, we should also have a flat policy for those attributes, and
that's why I made the genetlink to have a single policy.

At the same time, I do realize that this is not ideal. So far I've sort
of pushed this to be something that we should treat orthogonally to the
validation for the above reasons, i.e. *not* state this specifically in
the policy.

If we were able to express this in C, I'd probably say we should have
something like

static const struct genl_ops ops[] = {
        {
                .cmd = MY_CMD,
                .doit = my_cmd_doit,
		.valid_attrs = { MY_ATTR_A, MY_ATTR_B },
        },

	...
};

However, there's no way to express this in C code, except for

static const u16 my_cmd_valid_attrs[] = { MY_ATTR_A, MY_ATTR_B, 0 };

static const struct genl_ops ops[] = {
        {
                .cmd = MY_CMD,
                .doit = my_cmd_doit,
		.valid_attrs = my_cmd_valid_attrs,
        },

	...
};

which is clearly ugly to write. We could generate this code from a
domain-specific language like Pablo suggested, but I'm not really sure
that's ideal either.


Regardless, I think we should solve this problem orthogonally from the
policy, given that otherwise we can end up with the same attribute
meaning different things in different commands.

johannes