netdev - Re: [PATCH net-next 05/14] devlink: use an explicit structure for dump context

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: <20230104182218.33ac9da5@kernel.org>
Date:   Wed, 4 Jan 2023 18:22:18 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
        pabeni@...hat.com, jacob.e.keller@...el.com
Subject: Re: [PATCH net-next 05/14] devlink: use an explicit structure for
 dump context

On Wed, 4 Jan 2023 11:04:01 +0100 Jiri Pirko wrote:
> Wed, Jan 04, 2023 at 05:16:27AM CET, kuba@...nel.org wrote:
> >Create a dump context structure instead of using cb->args
> >as an unsigned long array. This is a pure conversion which
> >is intended to be as much of a noop as possible.
> >Subsequent changes will use this to simplify the code.
> >
> >The two non-trivial parts are:
> > - devlink_nl_cmd_health_reporter_dump_get_dumpit() checks args[0]
> >   to see if devlink_fmsg_dumpit() has already been called (whether
> >   this is the first msg), but doesn't use the exact value, so we
> >   can drop the local variable there already
> > - devlink_nl_cmd_region_read_dumpit() uses args[0] for address
> >   but we'll use args[1] now, shouldn't matter  
> 
> I don't follow this. Where do you use args[1]? you mean
> dump->start_offset?

Yes, it used to be stored at the start of the args/cb buffer,
now it's stored after index, even tho index is not used.

> If yes, it does not matter at all and I think
> mentioning that only confuses reader (as it did for me).

I think that's fine. I want a note for myself that I knew the "binary
layouts" are changed.

> >diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
> >index e01ba7999b91..bcc930b7cfcf 100644
> >--- a/net/devlink/leftover.c
> >+++ b/net/devlink/leftover.c
> >@@ -1222,9 +1222,10 @@ static void devlink_rate_notify(struct devlink_rate *devlink_rate,
> > static int devlink_nl_cmd_rate_get_dumpit(struct sk_buff *msg,
> > 					  struct netlink_callback *cb)
> > {
> >+	struct devlink_nl_dump_state *dump = devl_dump_state(cb);  
> 
> Could this be named "state" or "dump_state"? "dump" is not what it is.

Sure...