diff mbox series

[iproute2-next,v2,3/3] devlink: add dry run attribute support to devlink flash

Message ID 20220721211451.2475600-7-jacob.e.keller@intel.com (mailing list archive)
State Superseded
Delegated to: David Ahern
Headers show
Series devlink: implement dry run support for flash update | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/apply fail Patch does not apply to net-next

Commit Message

Jacob Keller July 21, 2022, 9:14 p.m. UTC 
Recent versions of the kernel support the DEVLINK_ATTR_DRY_RUN attribute
which allows requesting a dry run of a command. A dry run is simply
a request to validate that a command would work, without performing any
destructive changes.

The attribute is supported by the devlink flash update as a way to
validate an update, including potentially the binary image, without
modifying the device.

Add a "dry_run" option to the command line parsing which will enable
this attribute when requested.

To avoid potential issues, only allow the attribute to be added to
commands when the kernel recognizes it. This is important because some
commands do not perform strict validation. If we were to add the
attribute without this check, an old kernel may silently accept the
command and perform an update even when dry_run was requested.

Before adding the attribute, check the maximum attribute from the
CTRL_CMD_GETFAMILY and make sure that the kernel recognizes the
DEVLINK_ATTR_DRY_RUN attribute.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Changes since v1
* Make dl_kernel_supports_dry_run more generic by passing attribute

 devlink/devlink.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 43 insertions(+), 2 deletions(-)

Comments

