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

On Thu, 2019-05-02 at 07:36 -0600, David Ahern wrote:

On 5/2/19 7:32 AM, Michal Kubecek wrote:

quoted

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