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]

Message-ID: <Y7fi8tIYTlQp1PWo@nanopsycho>
Date:   Fri, 6 Jan 2023 09:59:30 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jacob Keller <jacob.e.keller@...el.com>
Cc:     Jakub Kicinski <kuba@...nel.org>, 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

Fri, Jan 06, 2023 at 01:16:05AM CET, jacob.e.keller@...el.com wrote:
>
>
>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

Okay, that confuses me. What is supposed to be "generic devlink
command"? I don't see anything "generic" about these.


>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 */
>>> 	},
>> 
>> [...]