On Mon, 25 Jul 2022 20:46:01 +0000 Keller, Jacob E wrote:

There are two problems, and only one of them is solved by strict
validation right now:

1) Does the kernel know this attribute?

This is the question of whether the kernel is new enough to have the
attribute, i.e. does the DEVLINK_ATTR_DRY_RUN even exist in the
kernel's uapi yet.

This is straight forward, and usually good enough for most
attributes. This is what is solved by not setting
GENL_DONT_VALIDATE_STRICT.

However, consider what happens once we add  DEVLINK_ATTR_DRY_RUN and
support it in flash update, in version X. This leads us to the next
problem.

2) does the *command* recognize and support DEVLINK_ATTR_DRY_RUN

Since the kernel in this example already supports
DEVLINK_ATTR_DRY_RUN, it will be recognized and the current setup the
policy for attributes is the same for every command. Thus the kernel
will accept DEVLINK_ATTR_DRY_RUN for any command, strict or not.

But if the command itself doesn't honor DEVLINK_ATTR_DRY_RUN, it will
once again be silently ignored.

We currently use the same policy and the same attribute list for
every command, so we already silently ignore unexpected attributes,
even in strict validation, at least as far as I can tell when
analyzing the code. You could try to send an attribute for the wrong
command. Obviously existing iproute2 user space doesn't' do this..
but nothing stops it.

For some attributes, its not a problem. I.e. all flash update
attributes are only used for DEVLINK_CMD_FLASH_UPDATE, and passing
them to another command is meaningless and will likely stay
meaningless forever. Obviously I think we would prefer if the kernel
rejected the input anyways, but its at least not that surprising and
a smaller problem.

But for something generic like DRY_RUN, this is problematic because
we might want to add support for dry run in the future for other
commands. I didn't really analyze every existing command today to see
which ones make sense. We could minimize this problem for now by
checking DRY_RUN for every command that might want to support it in
the future...

Hm, yes. Don't invest too much effort into rendering per-cmd policies
right now, tho. I've started working on putting the parsing policies 
in YAML last Friday. This way we can auto-gen the policy for the kernel
and user space can auto-gen the parser/nl TLV writer. Long story short
we can kill two birds with one stone if you hold off until I have the
format ironed out. For now maybe just fork the policies into two - 
with and without dry run attr. We'll improve the granularity later 
when doing the YAML conversion.