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;
>> +}
>> +
>