Jiri Pirko July 22, 2022, 6:22 a.m. UTC | #1 
Thu, Jul 21, 2022 at 11:14:51PM CEST, jacob.e.keller@intel.com wrote:
>Recent versions of the kernel support the DEVLINK_ATTR_DRY_RUN attribute
>which allows requesting a dry run of a command. A dry run is simply
>a request to validate that a command would work, without performing any
>destructive changes.
>
>The attribute is supported by the devlink flash update as a way to
>validate an update, including potentially the binary image, without
>modifying the device.
>
>Add a "dry_run" option to the command line parsing which will enable
>this attribute when requested.
>
>To avoid potential issues, only allow the attribute to be added to
>commands when the kernel recognizes it. This is important because some
>commands do not perform strict validation. If we were to add the
>attribute without this check, an old kernel may silently accept the
>command and perform an update even when dry_run was requested.
>
>Before adding the attribute, check the maximum attribute from the
>CTRL_CMD_GETFAMILY and make sure that the kernel recognizes the
>DEVLINK_ATTR_DRY_RUN attribute.
>
>Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>---
>Changes since v1
>* Make dl_kernel_supports_dry_run more generic by passing attribute
>
> devlink/devlink.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 43 insertions(+), 2 deletions(-)
>
>diff --git a/devlink/devlink.c b/devlink/devlink.c
>index 1e2cfc3d4285..24f1a70a9656 100644
>--- a/devlink/devlink.c
>+++ b/devlink/devlink.c
>@@ -296,6 +296,7 @@ static void ifname_map_free(struct ifname_map *ifname_map)
> #define DL_OPT_PORT_FN_RATE_PARENT	BIT(51)
> #define DL_OPT_LINECARD		BIT(52)
> #define DL_OPT_LINECARD_TYPE	BIT(53)
>+#define DL_OPT_DRY_RUN			BIT(54)
> 
> struct dl_opts {
> 	uint64_t present; /* flags of present items */
>@@ -372,6 +373,8 @@ struct dl {
> 	bool verbose;
> 	bool stats;
> 	bool hex;
>+	bool max_attr_valid;
>+	uint32_t max_attr;
> 	struct {
> 		bool present;
> 		char *bus_name;
>@@ -701,6 +704,7 @@ static const enum mnl_attr_data_type devlink_policy[DEVLINK_ATTR_MAX + 1] = {
> 	[DEVLINK_ATTR_LINECARD_STATE] = MNL_TYPE_U8,
> 	[DEVLINK_ATTR_LINECARD_TYPE] = MNL_TYPE_STRING,
> 	[DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES] = MNL_TYPE_NESTED,
>+	[DEVLINK_ATTR_DRY_RUN] = MNL_TYPE_FLAG,
> };
> 
> static const enum mnl_attr_data_type
>@@ -1522,6 +1526,30 @@ static int dl_args_finding_required_validate(uint64_t o_required,
> 	return 0;
> }
> 
>+static void dl_get_max_attr(struct dl *dl)
>+{
>+	if (!dl->max_attr_valid) {

if (dl->max_attr_valid)
	return;

and then you can drop the indent.


>+		uint32_t max_attr;
>+		int err;
>+
>+		err = mnlg_socket_get_max_attr(&dl->nlg, &max_attr);
>+		if (err) {
>+			pr_err("Unable to determine maximum supported devlink attribute\n");
>+			return;
>+		}
>+
>+		dl->max_attr = max_attr;
>+		dl->max_attr_valid = true;
>+	}
>+}
>+
>+static bool dl_kernel_supports_attr(struct dl *dl, enum devlink_attr attr)
>+{
>+	dl_get_max_attr(dl);
>+
>+	return (dl->max_attr_valid && dl->max_attr >= attr);

Return is not a function. Drop the "()" here.


>+}
>+
> static int dl_argv_parse(struct dl *dl, uint64_t o_required,
> 			 uint64_t o_optional)
> {
>@@ -2037,6 +2065,16 @@ static int dl_argv_parse(struct dl *dl, uint64_t o_required,
> 			dl_arg_inc(dl);
> 			opts->linecard_type = "";
> 			o_found |= DL_OPT_LINECARD_TYPE;
>+		} else if (dl_argv_match(dl, "dry_run") &&
>+			   (o_all & DL_OPT_DRY_RUN)) {
>+
>+			if (!dl_kernel_supports_attr(dl, DEVLINK_ATTR_DRY_RUN)) {
>+				pr_err("Kernel does not support dry_run attribute\n");
>+				return -EOPNOTSUPP;
>+			}
>+
>+			dl_arg_inc(dl);
>+			o_found |= DL_OPT_DRY_RUN;
> 		} else {
> 			pr_err("Unknown option \"%s\"\n", dl_argv(dl));
> 			return -EINVAL;
>@@ -2115,6 +2153,8 @@ static void dl_opts_put(struct nlmsghdr *nlh, struct dl *dl)
> 		mnl_attr_put_strz(nlh, DEVLINK_ATTR_RATE_NODE_NAME,
> 				  opts->rate_node_name);
> 	}
>+	if (opts->present & DL_OPT_DRY_RUN)
>+		mnl_attr_put(nlh, DEVLINK_ATTR_DRY_RUN, 0, NULL);
> 	if (opts->present & DL_OPT_PORT_TYPE)
> 		mnl_attr_put_u16(nlh, DEVLINK_ATTR_PORT_TYPE,
> 				 opts->port_type);
>@@ -2326,7 +2366,7 @@ static void cmd_dev_help(void)
> 	pr_err("       devlink dev reload DEV [ netns { PID | NAME | ID } ]\n");
> 	pr_err("                              [ action { driver_reinit | fw_activate } ] [ limit no_reset ]\n");
> 	pr_err("       devlink dev info [ DEV ]\n");
>-	pr_err("       devlink dev flash DEV file PATH [ component NAME ] [ overwrite SECTION ]\n");
>+	pr_err("       devlink dev flash DEV file PATH [ component NAME ] [ overwrite SECTION ] [ dry_run ]\n");
> }
> 
> static bool cmp_arr_last_handle(struct dl *dl, const char *bus_name,
>@@ -3886,7 +3926,8 @@ static int cmd_dev_flash(struct dl *dl)
> 			       NLM_F_REQUEST | NLM_F_ACK);
> 
> 	err = dl_argv_parse_put(nlh, dl, DL_OPT_HANDLE | DL_OPT_FLASH_FILE_NAME,
>-				DL_OPT_FLASH_COMPONENT | DL_OPT_FLASH_OVERWRITE);
>+				DL_OPT_FLASH_COMPONENT | DL_OPT_FLASH_OVERWRITE |
>+				DL_OPT_DRY_RUN);
> 	if (err)
> 		return err;
> 
>-- 
>2.36.1
>
Jacob Keller July 22, 2022, 9:14 p.m. UTC | #2 
> -----Original Message-----
> From: Jiri Pirko <jiri@resnulli.us>
> Sent: Thursday, July 21, 2022 11:22 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: [iproute2-next v2 3/3] devlink: add dry run attribute support to
> devlink flash
> 
> Thu, Jul 21, 2022 at 11:14:51PM CEST, jacob.e.keller@intel.com wrote:
> >Recent versions of the kernel support the DEVLINK_ATTR_DRY_RUN attribute
> >which allows requesting a dry run of a command. A dry run is simply
> >a request to validate that a command would work, without performing any
> >destructive changes.
> >
> >The attribute is supported by the devlink flash update as a way to
> >validate an update, including potentially the binary image, without
> >modifying the device.
> >
> >Add a "dry_run" option to the command line parsing which will enable
> >this attribute when requested.
> >
> >To avoid potential issues, only allow the attribute to be added to
> >commands when the kernel recognizes it. This is important because some
> >commands do not perform strict validation. If we were to add the
> >attribute without this check, an old kernel may silently accept the
> >command and perform an update even when dry_run was requested.
> >
> >Before adding the attribute, check the maximum attribute from the
> >CTRL_CMD_GETFAMILY and make sure that the kernel recognizes the
> >DEVLINK_ATTR_DRY_RUN attribute.
> >
> >Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> >---
> >Changes since v1
> >* Make dl_kernel_supports_dry_run more generic by passing attribute
> >
> > devlink/devlink.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 43 insertions(+), 2 deletions(-)
> >
> >diff --git a/devlink/devlink.c b/devlink/devlink.c
> >index 1e2cfc3d4285..24f1a70a9656 100644
> >--- a/devlink/devlink.c
> >+++ b/devlink/devlink.c
> >@@ -296,6 +296,7 @@ static void ifname_map_free(struct ifname_map
> *ifname_map)
> > #define DL_OPT_PORT_FN_RATE_PARENT	BIT(51)
> > #define DL_OPT_LINECARD		BIT(52)
> > #define DL_OPT_LINECARD_TYPE	BIT(53)
> >+#define DL_OPT_DRY_RUN			BIT(54)
> >
> > struct dl_opts {
> > 	uint64_t present; /* flags of present items */
> >@@ -372,6 +373,8 @@ struct dl {
> > 	bool verbose;
> > 	bool stats;
> > 	bool hex;
> >+	bool max_attr_valid;
> >+	uint32_t max_attr;
> > 	struct {
> > 		bool present;
> > 		char *bus_name;
> >@@ -701,6 +704,7 @@ static const enum mnl_attr_data_type
> devlink_policy[DEVLINK_ATTR_MAX + 1] = {
> > 	[DEVLINK_ATTR_LINECARD_STATE] = MNL_TYPE_U8,
> > 	[DEVLINK_ATTR_LINECARD_TYPE] = MNL_TYPE_STRING,
> > 	[DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES] = MNL_TYPE_NESTED,
> >+	[DEVLINK_ATTR_DRY_RUN] = MNL_TYPE_FLAG,
> > };
> >
> > static const enum mnl_attr_data_type
> >@@ -1522,6 +1526,30 @@ static int dl_args_finding_required_validate(uint64_t
> o_required,
> > 	return 0;
> > }
> >
> >+static void dl_get_max_attr(struct dl *dl)
> >+{
> >+	if (!dl->max_attr_valid) {
> 
> if (dl->max_attr_valid)
> 	return;
> 
> and then you can drop the indent.
> 


Yep, makes sense.

> 
> >+		uint32_t max_attr;
> >+		int err;
> >+
> >+		err = mnlg_socket_get_max_attr(&dl->nlg, &max_attr);
> >+		if (err) {
> >+			pr_err("Unable to determine maximum supported
> devlink attribute\n");
> >+			return;
> >+		}
> >+
> >+		dl->max_attr = max_attr;
> >+		dl->max_attr_valid = true;
> >+	}
> >+}
> >+
> >+static bool dl_kernel_supports_attr(struct dl *dl, enum devlink_attr attr)
> >+{
> >+	dl_get_max_attr(dl);
> >+
> >+	return (dl->max_attr_valid && dl->max_attr >= attr);
> 
> Return is not a function. Drop the "()" here.
> 

Will fix.

> 
> >+}
> >+
> > static int dl_argv_parse(struct dl *dl, uint64_t o_required,
> > 			 uint64_t o_optional)
> > {
> >@@ -2037,6 +2065,16 @@ static int dl_argv_parse(struct dl *dl, uint64_t
> o_required,
> > 			dl_arg_inc(dl);
> > 			opts->linecard_type = "";
> > 			o_found |= DL_OPT_LINECARD_TYPE;
> >+		} else if (dl_argv_match(dl, "dry_run") &&
> >+			   (o_all & DL_OPT_DRY_RUN)) {
> >+
> >+			if (!dl_kernel_supports_attr(dl,
> DEVLINK_ATTR_DRY_RUN)) {
> >+				pr_err("Kernel does not support dry_run
> attribute\n");
> >+				return -EOPNOTSUPP;
> >+			}
> >+
> >+			dl_arg_inc(dl);
> >+			o_found |= DL_OPT_DRY_RUN;
> > 		} else {
> > 			pr_err("Unknown option \"%s\"\n", dl_argv(dl));
> > 			return -EINVAL;
> >@@ -2115,6 +2153,8 @@ static void dl_opts_put(struct nlmsghdr *nlh, struct dl
> *dl)
> > 		mnl_attr_put_strz(nlh, DEVLINK_ATTR_RATE_NODE_NAME,
> > 				  opts->rate_node_name);
> > 	}
> >+	if (opts->present & DL_OPT_DRY_RUN)
> >+		mnl_attr_put(nlh, DEVLINK_ATTR_DRY_RUN, 0, NULL);
> > 	if (opts->present & DL_OPT_PORT_TYPE)
> > 		mnl_attr_put_u16(nlh, DEVLINK_ATTR_PORT_TYPE,
> > 				 opts->port_type);
> >@@ -2326,7 +2366,7 @@ static void cmd_dev_help(void)
> > 	pr_err("       devlink dev reload DEV [ netns { PID | NAME | ID } ]\n");
> > 	pr_err("                              [ action { driver_reinit | fw_activate } ] [ limit
> no_reset ]\n");
> > 	pr_err("       devlink dev info [ DEV ]\n");
> >-	pr_err("       devlink dev flash DEV file PATH [ component NAME ] [
> overwrite SECTION ]\n");
> >+	pr_err("       devlink dev flash DEV file PATH [ component NAME ] [
> overwrite SECTION ] [ dry_run ]\n");
> > }
> >
> > static bool cmp_arr_last_handle(struct dl *dl, const char *bus_name,
> >@@ -3886,7 +3926,8 @@ static int cmd_dev_flash(struct dl *dl)
> > 			       NLM_F_REQUEST | NLM_F_ACK);
> >
> > 	err = dl_argv_parse_put(nlh, dl, DL_OPT_HANDLE |
> DL_OPT_FLASH_FILE_NAME,
> >-				DL_OPT_FLASH_COMPONENT |
> DL_OPT_FLASH_OVERWRITE);
> >+				DL_OPT_FLASH_COMPONENT |
> DL_OPT_FLASH_OVERWRITE |
> >+				DL_OPT_DRY_RUN);
> > 	if (err)
> > 		return err;
> >
> >--
> >2.36.1
> >
diff mbox series

Patch

diff --git a/devlink/devlink.c b/devlink/devlink.c
index 1e2cfc3d4285..24f1a70a9656 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -296,6 +296,7 @@  static void ifname_map_free(struct ifname_map *ifname_map)
 #define DL_OPT_PORT_FN_RATE_PARENT	BIT(51)
 #define DL_OPT_LINECARD		BIT(52)
 #define DL_OPT_LINECARD_TYPE	BIT(53)
+#define DL_OPT_DRY_RUN			BIT(54)
 
 struct dl_opts {
 	uint64_t present; /* flags of present items */
@@ -372,6 +373,8 @@  struct dl {
 	bool verbose;
 	bool stats;
 	bool hex;
+	bool max_attr_valid;
+	uint32_t max_attr;
 	struct {
 		bool present;
 		char *bus_name;
@@ -701,6 +704,7 @@  static const enum mnl_attr_data_type devlink_policy[DEVLINK_ATTR_MAX + 1] = {
 	[DEVLINK_ATTR_LINECARD_STATE] = MNL_TYPE_U8,
 	[DEVLINK_ATTR_LINECARD_TYPE] = MNL_TYPE_STRING,
 	[DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES] = MNL_TYPE_NESTED,
+	[DEVLINK_ATTR_DRY_RUN] = MNL_TYPE_FLAG,
 };
 
 static const enum mnl_attr_data_type
@@ -1522,6 +1526,30 @@  static int dl_args_finding_required_validate(uint64_t o_required,
 	return 0;
 }
 
+static void dl_get_max_attr(struct dl *dl)
+{
+	if (!dl->max_attr_valid) {
+		uint32_t max_attr;
+		int err;
+
+		err = mnlg_socket_get_max_attr(&dl->nlg, &max_attr);
+		if (err) {
+			pr_err("Unable to determine maximum supported devlink attribute\n");
+			return;
+		}
+
+		dl->max_attr = max_attr;
+		dl->max_attr_valid = true;
+	}
+}
+
+static bool dl_kernel_supports_attr(struct dl *dl, enum devlink_attr attr)
+{
+	dl_get_max_attr(dl);
+
+	return (dl->max_attr_valid && dl->max_attr >= attr);
+}
+
 static int dl_argv_parse(struct dl *dl, uint64_t o_required,
 			 uint64_t o_optional)
 {
@@ -2037,6 +2065,16 @@  static int dl_argv_parse(struct dl *dl, uint64_t o_required,
 			dl_arg_inc(dl);
 			opts->linecard_type = "";
 			o_found |= DL_OPT_LINECARD_TYPE;
+		} else if (dl_argv_match(dl, "dry_run") &&
+			   (o_all & DL_OPT_DRY_RUN)) {
+
+			if (!dl_kernel_supports_attr(dl, DEVLINK_ATTR_DRY_RUN)) {
+				pr_err("Kernel does not support dry_run attribute\n");
+				return -EOPNOTSUPP;
+			}
+
+			dl_arg_inc(dl);
+			o_found |= DL_OPT_DRY_RUN;
 		} else {
 			pr_err("Unknown option \"%s\"\n", dl_argv(dl));
 			return -EINVAL;
@@ -2115,6 +2153,8 @@  static void dl_opts_put(struct nlmsghdr *nlh, struct dl *dl)
 		mnl_attr_put_strz(nlh, DEVLINK_ATTR_RATE_NODE_NAME,
 				  opts->rate_node_name);
 	}
+	if (opts->present & DL_OPT_DRY_RUN)
+		mnl_attr_put(nlh, DEVLINK_ATTR_DRY_RUN, 0, NULL);
 	if (opts->present & DL_OPT_PORT_TYPE)
 		mnl_attr_put_u16(nlh, DEVLINK_ATTR_PORT_TYPE,
 				 opts->port_type);
@@ -2326,7 +2366,7 @@  static void cmd_dev_help(void)
 	pr_err("       devlink dev reload DEV [ netns { PID | NAME | ID } ]\n");
 	pr_err("                              [ action { driver_reinit | fw_activate } ] [ limit no_reset ]\n");
 	pr_err("       devlink dev info [ DEV ]\n");
-	pr_err("       devlink dev flash DEV file PATH [ component NAME ] [ overwrite SECTION ]\n");
+	pr_err("       devlink dev flash DEV file PATH [ component NAME ] [ overwrite SECTION ] [ dry_run ]\n");
 }
 
 static bool cmp_arr_last_handle(struct dl *dl, const char *bus_name,
@@ -3886,7 +3926,8 @@  static int cmd_dev_flash(struct dl *dl)
 			       NLM_F_REQUEST | NLM_F_ACK);
 
 	err = dl_argv_parse_put(nlh, dl, DL_OPT_HANDLE | DL_OPT_FLASH_FILE_NAME,
-				DL_OPT_FLASH_COMPONENT | DL_OPT_FLASH_OVERWRITE);
+				DL_OPT_FLASH_COMPONENT | DL_OPT_FLASH_OVERWRITE |
+				DL_OPT_DRY_RUN);
 	if (err)
 		return err;