Date:   Tue, 26 Jul 2022 17:23:53 +0000
From:   "Keller, Jacob E" <jacob.e.keller@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     Jiri Pirko <jiri@...nulli.us>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [net-next PATCH 1/2] devlink: add dry run attribute to flash
 update



> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Monday, July 25, 2022 6:14 PM
> To: Keller, Jacob E <jacob.e.keller@...el.com>
> Cc: Jiri Pirko <jiri@...nulli.us>; netdev@...r.kernel.org
> Subject: Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update
> 
> 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.

Makes sense, this would definitely simplify writing policy and avoid some duplication that would occur otherwise.

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

Not quite sure I follow this. I guess just add a separate policy array with dry_run and then make that the policy for the flash update command? I don't think flash update is strict yet, and I'm not sure what the impact of changing it to strict is in terms of backwards compatibility with the interface.

Thanks,
Jake

Powered by blists - more mailing lists