RE: [net-next PATCH 1/2] devlink: add dry run attribute to flash update

["Keller, Jacob E" <jacob.e.keller@intel.com>]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Monday, July 25, 2022 1:33 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Jiri Pirko <jiri@resnulli.us>; netdev@vger.kernel.org
> Subject: Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update
> 
> On Mon, 25 Jul 2022 20:27:02 +0000 Keller, Jacob E wrote:
> > > ...or repost without the comment and move on. IDK if Jiri would like
> > > to see the general problem of attr rejection solved right now but IMHO
> > > it's perfectly fine to just make the user space DTRT.
> >
> > Its probably worth fixing policy, but would like to come up with a
> > proper path that doesn't break compatibility and that will require
> > discussion to figure out what approach works.
> 
> The problem does not exist for new commands, right? Because we don't
> set GENL_DONT_VALIDATE_STRICT for new additions. For that reason I
> don't think we are in the "sooner we fix the better" situation.

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...