netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [iproute2-next PATCH 0/3] devlink: support dry run attribute for flash update
@ 2022-07-20 18:34 Jacob Keller
  2022-07-20 18:34 ` [iproute2-next PATCH 1/3] update <linux/devlink.h> UAPI header Jacob Keller
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Jacob Keller @ 2022-07-20 18:34 UTC (permalink / raw)
  To: netdev; +Cc: Jakub Kicinski, Jacob Keller

Allow users to request a dry run of a flash update by adding the
DEVLINK_ATTR_DRY_RUN.

Because many devlink commands do not validate and reject unknown attributes,
this could have unexpected side effects on older kernels which lack the
attribute. To handle this, check the socket and determine the maximum
attribute the kernel supports. Only allow passing the DEVLINK_ATTR_DRY_RUN
for kernels which have the attribute.

This allows a user to validate that a flash update will be accepted by the
driver and device without being forced to commit to updating.

Jacob Keller (3):
  update <linux/devlink.h> UAPI header
  mnlg: add function to get CTRL_ATTR_MAXATTR value
  devlink: add dry run attribute support to devlink flash

 devlink/devlink.c            | 45 +++++++++++++++++++++++++++--
 devlink/mnlg.c               | 56 ++++++++++++++++++++++++++++++++++++
 devlink/mnlg.h               |  1 +
 include/uapi/linux/devlink.h |  8 ++++++
 4 files changed, 108 insertions(+), 2 deletions(-)

-- 
2.36.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [iproute2-next PATCH 1/3] update <linux/devlink.h> UAPI header
  2022-07-20 18:34 [iproute2-next PATCH 0/3] devlink: support dry run attribute for flash update Jacob Keller
@ 2022-07-20 18:34 ` Jacob Keller
  2022-07-20 18:34 ` [iproute2-next PATCH 2/3] mnlg: add function to get CTRL_ATTR_MAXATTR value Jacob Keller
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 7+ messages in thread
From: Jacob Keller @ 2022-07-20 18:34 UTC (permalink / raw)
  To: netdev; +Cc: Jakub Kicinski, Jacob Keller

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 include/uapi/linux/devlink.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index da0f1ba8f7a0..90f6cf97d308 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,
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [iproute2-next PATCH 2/3] mnlg: add function to get CTRL_ATTR_MAXATTR value
  2022-07-20 18:34 [iproute2-next PATCH 0/3] devlink: support dry run attribute for flash update Jacob Keller
  2022-07-20 18:34 ` [iproute2-next PATCH 1/3] update <linux/devlink.h> UAPI header Jacob Keller
