| Message ID | 20220720183433.2070122-2-jacob.e.keller@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | devlink: implement dry run support for flash update | expand |
Wed, Jul 20, 2022 at 08:34:32PM CEST, jacob.e.keller@intel.com wrote: >Users use the devlink flash interface to request a device driver program or >update the device flash chip. In some cases, a user (or script) may want to >verify that a given flash update command is supported without actually >committing to immediately updating the device. For example, a system >administrator may want to validate that a particular flash binary image >will be accepted by the device, or simply validate a command before finally >committing to it. > >The current flash update interface lacks a method to support such a dry >run. Add a new DEVLINK_ATTR_DRY_RUN attribute which shall be used by a >devlink command to indicate that a request is a dry run which should not >perform device configuration. Instead, the command should report whether >the command or configuration request is valid. > >While we can validate the initial arguments of the devlink command, a >proper dry run must be processed by the device driver. This is required >because only the driver can perform validation of the flash binary file. > >Add a new dry_run parameter to the devlink_flash_update_params struct, >along with the associated bit to indicate if a driver supports verifying a >dry run. > >We always check the dry run attribute last in order to allow as much >verification of other parameters as possible. For example, even if a driver >does not support the dry_run option, we can still validate the other >optional parameters such as the overwrite_mask and per-component update >name. > >Document that userspace should take care when issuing a dry run to older >kernels, as the flash update command is not strictly verified. Thus, >unknown attributes will be ignored and this could cause a request for a dry >run to perform an actual update. We can't fix old kernels to verify unknown >attributes, but userspace can check the maximum attribute and reject the >dry run request if it is not supported by the kernel. > >Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> >--- > .../networking/devlink/devlink-flash.rst | 23 +++++++++++++++++++ > include/net/devlink.h | 2 ++ > include/uapi/linux/devlink.h | 8 +++++++ > net/core/devlink.c | 19 ++++++++++++++- > 4 files changed, 51 insertions(+), 1 deletion(-) > >diff --git a/Documentation/networking/devlink/devlink-flash.rst b/Documentation/networking/devlink/devlink-flash.rst >index 603e732f00cc..1dc373229a54 100644 >--- a/Documentation/networking/devlink/devlink-flash.rst >+++ b/Documentation/networking/devlink/devlink-flash.rst >@@ -44,6 +44,29 @@ preserved across the update. A device may not support every combination and > the driver for such a device must reject any combination which cannot be > faithfully implemented. > >+Dry run >+======= >+ >+Users can request a "dry run" of a flash update by adding the >+``DEVLINK_ATTR_DRY_RUN`` attribute to the ``DEVLINK_CMD_FLASH_UPDATE`` >+command. If the attribute is present, the kernel will only verify that the >+provided command is valid. During a dry run, an update is not performed. >+ >+If supported by the driver, the flash image contents are also validated and >+the driver may indicate whether the file is a valid flash image for the >+device. >+ >+.. code:: shell >+ >+ $ devlink dev flash pci/0000:af:00.0 file image.bin dry-run >+ Validating flash binary >+ >+Note that user space should take care when adding this attribute. Older >+kernels which do not recognize the attribute may accept the command with an >+unknown attribute. This could lead to a request for a dry run which performs >+an unexpected update. To avoid this, user space should check the policy dump >+and verify that the attribute is recognized before adding it to the command. >+ > Firmware Loading > ================ > >diff --git a/include/net/devlink.h b/include/net/devlink.h >index 88c701b375a2..ff5b1e60ad6a 100644 >--- a/include/net/devlink.h >+++ b/include/net/devlink.h >@@ -622,10 +622,12 @@ struct devlink_flash_update_params { > const struct firmware *fw; > const char *component; > u32 overwrite_mask; >+ bool dry_run; > }; > > #define DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT BIT(0) > #define DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK BIT(1) >+#define DEVLINK_SUPPORT_FLASH_UPDATE_DRY_RUN BIT(2) > > struct devlink_region; > struct devlink_info_req; >diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h >index b3d40a5d72ff..e24a5a808a12 100644 >--- a/include/uapi/linux/devlink.h >+++ b/include/uapi/linux/devlink.h >@@ -576,6 +576,14 @@ enum devlink_attr { > DEVLINK_ATTR_LINECARD_TYPE, /* string */ > DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES, /* nested */ > >+ /* Before adding this attribute to a command, user space should check >+ * the policy dump and verify the kernel recognizes the attribute. >+ * Otherwise older kernels which do not recognize the attribute may >+ * silently accept the unknown attribute while not actually performing >+ * a dry run. Why this comment is needed? Isn't that something generic which applies to all new attributes what userspace may pass and kernel may ignore? >+ */ >+ DEVLINK_ATTR_DRY_RUN, /* flag */ >+ > /* add new attributes above here, update the policy in devlink.c */ > > __DEVLINK_ATTR_MAX, >diff --git a/net/core/devlink.c b/net/core/devlink.c >index a9776ea923ae..7d403151bee2 100644 >--- a/net/core/devlink.c >+++ b/net/core/devlink.c >@@ -4736,7 +4736,8 @@ EXPORT_SYMBOL_GPL(devlink_flash_update_timeout_notify); > static int devlink_nl_cmd_flash_update(struct sk_buff *skb, > struct genl_info *info) > { >- struct nlattr *nla_component, *nla_overwrite_mask, *nla_file_name; >+ struct nlattr *nla_component, *nla_overwrite_mask, *nla_file_name, >+ *nla_dry_run; > struct devlink_flash_update_params params = {}; > struct devlink *devlink = info->user_ptr[0]; > const char *file_name; >@@ -4782,6 +4783,21 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb, > return ret; > } > >+ /* Always check dry run last, in order to allow verification of other >+ * parameter support even if the particular driver does not yet >+ * support a full dry-run >+ */ >+ nla_dry_run = info->attrs[DEVLINK_ATTR_DRY_RUN]; >+ if (nla_dry_run) { >+ if (!(supported_params & DEVLINK_SUPPORT_FLASH_UPDATE_DRY_RUN)) { >+ NL_SET_ERR_MSG_ATTR(info->extack, nla_dry_run, >+ "flash update is supported, but dry run is not supported for this device"); >+ release_firmware(params.fw); >+ return -EOPNOTSUPP; >+ } >+ params.dry_run = true; >+ } >+ > devlink_flash_update_begin_notify(devlink); > ret = devlink->ops->flash_update(devlink, ¶ms, info->extack); > devlink_flash_update_end_notify(devlink); >@@ -8997,6 +9013,7 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = { > [DEVLINK_ATTR_RATE_PARENT_NODE_NAME] = { .type = NLA_NUL_STRING }, > [DEVLINK_ATTR_LINECARD_INDEX] = { .type = NLA_U32 }, > [DEVLINK_ATTR_LINECARD_TYPE] = { .type = NLA_NUL_STRING }, >+ [DEVLINK_ATTR_DRY_RUN] = { .type = NLA_FLAG }, > }; > > static const struct genl_small_ops devlink_nl_ops[] = { >-- >2.35.1.456.ga9c7032d4631 >
On Wed, 20 Jul 2022 11:34:32 -0700 Jacob Keller wrote: > + nla_dry_run = info->attrs[DEVLINK_ATTR_DRY_RUN]; > + if (nla_dry_run) { > + if (!(supported_params & DEVLINK_SUPPORT_FLASH_UPDATE_DRY_RUN)) { > + NL_SET_ERR_MSG_ATTR(info->extack, nla_dry_run, > + "flash update is supported, but dry run is not supported for this device"); > + release_firmware(params.fw); > + return -EOPNOTSUPP; > + } > + params.dry_run = true; > + } Looks over-indented. You can do this, right? params.dry_run = nla_get_flag(info->attrs[DEVLINK_ATTR_DRY_RUN]); if (params.dry_run && !(supported_params & DEVLINK_SUPPORT_FLASH_UPDATE_DRY_RUN)) { /* error handling */ }
On Wed, 20 Jul 2022 11:34:32 -0700 Jacob Keller wrote: > Users use the devlink flash interface to request a device driver program or > update the device flash chip. In some cases, a user (or script) may want to > verify that a given flash update command is supported without actually > committing to immediately updating the device. For example, a system > administrator may want to validate that a particular flash binary image > will be accepted by the device, or simply validate a command before finally > committing to it. Also include/net/devlink.h:627: warning: Function parameter or member 'dry_run' not described in 'devlink_flash_update_params'
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Thursday, July 21, 2022 9:48 AM > To: Keller, Jacob E <jacob.e.keller@intel.com> > Cc: netdev@vger.kernel.org > Subject: Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update > > On Wed, 20 Jul 2022 11:34:32 -0700 Jacob Keller wrote: > > + nla_dry_run = info->attrs[DEVLINK_ATTR_DRY_RUN]; > > + if (nla_dry_run) { > > + if (!(supported_params & > DEVLINK_SUPPORT_FLASH_UPDATE_DRY_RUN)) { > > + NL_SET_ERR_MSG_ATTR(info->extack, nla_dry_run, > > + "flash update is supported, but dry run > is not supported for this device"); > > + release_firmware(params.fw); > > + return -EOPNOTSUPP; > > + } > > + params.dry_run = true; > > + } > > Looks over-indented. You can do this, right? > > params.dry_run = nla_get_flag(info->attrs[DEVLINK_ATTR_DRY_RUN]); > if (params.dry_run && > !(supported_params & > DEVLINK_SUPPORT_FLASH_UPDATE_DRY_RUN)) { > /* error handling */ > } Yea that makes more sense. Thanks, Jake
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Thursday, July 21, 2022 9:48 AM > To: Keller, Jacob E <jacob.e.keller@intel.com> > Cc: netdev@vger.kernel.org > Subject: Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update > > On Wed, 20 Jul 2022 11:34:32 -0700 Jacob Keller wrote: > > Users use the devlink flash interface to request a device driver program or > > update the device flash chip. In some cases, a user (or script) may want to > > verify that a given flash update command is supported without actually > > committing to immediately updating the device. For example, a system > > administrator may want to validate that a particular flash binary image > > will be accepted by the device, or simply validate a command before finally > > committing to it. > > Also > > include/net/devlink.h:627: warning: Function parameter or member 'dry_run' > not described in 'devlink_flash_update_params' Will fix. Thanks, Jake
> -----Original Message----- > From: Jiri Pirko <jiri@resnulli.us> > Sent: Wednesday, July 20, 2022 10:55 PM > To: Keller, Jacob E <jacob.e.keller@intel.com> > Cc: netdev@vger.kernel.org; Jakub Kicinski <kuba@kernel.org> > Subject: Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update <...> > > struct devlink_region; > > struct devlink_info_req; > >diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h > >index b3d40a5d72ff..e24a5a808a12 100644 > >--- a/include/uapi/linux/devlink.h > >+++ b/include/uapi/linux/devlink.h > >@@ -576,6 +576,14 @@ enum devlink_attr { > > DEVLINK_ATTR_LINECARD_TYPE, /* string */ > > DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES, /* nested */ > > > >+ /* Before adding this attribute to a command, user space should check > >+ * the policy dump and verify the kernel recognizes the attribute. > >+ * Otherwise older kernels which do not recognize the attribute may > >+ * silently accept the unknown attribute while not actually performing > >+ * a dry run. > > Why this comment is needed? Isn't that something generic which applies > to all new attributes what userspace may pass and kernel may ignore? > Because other attributes may not have such a negative and unexpected side effect. In most cases the side effect will be "the thing you wanted doesn't happen", but in this case its "the thing you didn't want to happen does". I think that deserves some warning. A dry run is a request to *not* do something. Thanks, Jake
Thu, Jul 21, 2022 at 10:32:25PM CEST, jacob.e.keller@intel.com wrote: > > >> -----Original Message----- >> From: Jiri Pirko <jiri@resnulli.us> >> Sent: Wednesday, July 20, 2022 10:55 PM >> To: Keller, Jacob E <jacob.e.keller@intel.com> >> Cc: netdev@vger.kernel.org; Jakub Kicinski <kuba@kernel.org> >> Subject: Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update > ><...> > >> > struct devlink_region; >> > struct devlink_info_req; >> >diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h >> >index b3d40a5d72ff..e24a5a808a12 100644 >> >--- a/include/uapi/linux/devlink.h >> >+++ b/include/uapi/linux/devlink.h >> >@@ -576,6 +576,14 @@ enum devlink_attr { >> > DEVLINK_ATTR_LINECARD_TYPE, /* string */ >> > DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES, /* nested */ >> > >> >+ /* Before adding this attribute to a command, user space should check >> >+ * the policy dump and verify the kernel recognizes the attribute. >> >+ * Otherwise older kernels which do not recognize the attribute may >> >+ * silently accept the unknown attribute while not actually performing >> >+ * a dry run. >> >> Why this comment is needed? Isn't that something generic which applies >> to all new attributes what userspace may pass and kernel may ignore? >> > >Because other attributes may not have such a negative and unexpected side effect. In most cases the side effect will be "the thing you wanted doesn't happen", but in this case its "the thing you didn't want to happen does". I think that deserves some warning. A dry run is a request to *not* do something. Hmm. Another option, in order to be on the safe side, would be to have a new cmd for this... > >Thanks, >Jake
> -----Original Message----- > From: Jiri Pirko <jiri@resnulli.us> > Sent: Thursday, July 21, 2022 11:19 PM > To: Keller, Jacob E <jacob.e.keller@intel.com> > Cc: netdev@vger.kernel.org; Jakub Kicinski <kuba@kernel.org> > Subject: Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update > > Thu, Jul 21, 2022 at 10:32:25PM CEST, jacob.e.keller@intel.com wrote: > > > > > >> -----Original Message----- > >> From: Jiri Pirko <jiri@resnulli.us> > >> Sent: Wednesday, July 20, 2022 10:55 PM > >> To: Keller, Jacob E <jacob.e.keller@intel.com> > >> Cc: netdev@vger.kernel.org; Jakub Kicinski <kuba@kernel.org> > >> Subject: Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash > update > > > ><...> > > > >> > struct devlink_region; > >> > struct devlink_info_req; > >> >diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h > >> >index b3d40a5d72ff..e24a5a808a12 100644 > >> >--- a/include/uapi/linux/devlink.h > >> >+++ b/include/uapi/linux/devlink.h > >> >@@ -576,6 +576,14 @@ enum devlink_attr { > >> > DEVLINK_ATTR_LINECARD_TYPE, /* string */ > >> > DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES, /* nested */ > >> > > >> >+ /* Before adding this attribute to a command, user space should check > >> >+ * the policy dump and verify the kernel recognizes the attribute. > >> >+ * Otherwise older kernels which do not recognize the attribute may > >> >+ * silently accept the unknown attribute while not actually performing > >> >+ * a dry run. > >> > >> Why this comment is needed? Isn't that something generic which applies > >> to all new attributes what userspace may pass and kernel may ignore? > >> > > > >Because other attributes may not have such a negative and unexpected side > effect. In most cases the side effect will be "the thing you wanted doesn't > happen", but in this case its "the thing you didn't want to happen does". I think > that deserves some warning. A dry run is a request to *not* do something. > > Hmm. Another option, in order to be on the safe side, would be to have a > new cmd for this... > I think that the warning and implementation in the iproute2 devlink userspace is sufficient. The alternative would be to work towards converting devlink over to the explicit validation which rejects unknown parameters.. but that has its own backwards compatibility challenges as well. I guess we could use the same code to implement the command so it wouldn't be too much of a burden to add, but that also means we'd have a pattern of using a new command for every future devlink operation that wants a "dry run". I was anticipating we might want this kind of option for other commands such as port splitting and unsplitting. If we were going to add new commands, I would rather we go to the extra trouble of updating all the commands to be strict validation. Thanks, Jake
Fri, Jul 22, 2022 at 11:12:27PM CEST, jacob.e.keller@intel.com wrote: > > >> -----Original Message----- >> From: Jiri Pirko <jiri@resnulli.us> >> Sent: Thursday, July 21, 2022 11:19 PM >> To: Keller, Jacob E <jacob.e.keller@intel.com> >> Cc: netdev@vger.kernel.org; Jakub Kicinski <kuba@kernel.org> >> Subject: Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update >> >> Thu, Jul 21, 2022 at 10:32:25PM CEST, jacob.e.keller@intel.com wrote: >> > >> > >> >> -----Original Message----- >> >> From: Jiri Pirko <jiri@resnulli.us> >> >> Sent: Wednesday, July 20, 2022 10:55 PM >> >> To: Keller, Jacob E <jacob.e.keller@intel.com> >> >> Cc: netdev@vger.kernel.org; Jakub Kicinski <kuba@kernel.org> >> >> Subject: Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash >> update >> > >> ><...> >> > >> >> > struct devlink_region; >> >> > struct devlink_info_req; >> >> >diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h >> >> >index b3d40a5d72ff..e24a5a808a12 100644 >> >> >--- a/include/uapi/linux/devlink.h >> >> >+++ b/include/uapi/linux/devlink.h >> >> >@@ -576,6 +576,14 @@ enum devlink_attr { >> >> > DEVLINK_ATTR_LINECARD_TYPE, /* string */ >> >> > DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES, /* nested */ >> >> > >> >> >+ /* Before adding this attribute to a command, user space should check >> >> >+ * the policy dump and verify the kernel recognizes the attribute. >> >> >+ * Otherwise older kernels which do not recognize the attribute may >> >> >+ * silently accept the unknown attribute while not actually performing >> >> >+ * a dry run. >> >> >> >> Why this comment is needed? Isn't that something generic which applies >> >> to all new attributes what userspace may pass and kernel may ignore? >> >> >> > >> >Because other attributes may not have such a negative and unexpected side >> effect. In most cases the side effect will be "the thing you wanted doesn't >> happen", but in this case its "the thing you didn't want to happen does". I think >> that deserves some warning. A dry run is a request to *not* do something. >> >> Hmm. Another option, in order to be on the safe side, would be to have a >> new cmd for this... >> > >I think that the warning and implementation in the iproute2 devlink userspace is sufficient. The alternative would be to work towards converting devlink over to the explicit validation which rejects unknown parameters.. but that has its own backwards compatibility challenges as well. > >I guess we could use the same code to implement the command so it wouldn't be too much of a burden to add, but that also means we'd have a pattern of using a new command for every future devlink operation that wants a "dry run". I was anticipating we might want this kind of option for other commands such as port splitting and unsplitting. > >If we were going to add new commands, I would rather we go to the extra trouble of updating all the commands to be strict validation. I think it is good idea. We would prevent many surprises. > >Thanks, >Jake
> -----Original Message----- > From: Jiri Pirko <jiri@resnulli.us> > Sent: Saturday, July 23, 2022 8:42 AM > To: Keller, Jacob E <jacob.e.keller@intel.com> > Cc: netdev@vger.kernel.org; Jakub Kicinski <kuba@kernel.org> > Subject: Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update > > Fri, Jul 22, 2022 at 11:12:27PM CEST, jacob.e.keller@intel.com wrote: > > > > > >> -----Original Message----- > >> From: Jiri Pirko <jiri@resnulli.us> > >> Sent: Thursday, July 21, 2022 11:19 PM > >> To: Keller, Jacob E <jacob.e.keller@intel.com> > >> Cc: netdev@vger.kernel.org; Jakub Kicinski <kuba@kernel.org> > >> Subject: Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash > update > >> > >> Thu, Jul 21, 2022 at 10:32:25PM CEST, jacob.e.keller@intel.com wrote: > >> > > >> > > >> >> -----Original Message----- > >> >> From: Jiri Pirko <jiri@resnulli.us> > >> >> Sent: Wednesday, July 20, 2022 10:55 PM > >> >> To: Keller, Jacob E <jacob.e.keller@intel.com> > >> >> Cc: netdev@vger.kernel.org; Jakub Kicinski <kuba@kernel.org> > >> >> Subject: Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash > >> update > >> > > >> ><...> > >> > > >> >> > struct devlink_region; > >> >> > struct devlink_info_req; > >> >> >diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h > >> >> >index b3d40a5d72ff..e24a5a808a12 100644 > >> >> >--- a/include/uapi/linux/devlink.h > >> >> >+++ b/include/uapi/linux/devlink.h > >> >> >@@ -576,6 +576,14 @@ enum devlink_attr { > >> >> > DEVLINK_ATTR_LINECARD_TYPE, /* string */ > >> >> > DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES, /* nested */ > >> >> > > >> >> >+ /* Before adding this attribute to a command, user space should > check > >> >> >+ * the policy dump and verify the kernel recognizes the attribute. > >> >> >+ * Otherwise older kernels which do not recognize the attribute > may > >> >> >+ * silently accept the unknown attribute while not actually > performing > >> >> >+ * a dry run. > >> >> > >> >> Why this comment is needed? Isn't that something generic which applies > >> >> to all new attributes what userspace may pass and kernel may ignore? > >> >> > >> > > >> >Because other attributes may not have such a negative and unexpected side > >> effect. In most cases the side effect will be "the thing you wanted doesn't > >> happen", but in this case its "the thing you didn't want to happen does". I > think > >> that deserves some warning. A dry run is a request to *not* do something. > >> > >> Hmm. Another option, in order to be on the safe side, would be to have a > >> new cmd for this... > >> > > > >I think that the warning and implementation in the iproute2 devlink userspace is > sufficient. The alternative would be to work towards converting devlink over to > the explicit validation which rejects unknown parameters.. but that has its own > backwards compatibility challenges as well. > > > >I guess we could use the same code to implement the command so it wouldn't > be too much of a burden to add, but that also means we'd have a pattern of > using a new command for every future devlink operation that wants a "dry run". > I was anticipating we might want this kind of option for other commands such as > port splitting and unsplitting. > > > >If we were going to add new commands, I would rather we go to the extra > trouble of updating all the commands to be strict validation. > > I think it is good idea. We would prevent many surprises. > I'm not sure exactly what the process would be here. Maybe something like: 1. identify all of the commands which aren't yet strict 2. introduce new command IDs for these commands with something like _STRICT as a suffix? (or something shorter like _2?) 3. make all of those commands strict validation.. but now that I think about that, i am not sure it would work. We use the same attribute list for all devlink commands. This means that strict validation would only check that its passed existing/known attributes? But that doesn't necessarily mean the kernel will process that particular attribute for a given command does it? Like, once we introduce DEVLINK_ATTR_DRY_RUN support for flash, if we then want to introduce it later to something like port splitting.. it would be a valid attribute to send from kernels which support flash but would still be ignored on kernels that don't yet support it for port splitting? Wouldn't we want each individual command to have its own validation of what attributes are valid? I do think its probably a good idea to migrate to strict mode, but I am not sure it solves the problem of dry run. Thoughts? Am I missing something obvious? Would we instead have to convert from genl_small_ops to genl_ops and introduce a policy for each command? I think that sounds like the proper approach here.... Thanks, Jake
On Mon, 25 Jul 2022 19:15:10 +0000 Keller, Jacob E wrote: > I'm not sure exactly what the process would be here. Maybe something > like: > > 1. identify all of the commands which aren't yet strict > 2. introduce new command IDs for these commands with something like > _STRICT as a suffix? (or something shorter like _2?) 3. make all of > those commands strict validation.. > > but now that I think about that, i am not sure it would work. We use > the same attribute list for all devlink commands. This means that > strict validation would only check that its passed existing/known > attributes? But that doesn't necessarily mean the kernel will process > that particular attribute for a given command does it? > > Like, once we introduce DEVLINK_ATTR_DRY_RUN support for flash, if we > then want to introduce it later to something like port splitting.. it > would be a valid attribute to send from kernels which support flash > but would still be ignored on kernels that don't yet support it for > port splitting? > > Wouldn't we want each individual command to have its own validation > of what attributes are valid? > > I do think its probably a good idea to migrate to strict mode, but I > am not sure it solves the problem of dry run. Thoughts? Am I missing > something obvious? > > Would we instead have to convert from genl_small_ops to genl_ops and > introduce a policy for each command? I think that sounds like the > proper approach here.... ...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.
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Monday, July 25, 2022 12:39 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 19:15:10 +0000 Keller, Jacob E wrote: > > I'm not sure exactly what the process would be here. Maybe something > > like: > > > > 1. identify all of the commands which aren't yet strict > > 2. introduce new command IDs for these commands with something like > > _STRICT as a suffix? (or something shorter like _2?) 3. make all of > > those commands strict validation.. > > > > but now that I think about that, i am not sure it would work. We use > > the same attribute list for all devlink commands. This means that > > strict validation would only check that its passed existing/known > > attributes? But that doesn't necessarily mean the kernel will process > > that particular attribute for a given command does it? > > > > Like, once we introduce DEVLINK_ATTR_DRY_RUN support for flash, if we > > then want to introduce it later to something like port splitting.. it > > would be a valid attribute to send from kernels which support flash > > but would still be ignored on kernels that don't yet support it for > > port splitting? > > > > Wouldn't we want each individual command to have its own validation > > of what attributes are valid? > > > > I do think its probably a good idea to migrate to strict mode, but I > > am not sure it solves the problem of dry run. Thoughts? Am I missing > > something obvious? > > > > Would we instead have to convert from genl_small_ops to genl_ops and > > introduce a policy for each command? I think that sounds like the > > proper approach here.... > > ...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. I'll remove the comment though since this problem affects all attributes. Thanks, Jake
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.
> -----Original Message----- > From: Keller, Jacob E <jacob.e.keller@intel.com> > Sent: Monday, July 25, 2022 1:27 PM > To: Jakub Kicinski <kuba@kernel.org> > 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 > > > > > -----Original Message----- > > From: Jakub Kicinski <kuba@kernel.org> > > Sent: Monday, July 25, 2022 12:39 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 19:15:10 +0000 Keller, Jacob E wrote: > > > I'm not sure exactly what the process would be here. Maybe something > > > like: > > > > > > 1. identify all of the commands which aren't yet strict > > > 2. introduce new command IDs for these commands with something like > > > _STRICT as a suffix? (or something shorter like _2?) 3. make all of > > > those commands strict validation.. > > > > > > but now that I think about that, i am not sure it would work. We use > > > the same attribute list for all devlink commands. This means that > > > strict validation would only check that its passed existing/known > > > attributes? But that doesn't necessarily mean the kernel will process > > > that particular attribute for a given command does it? > > > > > > Like, once we introduce DEVLINK_ATTR_DRY_RUN support for flash, if we > > > then want to introduce it later to something like port splitting.. it > > > would be a valid attribute to send from kernels which support flash > > > but would still be ignored on kernels that don't yet support it for > > > port splitting? > > > > > > Wouldn't we want each individual command to have its own validation > > > of what attributes are valid? > > > > > > I do think its probably a good idea to migrate to strict mode, but I > > > am not sure it solves the problem of dry run. Thoughts? Am I missing > > > something obvious? > > > > > > Would we instead have to convert from genl_small_ops to genl_ops and > > > introduce a policy for each command? I think that sounds like the > > > proper approach here.... > > > > ...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. > > I'll remove the comment though since this problem affects all attributes. > > Thanks, > Jake Hmm. The more I think about it, the more it seems that per-command policy is the only way to make the addition of dry_run to new commands safe. Without it, we'd run the risk of a future kernel supporting dry_run but a command not supporting it yet.
> -----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...
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.
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Monday, July 25, 2022 6:14 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: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
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Monday, July 25, 2022 6:14 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 > > 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. I'm also worried about the process for transitioning from the existing non-strict policy to a strict validation of per-command policies. How does that impact backwards compatibilty, and will we need to introduce new ops or not? I know we had to introduce new ops for the strict versions of the PTP ioctls. However those were dealing with (possibly) uninitialized values, where-in userspace may have been accidentally sending values so we could no longer extend the reserved fields. For netlink, in this case the userspace code would need to be intentionally adding netlink attributes. I would think that well behaved userspace would rather get an error when the command isn't honoring an attribute... But in a technical sense that would still be breaking backwards compatibility, since it would cause an application that used to "work" to break. Ofcourse in the case of something like DEVLINK_ATTR_DRY_RUN, the userspace may not be working as intended, and it might be considered a bug. In short: for backwards compatibility, it seems like we might not be able to migrate existing ops to strict validation and would need to replace them? That seems like a very big step...
On Tue, 26 Jul 2022 17:23:53 +0000 Keller, Jacob E wrote: > > 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. Strictness is a separate issue, the policy split addresses just the support discoverability question.
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Tuesday, July 26, 2022 11:49 AM > 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 Tue, 26 Jul 2022 17:23:53 +0000 Keller, Jacob E wrote: > > > 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. > > Strictness is a separate issue, the policy split addresses just > the support discoverability question. Right, ok.
Hi, > -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Monday, July 25, 2022 6:14 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 > > 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. Any update on this? FWIW I started looking at iproute2 code to dump policy and check whether a specific attribute is accepted by the kernel. Thanks, Jake
On Fri, 5 Aug 2022 16:32:30 +0000 Keller, Jacob E wrote: > > 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. > > Any update on this? > > FWIW I started looking at iproute2 code to dump policy and check > whether a specific attribute is accepted by the kernel. Yes and no, I coded a little bit of it up, coincidentally I have a YAML policy for genetlink policy querying if that's helpful: https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git/tree/tools/net/ynl/samples/nlctrl.c?h=gnl-gen-dpll I'll try to wrap up the YAML format by today / tomorrow and send an early RFC, but the codegen part (and everything else really) still requires much work. Probably another month until I can post the first non-RFC with error checking, kernel policy generation, uAPI generation etc.
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Friday, August 05, 2022 11:51 AM > 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 Fri, 5 Aug 2022 16:32:30 +0000 Keller, Jacob E wrote: > > > 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. > > > > Any update on this? > > > > FWIW I started looking at iproute2 code to dump policy and check > > whether a specific attribute is accepted by the kernel. > > Yes and no, I coded a little bit of it up, coincidentally I have a YAML > policy for genetlink policy querying if that's helpful: > > https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git/tree/tools/net/ynl > /samples/nlctrl.c?h=gnl-gen-dpll > I'll take a look at this. > I'll try to wrap up the YAML format by today / tomorrow and send an > early RFC, but the codegen part (and everything else really) still > requires much work. I'd like to see it and provide some early review. > Probably another month until I can post the first > non-RFC with error checking, kernel policy generation, uAPI generation > etc. Ya, I figured this would take quite a lot of effort to get to completion.
diff --git a/Documentation/networking/devlink/devlink-flash.rst b/Documentation/networking/devlink/devlink-flash.rst index 603e732f00cc..1dc373229a54 100644 --- a/Documentation/networking/devlink/devlink-flash.rst +++ b/Documentation/networking/devlink/devlink-flash.rst @@ -44,6 +44,29 @@ preserved across the update. A device may not support every combination and the driver for such a device must reject any combination which cannot be faithfully implemented. +Dry run +======= + +Users can request a "dry run" of a flash update by adding the +``DEVLINK_ATTR_DRY_RUN`` attribute to the ``DEVLINK_CMD_FLASH_UPDATE`` +command. If the attribute is present, the kernel will only verify that the +provided command is valid. During a dry run, an update is not performed. + +If supported by the driver, the flash image contents are also validated and +the driver may indicate whether the file is a valid flash image for the +device. + +.. code:: shell + + $ devlink dev flash pci/0000:af:00.0 file image.bin dry-run + Validating flash binary + +Note that user space should take care when adding this attribute. Older +kernels which do not recognize the attribute may accept the command with an +unknown attribute. This could lead to a request for a dry run which performs +an unexpected update. To avoid this, user space should check the policy dump +and verify that the attribute is recognized before adding it to the command. + Firmware Loading ================ diff --git a/include/net/devlink.h b/include/net/devlink.h index 88c701b375a2..ff5b1e60ad6a 100644 --- a/include/net/devlink.h +++ b/include/net/devlink.h @@ -622,10 +622,12 @@ struct devlink_flash_update_params { const struct firmware *fw; const char *component; u32 overwrite_mask; + bool dry_run; }; #define DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT BIT(0) #define DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK BIT(1) +#define DEVLINK_SUPPORT_FLASH_UPDATE_DRY_RUN BIT(2) struct devlink_region; struct devlink_info_req; diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h index b3d40a5d72ff..e24a5a808a12 100644 --- a/include/uapi/linux/devlink.h +++ b/include/uapi/linux/devlink.h @@ -576,6 +576,14 @@ enum devlink_attr { DEVLINK_ATTR_LINECARD_TYPE, /* string */ DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES, /* nested */ + /* Before adding this attribute to a command, user space should check + * the policy dump and verify the kernel recognizes the attribute. + * Otherwise older kernels which do not recognize the attribute may + * silently accept the unknown attribute while not actually performing + * a dry run. + */ + DEVLINK_ATTR_DRY_RUN, /* flag */ + /* add new attributes above here, update the policy in devlink.c */ __DEVLINK_ATTR_MAX, diff --git a/net/core/devlink.c b/net/core/devlink.c index a9776ea923ae..7d403151bee2 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -4736,7 +4736,8 @@ EXPORT_SYMBOL_GPL(devlink_flash_update_timeout_notify); static int devlink_nl_cmd_flash_update(struct sk_buff *skb, struct genl_info *info) { - struct nlattr *nla_component, *nla_overwrite_mask, *nla_file_name; + struct nlattr *nla_component, *nla_overwrite_mask, *nla_file_name, + *nla_dry_run; struct devlink_flash_update_params params = {}; struct devlink *devlink = info->user_ptr[0]; const char *file_name; @@ -4782,6 +4783,21 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb, return ret; } + /* Always check dry run last, in order to allow verification of other + * parameter support even if the particular driver does not yet + * support a full dry-run + */ + nla_dry_run = info->attrs[DEVLINK_ATTR_DRY_RUN]; + if (nla_dry_run) { + if (!(supported_params & DEVLINK_SUPPORT_FLASH_UPDATE_DRY_RUN)) { + NL_SET_ERR_MSG_ATTR(info->extack, nla_dry_run, + "flash update is supported, but dry run is not supported for this device"); + release_firmware(params.fw); + return -EOPNOTSUPP; + } + params.dry_run = true; + } + devlink_flash_update_begin_notify(devlink); ret = devlink->ops->flash_update(devlink, ¶ms, info->extack); devlink_flash_update_end_notify(devlink); @@ -8997,6 +9013,7 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = { [DEVLINK_ATTR_RATE_PARENT_NODE_NAME] = { .type = NLA_NUL_STRING }, [DEVLINK_ATTR_LINECARD_INDEX] = { .type = NLA_U32 }, [DEVLINK_ATTR_LINECARD_TYPE] = { .type = NLA_NUL_STRING }, + [DEVLINK_ATTR_DRY_RUN] = { .type = NLA_FLAG }, }; static const struct genl_small_ops devlink_nl_ops[] = {
Users use the devlink flash interface to request a device driver program or update the device flash chip. In some cases, a user (or script) may want to verify that a given flash update command is supported without actually committing to immediately updating the device. For example, a system administrator may want to validate that a particular flash binary image will be accepted by the device, or simply validate a command before finally committing to it. The current flash update interface lacks a method to support such a dry run. Add a new DEVLINK_ATTR_DRY_RUN attribute which shall be used by a devlink command to indicate that a request is a dry run which should not perform device configuration. Instead, the command should report whether the command or configuration request is valid. While we can validate the initial arguments of the devlink command, a proper dry run must be processed by the device driver. This is required because only the driver can perform validation of the flash binary file. Add a new dry_run parameter to the devlink_flash_update_params struct, along with the associated bit to indicate if a driver supports verifying a dry run. We always check the dry run attribute last in order to allow as much verification of other parameters as possible. For example, even if a driver does not support the dry_run option, we can still validate the other optional parameters such as the overwrite_mask and per-component update name. Document that userspace should take care when issuing a dry run to older kernels, as the flash update command is not strictly verified. Thus, unknown attributes will be ignored and this could cause a request for a dry run to perform an actual update. We can't fix old kernels to verify unknown attributes, but userspace can check the maximum attribute and reject the dry run request if it is not supported by the kernel. Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> --- .../networking/devlink/devlink-flash.rst | 23 +++++++++++++++++++ include/net/devlink.h | 2 ++ include/uapi/linux/devlink.h | 8 +++++++ net/core/devlink.c | 19 ++++++++++++++- 4 files changed, 51 insertions(+), 1 deletion(-)