netdev - Re: [patch net-next RFC v2 02/11] devlink: Add support for resource abstraction

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]

Message-ID: <71d6e7e9-8bb4-0877-798b-a03afc6b5348@mellanox.com>
Date:   Sun, 19 Nov 2017 10:17:17 +0200
From:   Arkadi Sharshevsky <arkadis@...lanox.com>
To:     David Ahern <dsa@...ulusnetworks.com>,
        Jiri Pirko <jiri@...nulli.us>, netdev@...r.kernel.org
Cc:     davem@...emloft.net, mlxsw@...lanox.com, andrew@...n.ch,
        vivien.didelot@...oirfairelinux.com, f.fainelli@...il.com,
        michael.chan@...adcom.com, ganeshgr@...lsio.com,
        saeedm@...lanox.com, matanb@...lanox.com, leonro@...lanox.com,
        idosch@...lanox.com, jakub.kicinski@...ronome.com, ast@...nel.org,
        daniel@...earbox.net, simon.horman@...ronome.com,
        pieter.jansenvanvuuren@...ronome.com, john.hurley@...ronome.com,
        alexander.h.duyck@...el.com, linville@...driver.com,
        gospo@...adcom.com, steven.lin1@...adcom.com, yuvalm@...lanox.com,
        ogerlitz@...lanox.com, roopa@...ulusnetworks.com
Subject: Re: [patch net-next RFC v2 02/11] devlink: Add support for resource
 abstraction



On 11/18/2017 08:34 PM, David Ahern wrote:
> On 11/14/17 9:18 AM, Jiri Pirko wrote:
>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>> index 4d2c6fc..960e80a 100644
>> --- a/include/net/devlink.h
>> +++ b/include/net/devlink.h
> ...
> 
>> @@ -469,6 +523,32 @@ devlink_dpipe_match_put(struct sk_buff *skb,
>>  	return 0;
>>  }
>>  
>> +static inline int
>> +devlink_resource_register(struct devlink *devlink,
>> +			  const char *resource_name,
>> +			  bool top_hierarchy,
>> +			  bool reload_required,
>> +			  u64 resource_size,
>> +			  u64 resource_id,
>> +			  u64 parent_resource_id,
>> +			  struct devlink_resource_ops *resource_ops)
>> +{
>> +	return 0;
>> +}
>> +
>> +static inline void
>> +devlink_resources_unregister(struct devlink *devlink,
>> +			     struct devlink_resource *resource)
>> +{
>> +}
>> +
>> +static inline int
>> +devlink_resource_size_get(struct devlink *devlink, u64 resource_id,
>> +			  u64 *p_resource_size)
>> +{
>> +	return -EINVAL;
> 
> It's compiled out so -EOPNOTSUPP seems more appropriate.
> 

will fix

> 
> 
>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>> index 0114dfc..6ae644f 100644
>> --- a/net/core/devlink.c
>> +++ b/net/core/devlink.c
>> +static int devlink_nl_cmd_resource_set(struct sk_buff *skb,
>> +				       struct genl_info *info)
>> +{
>> +	struct devlink *devlink = info->user_ptr[0];
>> +	struct devlink_resource *resource;
>> +	u64 resource_id;
>> +	u64 size;
>> +	int err;
>> +
>> +	if (!info->attrs[DEVLINK_ATTR_RESOURCE_ID] ||
>> +	    !info->attrs[DEVLINK_ATTR_RESOURCE_SIZE])
>> +		return -EINVAL;
> 
> several of the of the DEVLINK_ATTR_RESOURCE attributes are kernel to
> user only (e.g., DEVLINK_ATTR_RESOURCE_SIZE_NEW and
> DEVLINK_ATTR_RESOURCE_RELOAD_REQUIRED), so if they are given by the user
> that should be an error too right?
> 

Not sure I understood. As you see I only check for the mandatory
attributes, if the user provides not relevant data its ignored.

We use one single nla_policy for all the commands (devlink_nl_policy)

> 
>> +	resource_id = nla_get_u64(info->attrs[DEVLINK_ATTR_RESOURCE_ID]);
> 
> I don't see where these attributes are validated for proper size.
> 

right, forgot to update the policy.

>> +
>> +	resource = devlink_resource_find(devlink, NULL, resource_id);
>> +	if (!resource)
>> +		return -EINVAL;
>> +
>> +	if (!resource->resource_ops->size_validate)
>> +		return -EINVAL;
> 
> genl_info has extack; please add user messages for the above failures.
> 

Isn't EOPNOTSUPP enough ?

> 
>> +
>> +	size = nla_get_u64(info->attrs[DEVLINK_ATTR_RESOURCE_SIZE]);
>> +	err = resource->resource_ops->size_validate(devlink, size,
>> +						    &resource->resource_list,
>> +						    info->extack);
>> +	if (err)
>> +		return err;
>> +
>> +	resource->size_new = size;
>> +	return 0;
>> +}
>> +
>