| 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
| ||
|
Date: Wed, 4 Jan 2023 17:50:33 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
pabeni@...hat.com, jacob.e.keller@...el.com
Subject: Re: [PATCH net-next 13/14] devlink: add by-instance dump infra
Wed, Jan 04, 2023 at 05:16:35AM 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 | 33 ++++++++++++++++++++++
> 3 files changed, 67 insertions(+), 31 deletions(-)
>
>diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
>index 5adac38454fd..e49b82dd77cd 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 {
What is "gen"? Generic netlink? Not sure why perhaps "nl" would be fine
to be consistent with the rest of the code?
Why "cmd"? That looks a bit odd to me.
>+ int (*dump_one)(struct sk_buff *msg, struct devlink *devlink,
>+ struct netlink_callback *cb);
Do you plan to have more callbacks here? If no, wouldn't it be better
to just have typedef and assign the pointer to the dump_one in devl_gen_cmds
array?
>+};
>+
> /* Iterate over devlink pointers which were possible to get reference to.
> * devlink_put() needs to be called for each iterated devlink pointer
> * in loop body in order to release the reference.
>@@ -138,6 +143,9 @@ struct devlink *devlink_get_from_attrs(struct net *net, struct nlattr **attrs);
> void devlink_notify_unregister(struct devlink *devlink);
> void devlink_notify_register(struct devlink *devlink);
>
>+int devlink_instance_iter_dump(struct sk_buff *msg,
>+ struct netlink_callback *cb);
>+
> static inline struct devlink_nl_dump_state *
> devl_dump_state(struct netlink_callback *cb)
> {
>@@ -173,6 +181,8 @@ devlink_linecard_get_from_info(struct devlink *devlink, struct genl_info *info);
> void devlink_linecard_put(struct devlink_linecard *linecard);
>
> /* Rates */
>+extern const struct devlink_gen_cmd devl_gen_rate_get;
The struct name is *_cmd, not sure why the variable name is *_get
Shouldn't it be rather devl_gen_cmd_rate?
Still sounds a bit odd.
>+
> struct devlink_rate *
> devlink_rate_get_from_info(struct devlink *devlink, struct genl_info *info);
> struct devlink_rate *
>diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
>index c6ad8133fc23..f18d8dcf9751 100644
>--- a/net/devlink/leftover.c
>+++ b/net/devlink/leftover.c
>@@ -1219,47 +1219,40 @@ static void devlink_rate_notify(struct devlink_rate *devlink_rate,
> 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL);
> }
>
>-static int devlink_nl_cmd_rate_get_dumpit(struct sk_buff *msg,
>- struct netlink_callback *cb)
>+static int
>+devlink_nl_cmd_rate_get_dumpinst(struct sk_buff *msg, struct devlink *devlink,
>+ struct netlink_callback *cb)
> {
> struct devlink_nl_dump_state *dump = devl_dump_state(cb);
>- struct devlink *devlink;
>+ struct devlink_rate *devlink_rate;
>+ int idx = 0;
> int err = 0;
>
>- devlink_dump_for_each_instance_get(msg, dump, devlink) {
>- struct devlink_rate *devlink_rate;
>- int idx = 0;
>-
>- devl_lock(devlink);
>- list_for_each_entry(devlink_rate, &devlink->rate_list, list) {
>- enum devlink_command cmd = DEVLINK_CMD_RATE_NEW;
>- u32 id = NETLINK_CB(cb->skb).portid;
>+ list_for_each_entry(devlink_rate, &devlink->rate_list, list) {
>+ enum devlink_command cmd = DEVLINK_CMD_RATE_NEW;
>+ u32 id = NETLINK_CB(cb->skb).portid;
>
>- if (idx < dump->idx) {
>- idx++;
>- continue;
>- }
>- err = devlink_nl_rate_fill(msg, devlink_rate, cmd, id,
>- cb->nlh->nlmsg_seq,
>- NLM_F_MULTI, NULL);
>- if (err) {
>- devl_unlock(devlink);
>- devlink_put(devlink);
>- dump->idx = idx;
>- goto out;
>- }
>+ if (idx < dump->idx) {
> idx++;
>+ continue;
> }
>- devl_unlock(devlink);
>- devlink_put(devlink);
>+ err = devlink_nl_rate_fill(msg, devlink_rate, cmd, id,
>+ cb->nlh->nlmsg_seq,
>+ NLM_F_MULTI, NULL);
>+ if (err) {
>+ dump->idx = idx;
>+ break;
>+ }
>+ idx++;
> }
>-out:
>- if (err != -EMSGSIZE)
>- return err;
>
>- return msg->len;
>+ return err;
> }
>
>+const struct devlink_gen_cmd devl_gen_rate_get = {
>+ .dump_one = devlink_nl_cmd_rate_get_dumpinst,
dump_one/dumpinst inconsistency in names
>+};
>+
> static int devlink_nl_cmd_rate_get_doit(struct sk_buff *skb,
> struct genl_info *info)
> {
>@@ -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_instance_iter_dump,
again, inconsistency:
devlink_instance_iter_dumpit
> .internal_flags = DEVLINK_NL_FLAG_NEED_RATE,
> /* can be retrieved by unprivileged users */
Unrelated to this patch, I wonder, why you didn't move devlink_nl_ops
along with the rest of the netlink code to netlink.c?
> },
>diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
>index ce1a7d674d14..fcf10c288480 100644
>--- a/net/devlink/netlink.c
>+++ b/net/devlink/netlink.c
>@@ -5,6 +5,7 @@
> */
>
> #include <net/genetlink.h>
>+#include <net/sock.h>
>
> #include "devl_internal.h"
>
>@@ -177,6 +178,38 @@ static void devlink_nl_post_doit(const struct genl_split_ops *ops,
> devlink_put(devlink);
> }
>
>+static const struct devlink_gen_cmd *devl_gen_cmds[] = {
>+ [DEVLINK_CMD_RATE_GET] = &devl_gen_rate_get,
static const devlink_nl_dump_one_t *devlink_nl_dump_one[] = {
[DEVLINK_CMD_RATE_GET] = &devl_nl_rate_dump_one,
}
Maybe? (not sure how the devlink/devl should be used here though)
>+};
>+
>+int devlink_instance_iter_dump(struct sk_buff *msg, struct netlink_callback *cb)
>+{
>+ const struct genl_dumpit_info *info = genl_dumpit_info(cb);
>+ struct devlink_nl_dump_state *dump = devl_dump_state(cb);
>+ const struct devlink_gen_cmd *cmd;
>+ struct devlink *devlink;
>+ int err = 0;
>+
>+ cmd = devl_gen_cmds[info->op.cmd];
>+
>+ devlink_dump_for_each_instance_get(msg, dump, devlink) {
>+ devl_lock(devlink);
>+ err = cmd->dump_one(msg, devlink, cb);
>+ devl_unlock(devlink);
>+ devlink_put(devlink);
>+
>+ if (err)
>+ break;
>+
>+ /* restart sub-object walk for the next instance */
>+ dump->idx = 0;
>+ }
>+
>+ if (err != -EMSGSIZE)
>+ return err;
>+ return msg->len;
>+}
>+
> struct genl_family devlink_nl_family __ro_after_init = {
> .name = DEVLINK_GENL_NAME,
> .version = DEVLINK_GENL_VERSION,
>--
>2.38.1
>
Powered by blists - more mailing lists