Re: [patch net-next v2 10/11] devlink: introduce dump selector attr and use it for per-instance dumps

[Jiri Pirko <jiri@resnulli.us>]

Tue, Jul 25, 2023 at 08:40:44PM CEST, kuba@kernel.org wrote:
>On Thu, 20 Jul 2023 14:18:28 +0200 Jiri Pirko wrote:
>> +static void devlink_nl_policy_cpy(struct nla_policy *policy, unsigned int attr)
>> +{
>> +	memcpy(&policy[attr], &devlink_nl_policy[attr], sizeof(*policy));
>> +}
>> +
>> +static void devlink_nl_dump_selector_policy_init(const struct devlink_cmd *cmd,
>> +						 struct nla_policy *policy)
>> +{
>> +	devlink_nl_policy_cpy(policy, DEVLINK_ATTR_BUS_NAME);
>> +	devlink_nl_policy_cpy(policy, DEVLINK_ATTR_DEV_NAME);
>> +}
>> +
>> +static int devlink_nl_start(struct netlink_callback *cb)
>> +{
>> +	struct devlink_nl_dump_state *state = devlink_dump_state(cb);
>> +	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
>> +	struct nlattr **attrs = info->attrs;
>> +	const struct devlink_cmd *cmd;
>> +	struct nla_policy *policy;
>> +	struct nlattr **selector;
>> +	int err;
>> +
>> +	if (!attrs[DEVLINK_ATTR_DUMP_SELECTOR])
>> +		return 0;
>> +
>> +	selector = kzalloc(sizeof(*selector) * (DEVLINK_ATTR_MAX + 1),
>> +			   GFP_KERNEL);
>> +	if (!selector)
>> +		return -ENOMEM;
>> +	policy = kzalloc(sizeof(*policy) * (DEVLINK_ATTR_MAX + 1), GFP_KERNEL);
>> +	if (!policy) {
>> +		kfree(selector);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	cmd = devl_cmds[info->op.cmd];
>> +	devlink_nl_dump_selector_policy_init(cmd, policy);
>> +	err = nla_parse_nested(selector, DEVLINK_ATTR_MAX,
>> +			       attrs[DEVLINK_ATTR_DUMP_SELECTOR],
>> +			       policy, cb->extack);
>> +	kfree(policy);
>> +	if (err) {
>> +		kfree(selector);
>> +		return err;
>> +	}
>> +
>> +	state->selector = selector;
>> +	return 0;
>> +}
>
>Why not declare a fully nested policy with just the two attrs?

Not sure I follow. But the nest under DEVLINK_ATTR_DUMP_SELECTOR has
its own policy, generated by devlink_nl_dump_selector_policy_init(). I
did it this way instead of separate policy array for 2 reasons:
1) We don't have duplicate and possibly conflicting policies for devlink
   root and selector
2) It is easy for specific object type to pass attrs that are included
   in the policy initialization (see the health reporter extension later
   in this patchset). There are couple of object to benefit from this,
   for example "sb".
3) It is I think a bit nicer for specific object type to pass array of
   attrs, instead of a policy array that would be exported from netlink.c

If you insist on separate policy arrays, I can do it though. I had it
like that initially, I just decided to go this way for the 3 reasons
listed above.


>
>Also - do you know of any userspace which would pass garbage attrs 
>to the dumps? Do we really need to accept all attributes, or can
>we trim the dump policies to what's actually supported?

That's what this patch is doing. It only accepts what the kernel
understands. It gives the object types (as for example health reporter)
option to extend the attr set to accept them into selectors as well, if
they know how to handle them.


>-- 
>pw-bot: cr