| Message ID | 20220721211451.2475600-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 |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
Thu, Jul 21, 2022 at 11:14:46PM 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> >--- > >Changes since v1: >* Add kernel doc comments to devlink_flash_update_params >* Reduce indentation by using nla_get_flag > > .../networking/devlink/devlink-flash.rst | 23 +++++++++++++++++++ > include/net/devlink.h | 4 ++++ > include/uapi/linux/devlink.h | 8 +++++++ > net/core/devlink.c | 17 +++++++++++++- > 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 780744b550b8..47b86ccb85b0 100644 >--- a/include/net/devlink.h >+++ b/include/net/devlink.h >@@ -613,6 +613,8 @@ enum devlink_param_generic_id { > * struct devlink_flash_update_params - Flash Update parameters > * @fw: pointer to the firmware data to update from > * @component: the flash component to update >+ * @overwrite_mask: what sections of flash can be overwritten Well, strictly speaking, this is not related to this patch and should be done in a separate one. But hey, it's a comment, so I guess noone really cares. >+ * @dry_run: if true, do not actually update the flash > * > * With the exception of fw, drivers must opt-in to parameters by > * setting the appropriate bit in the supported_flash_update_params field in >@@ -622,10 +624,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 98d79feeb3dc..1cff636c9b2b 100644 >--- a/net/core/devlink.c >+++ b/net/core/devlink.c >@@ -4743,7 +4743,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; >@@ -4789,6 +4790,19 @@ 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 >+ */ >+ params.dry_run = nla_get_flag(info->attrs[DEVLINK_ATTR_DRY_RUN]); >+ if (params.dry_run && >+ !(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; >+ } >+ > devlink_flash_update_begin_notify(devlink); > ret = devlink->ops->flash_update(devlink, ¶ms, info->extack); > devlink_flash_update_end_notify(devlink); >@@ -9004,6 +9018,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 > Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> -----Original Message----- > From: Jiri Pirko <jiri@resnulli.us> > Sent: Thursday, July 21, 2022 11:26 PM > To: Keller, Jacob E <jacob.e.keller@intel.com> > Cc: netdev@vger.kernel.org; Jonathan Corbet <corbet@lwn.net>; Jiri Pirko > <jiri@nvidia.com>; David S. Miller <davem@davemloft.net>; Eric Dumazet > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni > <pabeni@redhat.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; > David Ahern <dsahern@kernel.org>; Stephen Hemminger > <stephen@networkplumber.org>; linux-doc@vger.kernel.org; intel-wired- > lan@lists.osuosl.org > Subject: Re: [net-next v2 1/2] devlink: add dry run attribute to flash update > > Thu, Jul 21, 2022 at 11:14:46PM 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> > >--- > > > >Changes since v1: > >* Add kernel doc comments to devlink_flash_update_params > >* Reduce indentation by using nla_get_flag > > > > .../networking/devlink/devlink-flash.rst | 23 +++++++++++++++++++ > > include/net/devlink.h | 4 ++++ > > include/uapi/linux/devlink.h | 8 +++++++ > > net/core/devlink.c | 17 +++++++++++++- > > 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 780744b550b8..47b86ccb85b0 100644 > >--- a/include/net/devlink.h > >+++ b/include/net/devlink.h > >@@ -613,6 +613,8 @@ enum devlink_param_generic_id { > > * struct devlink_flash_update_params - Flash Update parameters > > * @fw: pointer to the firmware data to update from > > * @component: the flash component to update > >+ * @overwrite_mask: what sections of flash can be overwritten > > Well, strictly speaking, this is not related to this patch and should be > done in a separate one. But hey, it's a comment, so I guess noone really > cares. > Ah yep. I'll split this out since I need to send a new version to split the iproute stuff into its own thread anyways.
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 780744b550b8..47b86ccb85b0 100644 --- a/include/net/devlink.h +++ b/include/net/devlink.h @@ -613,6 +613,8 @@ enum devlink_param_generic_id { * struct devlink_flash_update_params - Flash Update parameters * @fw: pointer to the firmware data to update from * @component: the flash component to update + * @overwrite_mask: what sections of flash can be overwritten + * @dry_run: if true, do not actually update the flash * * With the exception of fw, drivers must opt-in to parameters by * setting the appropriate bit in the supported_flash_update_params field in @@ -622,10 +624,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 98d79feeb3dc..1cff636c9b2b 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -4743,7 +4743,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; @@ -4789,6 +4790,19 @@ 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 + */ + params.dry_run = nla_get_flag(info->attrs[DEVLINK_ATTR_DRY_RUN]); + if (params.dry_run && + !(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; + } + devlink_flash_update_begin_notify(devlink); ret = devlink->ops->flash_update(devlink, ¶ms, info->extack); devlink_flash_update_end_notify(devlink); @@ -9004,6 +9018,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> --- Changes since v1: * Add kernel doc comments to devlink_flash_update_params * Reduce indentation by using nla_get_flag .../networking/devlink/devlink-flash.rst | 23 +++++++++++++++++++ include/net/devlink.h | 4 ++++ include/uapi/linux/devlink.h | 8 +++++++ net/core/devlink.c | 17 +++++++++++++- 4 files changed, 51 insertions(+), 1 deletion(-)