| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 21 Jul 2022 18:55:48 +0000
From: "Keller, Jacob E" <jacob.e.keller@...el.com>
To: Jiri Pirko <jiri@...nulli.us>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
Jakub Kicinski <kuba@...nel.org>
Subject: RE: [iproute2-next PATCH 3/3] devlink: add dry run attribute support
to devlink flash
> -----Original Message-----
> From: Jiri Pirko <jiri@...nulli.us>
> Sent: Wednesday, July 20, 2022 11:00 PM
> To: Keller, Jacob E <jacob.e.keller@...el.com>
> Cc: netdev@...r.kernel.org; Jakub Kicinski <kuba@...nel.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@...el.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@...el.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
> >
Powered by blists - more mailing lists