| 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
| ||
|
Message-ID: <AM0PR0502MB3683CA10099EA2075269A979BF400@AM0PR0502MB3683.eurprd05.prod.outlook.com>
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