netdev - Re: [PATCH net-next v2 14/15] devlink: add by-instance dump infra

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:   Thu, 5 Jan 2023 16:16:05 -0800
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Jiri Pirko <jiri@...nulli.us>, Jakub Kicinski <kuba@...nel.org>
CC:     <davem@...emloft.net>, <netdev@...r.kernel.org>,
        <edumazet@...gle.com>, <pabeni@...hat.com>
Subject: Re: [PATCH net-next v2 14/15] devlink: add by-instance dump infra



On 1/5/2023 1:24 AM, Jiri Pirko wrote:
> Thu, Jan 05, 2023 at 05:05:30AM CET, kuba@...nel.org wrote:
>> Most dumpit implementations walk the devlink instances.
>> This requires careful lock taking and reference dropping.
>> Factor the loop out and provide just a callback to handle
>> a single instance dump.
>>
>> Convert one user as an example, other users converted
>> in the next change.
>>
>> Slightly inspired by ethtool netlink code.
>>
>> Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>
>> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
>> ---
>> net/devlink/devl_internal.h | 10 +++++++
>> net/devlink/leftover.c      | 55 ++++++++++++++++---------------------
>> net/devlink/netlink.c       | 34 +++++++++++++++++++++++
>> 3 files changed, 68 insertions(+), 31 deletions(-)
>>
>> diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
>> index 15149b0a68af..734553beccde 100644
>> --- a/net/devlink/devl_internal.h
>> +++ b/net/devlink/devl_internal.h
>> @@ -122,6 +122,11 @@ struct devlink_nl_dump_state {
>> 	};
>> };
>>
>> +struct devlink_gen_cmd {
> 
> As I wrote in reply to v1, could this be "genl"?
> 

Except Kuba already said this wasn't about "generic netlink" but
"generic devlink command" vs "complicated command that can't use the new
iterator for whatever reason", so genl feels misleading.

I guess gen is also kind of misleading but I can't think of anything better.

> 
>> +	int (*dump_one)(struct sk_buff *msg, struct devlink *devlink,
>> +			struct netlink_callback *cb);
>> +};
>> +
>> /* Iterate over registered devlink instances for devlink dump.
>>  * devlink_put() needs to be called for each iterated devlink pointer
>>  * in loop body in order to release the reference.
> 
> [...]
> 
> 
>> @@ -9130,7 +9123,7 @@ const struct genl_small_ops devlink_nl_ops[56] = {
>> 	{
>> 		.cmd = DEVLINK_CMD_RATE_GET,
>> 		.doit = devlink_nl_cmd_rate_get_doit,
>> -		.dumpit = devlink_nl_cmd_rate_get_dumpit,
>> +		.dumpit = devlink_nl_instance_iter_dump,
> 
> devlink_nl_instance_iter_dumpit to match ".dumpit".
> 
> 
>> 		.internal_flags = DEVLINK_NL_FLAG_NEED_RATE,
>> 		/* can be retrieved by unprivileged users */
>> 	},
> 
> [...]