@ 2022-07-20 18:34 ` Jacob Keller
  2022-07-20 18:34 ` [iproute2-next PATCH 3/3] devlink: add dry run attribute support to devlink flash Jacob Keller
  2022-07-21  5:59 ` [iproute2-next PATCH 0/3] devlink: support dry run attribute for flash update Jiri Pirko
  3 siblings, 0 replies; 7+ messages in thread
From: Jacob Keller @ 2022-07-20 18:34 UTC (permalink / raw)
  To: netdev; +Cc: Jakub Kicinski, Jacob Keller

Add a new function to extract the CTRL_ATTR_MAXATTR attribute of the
CTRL_CMD_GETFAMILY request. This will be used to allow reading the
maximum supported devlink attribute of the running kernel in an upcoming
change.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 devlink/mnlg.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++
 devlink/mnlg.h |  1 +
 2 files changed, 57 insertions(+)

diff --git a/devlink/mnlg.c b/devlink/mnlg.c
index e6d92742c150..348c3ff5c959 100644
--- a/devlink/mnlg.c
+++ b/devlink/mnlg.c
@@ -41,6 +41,10 @@ struct group_info {
 	const char *name;
 };
 
+struct family_info {
+	uint32_t max_attr;
+};
+
 static int parse_mc_grps_cb(const struct nlattr *attr, void *data)
 {
 	const struct nlattr **tb = data;
@@ -149,6 +153,58 @@ int mnlg_socket_group_add(struct mnlu_gen_socket *nlg, const char *group_name)
 	return 0;
 }
 
+static int get_family_attr_cb(const struct nlattr *attr, void *data)
+{
+	const struct nlattr **tb = data;
+	int type = mnl_attr_get_type(attr);
+
+	if (mnl_attr_type_valid(attr, CTRL_ATTR_MAX) < 0)
+		return MNL_CB_ERROR;
+
+	tb[type] = attr;
+	return MNL_CB_OK;
+}
+
+static int get_family_cb(const struct nlmsghdr *nlh, void *data)
+{
+	struct family_info *family = data;
+	struct nlattr *tb[CTRL_ATTR_MAX + 1] = {};
+	struct genlmsghdr *genl = mnl_nlmsg_get_payload(nlh);
+
+	mnl_attr_parse(nlh, sizeof(*genl), get_family_attr_cb, tb);
+	if (!tb[CTRL_ATTR_MAXATTR])
+		return MNL_CB_ERROR;
+
+	family->max_attr = mnl_attr_get_u32(tb[CTRL_ATTR_MAXATTR]);
+
+	return MNL_CB_OK;
+}
+
+int mnlg_socket_get_max_attr(struct mnlu_gen_socket *nlg, uint32_t *max_attr)
+{
+	struct nlmsghdr *nlh;
+	struct family_info family;
+	int err;
+
+	nlh = _mnlu_gen_socket_cmd_prepare(nlg, CTRL_CMD_GETFAMILY,
+					   NLM_F_REQUEST | NLM_F_ACK,
+					   GENL_ID_CTRL, 1);
+
+	mnl_attr_put_u16(nlh, CTRL_ATTR_FAMILY_ID, nlg->family);
+
+	err = mnlg_socket_send(nlg, nlh);
+	if (err < 0)
+		return err;
+
+	err = mnlu_gen_socket_recv_run(nlg, get_family_cb, &family);
+	if (err < 0)
+		return err;
+
+	*max_attr = family.max_attr;
+
+	return 0;
+}
+
 int mnlg_socket_get_fd(struct mnlu_gen_socket *nlg)
 {
 	return mnl_socket_get_fd(nlg->nl);
diff --git a/devlink/mnlg.h b/devlink/mnlg.h
index 24aa17566a9b..6348ad45ed26 100644
--- a/devlink/mnlg.h
+++ b/devlink/mnlg.h
@@ -18,6 +18,7 @@ struct mnlu_gen_socket;
 
 int mnlg_socket_send(struct mnlu_gen_socket *nlg, const struct nlmsghdr *nlh);
 int mnlg_socket_group_add(struct mnlu_gen_socket *nlg, const char *group_name);
+int mnlg_socket_get_max_attr(struct mnlu_gen_socket *nlg, uint32_t *max_attr);
 int mnlg_socket_get_fd(struct mnlu_gen_socket *nlg);
 
 #endif /* _MNLG_H_ */
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [iproute2-next PATCH 3/3] devlink: add dry run attribute support to devlink flash
  2022-07-20 18:34 [iproute2-next PATCH 0/3] devlink: support dry run attribute for flash update Jacob Keller
  2022-07-20 18:34 ` [iproute2-next PATCH 1/3] update <linux/devlink.h> UAPI header Jacob Keller
  2022-07-20 18:34 ` [iproute2-next PATCH 2/3] mnlg: add function to get CTRL_ATTR_MAXATTR value Jacob Keller
@ 2022-07-20 18:34 ` Jacob Keller
  2022-07-21  5:59   ` Jiri Pirko
  2022-07-21  5:59 ` [iproute2-next PATCH 0/3] devlink: support dry run attribute for flash update Jiri Pirko
  3 siblings, 1 reply; 7+ messages in thread
