| 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
| ||
|
Message-ID: <CAGSyskXmLQi7urQodZVNF7n6j2OTVB4yGoXDQrHccsM0kniSkA@mail.gmail.com>
Date: Fri, 7 Nov 2025 20:53:18 +0000
From: Gustavo Luiz Duarte <gustavold@...il.com>
To: Breno Leitao <leitao@...ian.org>
Cc: Andre Carvalho <asantostc@...il.com>, Simon Horman <horms@...nel.org>,
Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Shuah Khan <shuah@...nel.org>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next 2/4] netconsole: Split userdata and sysdata
On Fri, Nov 7, 2025 at 1:23 PM Breno Leitao <leitao@...ian.org> wrote:
> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > index 0a8ba7c4bc9d..e780c884db83 100644
> > --- a/drivers/net/netconsole.c
> > +++ b/drivers/net/netconsole.c
> > @@ -50,7 +50,8 @@ MODULE_LICENSE("GPL");
> > /* The number 3 comes from userdata entry format characters (' ', '=', '\n') */
> > #define MAX_EXTRADATA_NAME_LEN (MAX_EXTRADATA_ENTRY_LEN - \
> > MAX_EXTRADATA_VALUE_LEN - 3)
> > -#define MAX_EXTRADATA_ITEMS 16
> > +#define MAX_USERDATA_ITEMS 16
>
> Do we still need to have MAX_USERDATA_ITEMS cap with your new approach?
That is a good point. I did think about this and ended up deciding to
keep a limit as a safety measure against userspace creating a boatload
of items until we run out of memory.
>
> > +#define MAX_SYSDATA_ITEMS 4
>
> Can we have this one inside enum sysdata_feature?
>
> Something as:
>
> enum sysdata_feature {
> SYSDATA_CPU_NR = BIT(0),
> SYSDATA_TASKNAME = BIT(1),
> SYSDATA_RELEASE = BIT(2),
> SYSDATA_MSGID = BIT(3),
> MAX_SYSDATA_ITEMS = 4 /* Sentinel: highest bit position */
> };
Sure, I will do this in v2.
> > @@ -1353,22 +1311,21 @@ static void populate_configfs_item(struct netconsole_target *nt,
> >
> > static int sysdata_append_cpu_nr(struct netconsole_target *nt, int offset)
> > {
> > - /* Append cpu=%d at extradata_complete after userdata str */
> > - return scnprintf(&nt->extradata_complete[offset],
> > + return scnprintf(&nt->sysdata[offset],
> > MAX_EXTRADATA_ENTRY_LEN, " cpu=%u\n",
>
> This is confusing. It is writing to sysdata but checking extradata entry len.
My understanding is that extradata is a way to refer to both userdata
and sysdata, and MAX_EXTRADATA_ENTRY_LEN is the maximum size for both
(and the arithmetic used to define MAX_EXTRADATA_VALUE_LEN and
MAX_EXTRADATA_NAME_LEN also applies to both). Do you want to define
separate maximum sizes for userdata items and sysdata items?
> > @@ -1533,11 +1475,11 @@ static void send_msg_no_fragmentation(struct netconsole_target *nt,
> > memcpy(nt->buf, msg, msg_len);
> > }
> >
> > - if (extradata)
> > - msg_len += scnprintf(&nt->buf[msg_len],
> > - MAX_PRINT_CHUNK - msg_len,
> > - "%s", extradata);
> > -
> > +#ifdef CONFIG_NETCONSOLE_DYNAMIC
> > + msg_len += scnprintf(&nt->buf[msg_len],
> > + MAX_PRINT_CHUNK - msg_len,
> > + "%s%s", nt->userdata, nt->sysdata);
> > +#endif
>
> I am not sure I like this ifdef in here. Can you if userdata or sysdata are
> valid, and then scnprintf() instead of using ifdef?
OK, will do that in v2.
> > @@ -1594,12 +1547,20 @@ static void send_fragmented_body(struct netconsole_target *nt,
> > msgbody_offset += this_chunk;
> > this_offset += this_chunk;
> >
> > - /* after msgbody, append extradata */
> > - this_chunk = min(extradata_len - extradata_offset,
> > + /* after msgbody, append userdata */
> > + this_chunk = min(userdata_len - userdata_offset,
>
> Please assign this "userdata_len - userdata_offset" to a variable and give it a
> name, so, it help us to reason about the code.
What about:
int data_remaining = userdata_len - userdata_offset;
int buffer_remaining = MAX_PRINT_CHUNK - this_header - this_offset;
this_chunk = min(data_remaining, buffer_remaining);
>
> > MAX_PRINT_CHUNK - this_header - this_offset);
> > memcpy(nt->buf + this_header + this_offset,
> > - extradata + extradata_offset, this_chunk);
> > - extradata_offset += this_chunk;
> > + userdata + userdata_offset, this_chunk);
> > + userdata_offset += this_chunk;
> > + this_offset += this_chunk;
> > +
> > + /* after userdata, append sysdata */
> > + this_chunk = min(sysdata_len - sysdata_offset,
> > + MAX_PRINT_CHUNK - this_header - this_offset);
> > + memcpy(nt->buf + this_header + this_offset,
> > + sysdata + sysdata_offset, this_chunk);
I realize we have the same NULL pointer arithmetic problem here. I
will fix it by checking if sysdata or userdata is NULL.
>
> s/this_header/this_header_offset?
I just realized that this_header is not even necessary. I can simply
add header_len to this_offset and get rid of this variable altogether.
>
> Now that you are touching this code, please review these variable to keep them named correct.
>
> Maybe adding _ptr for pointer, and _offset for offsets and _len for lenghts?
Once I get rid of this_header and add the _ptr suffix, I think it will
be much clearer.
Also, I find offset and this_offset confusing. What about replacing by
msg_offset and buffer_offset ?
>
> Thank you for your work reasong about all of this!
Thanks for the review!
Powered by blists - more mailing lists