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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sat, 21 Oct 2017 14:12:10 +0000
From:   Yuval Mintz <yuvalm@...lanox.com>
To:     Steve Lin <steven.lin1@...adcom.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jiri Pirko <jiri@...lanox.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "michael.chan@...adcom.com" <michael.chan@...adcom.com>,
        "linville@...driver.com" <linville@...driver.com>,
        "gospo@...adcom.com" <gospo@...adcom.com>
Subject: RE: [PATCH net-next v2 1/6] devlink: Add permanent config parameter
 get/set operations

> On Thu, Oct 19, 2017 at 4:21 PM, Yuval Mintz <yuvalm@...lanox.com>
> wrote:
> >> Subject: [PATCH net-next v2 1/6] devlink: Add permanent config
> parameter
> >> get/set operations
> >>
> >> Add support for permanent config parameter get/set commands. Used
> >> for parameters held in NVRAM, persistent device configuration.
> >
> > Given some of the attributes aren't Boolean, what about an API that
> > allows the user to learn of supported values per option?
> > Otherwise only way for configuring some of them would be trial & error.
> 
> Interesting suggestion.  There's a couple of places where this could
> be a factor.  (1) When a user wants to know what values are
> defined/available in the API, and (2) When the user wants to know what
> values are supported by a specific driver/device.
> 
> The intention for (1) is to push that into userspace.  The userspace
> devlink tool patches I am working on (not yet submitted) essentially
> mirror the config parameters and their options, with string "keywords"
> associated with each parameter and option, since it's the userspace
> app that will be parsing the command line strings and converting to
> API enums.  So the userspace app can provide the list of
> parameters/options it supports, which could be a subset of what's
> available in the API.
> 
> For (2), currently there is no mechanism other than trial/error as you
> suggest (up to driver to either return an error or else make use of
> the value specified by the user).  We could contemplate adding such a
> mechanism, but it's a little complicated as some options take a range
> (i.e. # of VFs per PF for example), and others may take one of a set
> of enumerated values (pre-boot link speed for example).
> 
> To clarify, are you suggesting some mechanism to allow a driver to
> report which parameters and options it supports (case (2))?  Or are
> you suggesting something in the kernel API to handle case (1) above?

I was thinking of (2). And I agree it would take some effort.

> 
> >
> >>
> >> Signed-off-by: Steve Lin <steven.lin1@...adcom.com>
> >> Acked-by: Andy Gospodarek <gospo@...adcom.com>
> >> ---
> >>  include/net/devlink.h        |   3 +
> >>  include/uapi/linux/devlink.h |  11 ++
> >>  net/core/devlink.c           | 234
> >> +++++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 248 insertions(+)
> >>
> >> diff --git a/include/net/devlink.h b/include/net/devlink.h
> >> index b9654e1..bd64623 100644
> >> --- a/include/net/devlink.h
> >> +++ b/include/net/devlink.h
> >> @@ -270,6 +270,9 @@ struct devlink_ops {
> >>       int (*eswitch_inline_mode_set)(struct devlink *devlink, u8
> >> inline_mode);
> >>       int (*eswitch_encap_mode_get)(struct devlink *devlink, u8
> >> *p_encap_mode);
> >>       int (*eswitch_encap_mode_set)(struct devlink *devlink, u8
> >> encap_mode);
> >> +     int (*perm_config_get)(struct devlink *devlink, u32 param, u32
> >> *value);
> >> +     int (*perm_config_set)(struct devlink *devlink, u32 param, u32
> >> value,
> >> +                            u8 *restart_reqd);
> >>  };
> >>
> >>  static inline void *devlink_priv(struct devlink *devlink)
> >> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
> >> index 0cbca96..47cc584 100644
> >> --- a/include/uapi/linux/devlink.h
> >> +++ b/include/uapi/linux/devlink.h
> >> @@ -70,6 +70,10 @@ enum devlink_command {
> >>       DEVLINK_CMD_DPIPE_HEADERS_GET,
> >>       DEVLINK_CMD_DPIPE_TABLE_COUNTERS_SET,
> >>
> >> +     /* Permanent (NVRAM) device config get/set */
> >> +     DEVLINK_CMD_PERM_CONFIG_GET,
> >> +     DEVLINK_CMD_PERM_CONFIG_SET,
> >> +
> >>       /* add new commands above here */
> >>       __DEVLINK_CMD_MAX,
> >>       DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
> >> @@ -202,6 +206,13 @@ enum devlink_attr {
> >>
> >>       DEVLINK_ATTR_ESWITCH_ENCAP_MODE,        /* u8 */
> >>
> >> +     /* Permanent Configuration Parameters */
> >> +     DEVLINK_ATTR_PERM_CONFIGS,                      /* nested */
> >> +     DEVLINK_ATTR_PERM_CONFIG,                       /* nested */
> >> +     DEVLINK_ATTR_PERM_CONFIG_PARAMETER,             /* u32 */
> >> +     DEVLINK_ATTR_PERM_CONFIG_VALUE,                 /*
> >> u32 */
> >> +     DEVLINK_ATTR_PERM_CONFIG_RESTART_REQUIRED,      /* u8 */
> >> +
> >>       /* 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 7d430c1..c2cc7c6 100644
> >> --- a/net/core/devlink.c
> >> +++ b/net/core/devlink.c
> >> @@ -1566,6 +1566,224 @@ static int
> >> devlink_nl_cmd_eswitch_set_doit(struct sk_buff *skb,
> >>       return 0;
> >>  }
> >>
> >> +static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX +
> 1];
> >> +
> >> +static int devlink_nl_single_param_get(struct sk_buff *msg,
> >> +                                    struct devlink *devlink,
> >> +                                    uint32_t param)
> >> +{
> >> +     u32 value;
> >> +     int err;
> >> +     const struct devlink_ops *ops = devlink->ops;
> >> +     struct nlattr *param_attr;
> >> +
> >> +     err = ops->perm_config_get(devlink, param, &value);
> >> +     if (err)
> >> +             return err;
> >> +
> >> +     param_attr = nla_nest_start(msg, DEVLINK_ATTR_PERM_CONFIG);
> >> +     nla_put_u32(msg, DEVLINK_ATTR_PERM_CONFIG_PARAMETER,
> >> param);
> >> +     nla_put_u32(msg, DEVLINK_ATTR_PERM_CONFIG_VALUE, value);
> >> +     nla_nest_end(msg, param_attr);
> >> +
> >> +     return 0;
> >> +}
> >> +
> >> +static int devlink_nl_config_get_fill(struct sk_buff *msg,
> >> +                                   struct devlink *devlink,
> >> +                                   enum devlink_command cmd,
> >> +                                   struct genl_info *info)
> >> +{
> >> +     void *hdr;
> >> +     int err;
> >> +     struct nlattr *attr;
> >> +     int param_count = 0;
> >> +     struct nlattr *cfgparam_attr;
> >> +     int rem;
> >> +     struct nlattr *tb[DEVLINK_ATTR_MAX + 1];
> >> +     u32 param;
> >> +
> >> +     hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
> >> +                       &devlink_nl_family, 0, cmd);
> >> +     if (!hdr) {
> >> +             err = -EMSGSIZE;
> >> +             goto nla_msg_failure;
> >> +     }
> >> +
> >> +     err = devlink_nl_put_handle(msg, devlink);
> >> +     if (err)
> >> +             goto nla_put_failure;
> >> +
> >> +     if (!info->attrs[DEVLINK_ATTR_PERM_CONFIGS]) {
> >> +             /* No configuration parameters */
> >> +             goto nla_put_failure;
> >> +     }
> >> +
> >> +     cfgparam_attr = nla_nest_start(msg,
> >> DEVLINK_ATTR_PERM_CONFIGS);
> >> +
> >> +     nla_for_each_nested(attr, info-
> >> >attrs[DEVLINK_ATTR_PERM_CONFIGS],
> >> +                         rem) {
> > Isn't it possible that a response for a single request for multiple ATTRs
> > wouldn't fit in a single message?
> >
> 
> Hmm... Given the small size and relatively small total number of these
> attributes, even when additional drivers add their own, I think it's

We probably have a different idea about 'small'. 
Didn’t your *initial* series attempt to add 35 attributes at once?

> unlikely that a single request would require a multipart message, even
> in the event it's requesting all of the parameters for a given device.
> I guess we could support NLM_F_MULTI type messages, but it didn't seem
> necessary to me for this application?  Thoughts?
> 
> >> +             if (err)
> >> +                     goto nla_nest_failure;
> >> +             param_count++;
> >
> > Looks like an unused parameter
> >> +
> >> +     if (restart_reqd) {
> >
> > Doesn't seem like you're ever setting it.
> > A leftover from when this was an attribute of the configs instead
> > of per-config perhaps?
> 
> Thanks for catching these two issues - you're right, they are leftover
> from the v1 implementation; apologies for not catching them in v2.
> Will fix in v3.

Powered by blists - more mailing lists