From: Jacob Keller @ 2022-07-20 18:34 UTC (permalink / raw)
  To: netdev; +Cc: Jakub Kicinski, Jacob Keller

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>
---
 devlink/devlink.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 43 insertions(+), 2 deletions(-)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index ddf430bbb02a..5649360b1417 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -294,6 +294,7 @@ static void ifname_map_free(struct ifname_map *ifname_map)
 #define DL_OPT_PORT_FN_RATE_TX_MAX	BIT(49)
 #define DL_OPT_PORT_FN_RATE_NODE_NAME	BIT(50)
 #define DL_OPT_PORT_FN_RATE_PARENT	BIT(51)
+#define DL_OPT_DRY_RUN			BIT(52)
 
 struct dl_opts {
 	uint64_t present; /* flags of present items */
@@ -368,6 +369,8 @@ struct dl {
 	bool verbose;
 	bool stats;
 	bool hex;
+	bool max_attr_valid;
+	uint32_t max_attr;
 	struct {
 		bool present;
 		char *bus_name;
@@ -693,6 +696,7 @@ static const enum mnl_attr_data_type devlink_policy[DEVLINK_ATTR_MAX + 1] = {
 	[DEVLINK_ATTR_TRAP_POLICER_ID] = MNL_TYPE_U32,
 	[DEVLINK_ATTR_TRAP_POLICER_RATE] = MNL_TYPE_U64,
 	[DEVLINK_ATTR_TRAP_POLICER_BURST] = MNL_TYPE_U64,
+	[DEVLINK_ATTR_DRY_RUN] = MNL_TYPE_FLAG,
 };
 
 static const enum mnl_attr_data_type
@@ -1512,6 +1516,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_dry_run(struct dl *dl)
+{
+	dl_get_max_attr(dl);
+
+	return (dl->max_attr_valid && dl->max_attr >= DEVLINK_ATTR_DRY_RUN);
+}
+
 static int dl_argv_parse(struct dl *dl, uint64_t o_required,
 			 uint64_t o_optional)
 {
@@ -2008,6 +2036,16 @@ static int dl_argv_parse(struct dl *dl, uint64_t o_required,
 			dl_arg_inc(dl);
 			opts->rate_parent_node = "";
 			o_found |= DL_OPT_PORT_FN_RATE_PARENT;
+		} else if (dl_argv_match(dl, "dry_run") &&
+			   (o_all & DL_OPT_DRY_RUN)) {
+
+			if (!dl_kernel_supports_dry_run(dl)) {
+				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;
@@ -2086,6 +2124,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);
@@ -2284,7 +2324,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,
@@ -3844,7 +3884,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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [iproute2-next PATCH 3/3] devlink: add dry run attribute support to devlink flash
  2022-07-20 18:34 ` [iproute2-next PATCH 3/3] devlink: add dry run attribute support to devlink flash Jacob Keller
@ 2022-07-21  5:59   ` Jiri Pirko
  2022-07-21 18:55     ` Keller, Jacob E
  0 siblings, 1 reply; 7+ messages in thread
From: Jiri Pirko @ 2022-07-21  5:59 UTC (permalink / raw)
  To: Jacob Keller; +Cc: netdev, Jakub Kicinski

Wed, Jul 20, 2022 at 08:34:49PM 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>
>---
> devlink/devlink.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 43 insertions(+), 2 deletions(-)
>
>diff --git a/devlink/devlink.c b/devlink/devlink.c
>index ddf430bbb02a..5649360b1417 100644
>--- a/devlink/devlink.c
>+++ b/devlink/devlink.c
>@@ -294,6 +294,7 @@ static void ifname_map_free(struct ifname_map *ifname_map)
> #define DL_OPT_PORT_FN_RATE_TX_MAX	BIT(49)
> #define DL_OPT_PORT_FN_RATE_NODE_NAME	BIT(50)
> #define DL_OPT_PORT_FN_RATE_PARENT	BIT(51)
>+#define DL_OPT_DRY_RUN			BIT(52)
> 
> struct dl_opts {
> 	uint64_t present; /* flags of present items */
>@@ -368,6 +369,8 @@ struct dl {
> 	bool verbose;
> 	bool stats;
> 	bool hex;
>+	bool max_attr_valid;
>+	uint32_t max_attr;
> 	struct {
> 		bool present;
> 		char *bus_name;
>@@ -693,6 +696,7 @@ static const enum mnl_attr_data_type devlink_policy[DEVLINK_ATTR_MAX + 1] = {
> 	[DEVLINK_ATTR_TRAP_POLICER_ID] = MNL_TYPE_U32,
> 	[DEVLINK_ATTR_TRAP_POLICER_RATE] = MNL_TYPE_U64,
> 	[DEVLINK_ATTR_TRAP_POLICER_BURST] = MNL_TYPE_U64,
>+	[DEVLINK_ATTR_DRY_RUN] = MNL_TYPE_FLAG,
> };
> 
> static const enum mnl_attr_data_type
>@@ -1512,6 +1516,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_dry_run(struct dl *dl)
>+{
>+	dl_get_max_attr(dl);
>+
>+	return (dl->max_attr_valid && dl->max_attr >= DEVLINK_ATTR_DRY_RUN);

This looks like it would be handy for other attrs too. Could you make
this a generic helper accepting attr type as function argument?



>+}
>+
> static int dl_argv_parse(struct dl *dl, uint64_t o_required,
> 			 uint64_t o_optional)
> {
>@@ -2008,6 +2036,16 @@ static int dl_argv_parse(struct dl *dl, uint64_t o_required,
> 			dl_arg_inc(dl);
> 			opts->rate_parent_node = "";
> 			o_found |= DL_OPT_PORT_FN_RATE_PARENT;
>+		} else if (dl_argv_match(dl, "dry_run") &&
>+			   (o_all & DL_OPT_DRY_RUN)) {
>+
>+			if (!dl_kernel_supports_dry_run(dl)) {
>+				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;
>@@ -2086,6 +2124,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);
>@@ -2284,7 +2324,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,
>@@ -3844,7 +3884,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
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [iproute2-next PATCH 0/3] devlink: support dry run attribute for flash update
  2022-07-20 18:34 [iproute2-next PATCH 0/3] devlink: support dry run attribute for flash update Jacob Keller
                   ` (2 preceding siblings ...)
  2022-07-20 18:34 ` [iproute2-next PATCH 3/3] devlink: add dry run attribute support to devlink flash Jacob Keller
@ 2022-07-21  5:59 ` Jiri Pirko
  3 siblings, 0 replies; 7+ messages in thread
From: Jiri Pirko @ 2022-07-21  5:59 UTC (permalink / raw)
  To: Jacob Keller; +Cc: netdev, Jakub Kicinski

You are missing iproute2 maintainers in the cc.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [iproute2-next PATCH 3/3] devlink: add dry run attribute support to devlink flash
  2022-07-21  5:59   ` Jiri Pirko
@ 2022-07-21 18:55     ` Keller, Jacob E
  0 siblings, 0 replies; 7+ messages in thread
From: Keller, Jacob E @ 2022-07-21 18:55 UTC (permalink / raw)
  To: Jiri Pirko; +Cc: netdev, Jakub Kicinski



> -----Original Message-----
> From: Jiri Pirko <jiri@resnulli.us>
> Sent: Wednesday, July 20, 2022 11:00 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: netdev@vger.kernel.org; Jakub Kicinski <kuba@kernel.org>
> Subject: Re: [iproute2-next PATCH 3/3] devlink: add dry run attribute support to
> devlink flash
> 
> Wed, Jul 20, 2022 at 08:34:49PM 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>
> >---
> > devlink/devlink.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 43 insertions(+), 2 deletions(-)
> >
> >diff --git a/devlink/devlink.c b/devlink/devlink.c
> >index ddf430bbb02a..5649360b1417 100644
> >--- a/devlink/devlink.c
> >+++ b/devlink/devlink.c
> >@@ -294,6 +294,7 @@ static void ifname_map_free(struct ifname_map
> *ifname_map)
> > #define DL_OPT_PORT_FN_RATE_TX_MAX	BIT(49)
> > #define DL_OPT_PORT_FN_RATE_NODE_NAME	BIT(50)
> > #define DL_OPT_PORT_FN_RATE_PARENT	BIT(51)
> >+#define DL_OPT_DRY_RUN			BIT(52)
> >
> > struct dl_opts {
> > 	uint64_t present; /* flags of present items */
> >@@ -368,6 +369,8 @@ struct dl {
> > 	bool verbose;
> > 	bool stats;
> > 	bool hex;
> >+	bool max_attr_valid;
> >+	uint32_t max_attr;
> > 	struct {
> > 		bool present;
> > 		char *bus_name;
> >@@ -693,6 +696,7 @@ static const enum mnl_attr_data_type
> devlink_policy[DEVLINK_ATTR_MAX + 1] = {
> > 	[DEVLINK_ATTR_TRAP_POLICER_ID] = MNL_TYPE_U32,
> > 	[DEVLINK_ATTR_TRAP_POLICER_RATE] = MNL_TYPE_U64,
> > 	[DEVLINK_ATTR_TRAP_POLICER_BURST] = MNL_TYPE_U64,
> >+	[DEVLINK_ATTR_DRY_RUN] = MNL_TYPE_FLAG,
> > };
> >
> > static const enum mnl_attr_data_type
> >@@ -1512,6 +1516,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_dry_run(struct dl *dl)
> >+{
> >+	dl_get_max_attr(dl);
> >+
> >+	return (dl->max_attr_valid && dl->max_attr >=
> DEVLINK_ATTR_DRY_RUN);
> 
> This looks like it would be handy for other attrs too. Could you make
> this a generic helper accepting attr type as function argument?
> 
> 

Yea that makes sense.

> 
> >+}
> >+
> > static int dl_argv_parse(struct dl *dl, uint64_t o_required,
> > 			 uint64_t o_optional)
> > {
> >@@ -2008,6 +2036,16 @@ static int dl_argv_parse(struct dl *dl, uint64_t
> o_required,
> > 			dl_arg_inc(dl);
> > 			opts->rate_parent_node = "";
> > 			o_found |= DL_OPT_PORT_FN_RATE_PARENT;
> >+		} else if (dl_argv_match(dl, "dry_run") &&
> >+			   (o_all & DL_OPT_DRY_RUN)) {
> >+
> >+			if (!dl_kernel_supports_dry_run(dl)) {
> >+				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;
> >@@ -2086,6 +2124,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);
> >@@ -2284,7 +2324,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,
> >@@ -3844,7 +3884,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
> >

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-07-21 18:55 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-20 18:34 [iproute2-next PATCH 0/3] devlink: support dry run attribute for flash update Jacob Keller
2022-07-20 18:34 ` [iproute2-next PATCH 1/3] update <linux/devlink.h> UAPI header Jacob Keller
2022-07-20 18:34 ` [iproute2-next PATCH 2/3] mnlg: add function to get CTRL_ATTR_MAXATTR value Jacob Keller
2022-07-20 18:34 ` [iproute2-next PATCH 3/3] devlink: add dry run attribute support to devlink flash Jacob Keller
2022-07-21  5:59   ` Jiri Pirko
2022-07-21 18:55     ` Keller, Jacob E
2022-07-21  5:59 ` [iproute2-next PATCH 0/3] devlink: support dry run attribute for flash update Jiri Pirko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).