Refactor send_fragmented_body() to use separate offset tracking for
msgbody, and extradata instead of complex conditional logic.
The previous implementation used boolean flags and calculated offsets
which made the code harder to follow.
The new implementation maintains independent offset counters
(msgbody_offset, extradata_offset) and processes each section
sequentially, making the data flow more straightforward and the code
easier to maintain.
This is a preparatory refactoring with no functional changes, which will
allow easily splitting extradata_complete into separate userdata and
sysdata buffers in the next patch.
Signed-off-by: Gustavo Luiz Duarte <gustavold@gmail.com>
---
drivers/net/netconsole.c | 73 ++++++++++++++++--------------------------------
1 file changed, 24 insertions(+), 49 deletions(-)
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 5d8d0214786c..0a8ba7c4bc9d 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -1553,13 +1553,16 @@ static void send_fragmented_body(struct netconsole_target *nt,
const char *msgbody, int header_len,
int msgbody_len, int extradata_len)
{
- int sent_extradata, preceding_bytes;
const char *extradata = NULL;
int body_len, offset = 0;
+ int extradata_offset = 0;
+ int msgbody_offset = 0;
#ifdef CONFIG_NETCONSOLE_DYNAMIC
extradata = nt->extradata_complete;
#endif
+ if (WARN_ON_ONCE(!extradata && extradata_len != 0))
+ return;
/* body_len represents the number of bytes that will be sent. This is
* bigger than MAX_PRINT_CHUNK, thus, it will be split in multiple
@@ -1575,7 +1578,6 @@ static void send_fragmented_body(struct netconsole_target *nt,
*/
while (offset < body_len) {
int this_header = header_len;
- bool msgbody_written = false;
int this_offset = 0;
int this_chunk = 0;
@@ -1584,55 +1586,28 @@ static void send_fragmented_body(struct netconsole_target *nt,
",ncfrag=%d/%d;", offset,
body_len);
- /* Not all msgbody data has been written yet */
- if (offset < msgbody_len) {
- this_chunk = min(msgbody_len - offset,
- MAX_PRINT_CHUNK - this_header);
- if (WARN_ON_ONCE(this_chunk <= 0))
- return;
- memcpy(nt->buf + this_header, msgbody + offset,
- this_chunk);
- this_offset += this_chunk;
- }
-
- /* msgbody was finally written, either in the previous
- * messages and/or in the current buf. Time to write
- * the extradata.
- */
- msgbody_written |= offset + this_offset >= msgbody_len;
-
- /* Msg body is fully written and there is pending extradata to
- * write, append extradata in this chunk
- */
- if (msgbody_written && offset + this_offset < body_len) {
- /* Track how much user data was already sent. First
- * time here, sent_userdata is zero
- */
- sent_extradata = (offset + this_offset) - msgbody_len;
- /* offset of bytes used in current buf */
- preceding_bytes = this_chunk + this_header;
-
- if (WARN_ON_ONCE(sent_extradata < 0))
- return;
-
- this_chunk = min(extradata_len - sent_extradata,
- MAX_PRINT_CHUNK - preceding_bytes);
- if (WARN_ON_ONCE(this_chunk < 0))
- /* this_chunk could be zero if all the previous
- * message used all the buffer. This is not a
- * problem, extradata will be sent in the next
- * iteration
- */
- return;
-
- memcpy(nt->buf + this_header + this_offset,
- extradata + sent_extradata,
- this_chunk);
- this_offset += this_chunk;
- }
+ /* write msgbody first */
+ this_chunk = min(msgbody_len - msgbody_offset,
+ MAX_PRINT_CHUNK - this_header);
+ memcpy(nt->buf + this_header, msgbody + msgbody_offset,
+ this_chunk);
+ msgbody_offset += this_chunk;
+ this_offset += this_chunk;
+
+ /* after msgbody, append extradata */
+ this_chunk = min(extradata_len - extradata_offset,
+ MAX_PRINT_CHUNK - this_header - this_offset);
+ memcpy(nt->buf + this_header + this_offset,
+ extradata + extradata_offset, this_chunk);
+ extradata_offset += this_chunk;
+ this_offset += this_chunk;
+
+ /* if all is good, send the packet out */
+ offset += this_offset;
+ if (WARN_ON_ONCE(offset > body_len))
+ return;
send_udp(nt, nt->buf, this_header + this_offset);
- offset += this_offset;
}
}
--
2.47.3On Wed, Nov 05, 2025 at 09:06:43AM -0800, Gustavo Luiz Duarte wrote:
> Refactor send_fragmented_body() to use separate offset tracking for
> msgbody, and extradata instead of complex conditional logic.
> The previous implementation used boolean flags and calculated offsets
> which made the code harder to follow.
>
> The new implementation maintains independent offset counters
> (msgbody_offset, extradata_offset) and processes each section
> sequentially, making the data flow more straightforward and the code
> easier to maintain.
>
> This is a preparatory refactoring with no functional changes, which will
> allow easily splitting extradata_complete into separate userdata and
> sysdata buffers in the next patch.
>
> Signed-off-by: Gustavo Luiz Duarte <gustavold@gmail.com>
> ---
> drivers/net/netconsole.c | 73 ++++++++++++++++--------------------------------
> 1 file changed, 24 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 5d8d0214786c..0a8ba7c4bc9d 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -1553,13 +1553,16 @@ static void send_fragmented_body(struct netconsole_target *nt,
> const char *msgbody, int header_len,
> int msgbody_len, int extradata_len)
> {
> - int sent_extradata, preceding_bytes;
> const char *extradata = NULL;
> int body_len, offset = 0;
> + int extradata_offset = 0;
> + int msgbody_offset = 0;
>
> #ifdef CONFIG_NETCONSOLE_DYNAMIC
> extradata = nt->extradata_complete;
> #endif
extradata could be NULL at this time if CONFIG_NETCONSOLE_DYNAMIC is
unset. Basically extradata=NULL will not be replaced.
> + if (WARN_ON_ONCE(!extradata && extradata_len != 0))
> + return;
And entradata_len = 0 for CONFIG_NETCONSOLE_DYNAMIC disabled.
> + /* write msgbody first */
> + this_chunk = min(msgbody_len - msgbody_offset,
> + MAX_PRINT_CHUNK - this_header);
> + memcpy(nt->buf + this_header, msgbody + msgbody_offset,
> + this_chunk);
> + msgbody_offset += this_chunk;
> + this_offset += this_chunk;
> +
> + /* after msgbody, append extradata */
> + this_chunk = min(extradata_len - extradata_offset,
> + MAX_PRINT_CHUNK - this_header - this_offset);
> + memcpy(nt->buf + this_header + this_offset,
> + extradata + extradata_offset, this_chunk);
then you are going to memcpy from NULL pointer (`extradata + extradata_offset` == 0).
I got this my vim LSP that printed:
Null pointer passed as 2nd argument to memory copy function [unix.cstring.NullArg]
On Fri, Nov 7, 2025 at 12:15 PM Breno Leitao <leitao@debian.org> wrote:
> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > index 5d8d0214786c..0a8ba7c4bc9d 100644
> > --- a/drivers/net/netconsole.c
> > +++ b/drivers/net/netconsole.c
> > @@ -1553,13 +1553,16 @@ static void send_fragmented_body(struct netconsole_target *nt,
> > const char *msgbody, int header_len,
> > int msgbody_len, int extradata_len)
> > {
> > - int sent_extradata, preceding_bytes;
> > const char *extradata = NULL;
> > int body_len, offset = 0;
> > + int extradata_offset = 0;
> > + int msgbody_offset = 0;
> >
> > #ifdef CONFIG_NETCONSOLE_DYNAMIC
> > extradata = nt->extradata_complete;
> > #endif
>
>
> extradata could be NULL at this time if CONFIG_NETCONSOLE_DYNAMIC is
> unset. Basically extradata=NULL will not be replaced.
>
> > + if (WARN_ON_ONCE(!extradata && extradata_len != 0))
> > + return;
>
> And entradata_len = 0 for CONFIG_NETCONSOLE_DYNAMIC disabled.
>
> > + /* write msgbody first */
> > + this_chunk = min(msgbody_len - msgbody_offset,
> > + MAX_PRINT_CHUNK - this_header);
> > + memcpy(nt->buf + this_header, msgbody + msgbody_offset,
> > + this_chunk);
> > + msgbody_offset += this_chunk;
> > + this_offset += this_chunk;
> > +
> > + /* after msgbody, append extradata */
> > + this_chunk = min(extradata_len - extradata_offset,
> > + MAX_PRINT_CHUNK - this_header - this_offset);
> > + memcpy(nt->buf + this_header + this_offset,
> > + extradata + extradata_offset, this_chunk);
>
> then you are going to memcpy from NULL pointer (`extradata + extradata_offset` == 0).
I believe passing NULL to memcpy() should be safe as long as count is
zero (which is the case here).
However, what I didn't realize at first is that we would be doing
pointer arithmetic with NULL, which is undefined behavior :(
I will add a check if extradata is NULL here.
Thanks for the careful review!
>
> I got this my vim LSP that printed:
>
> Null pointer passed as 2nd argument to memory copy function [unix.cstring.NullArg]
>
On Fri, Nov 7, 2025 at 12:15 PM Breno Leitao <leitao@debian.org> wrote:
>
> On Wed, Nov 05, 2025 at 09:06:43AM -0800, Gustavo Luiz Duarte wrote:
> > Refactor send_fragmented_body() to use separate offset tracking for
> > msgbody, and extradata instead of complex conditional logic.
> > The previous implementation used boolean flags and calculated offsets
> > which made the code harder to follow.
> >
> > The new implementation maintains independent offset counters
> > (msgbody_offset, extradata_offset) and processes each section
> > sequentially, making the data flow more straightforward and the code
> > easier to maintain.
> >
> > This is a preparatory refactoring with no functional changes, which will
> > allow easily splitting extradata_complete into separate userdata and
> > sysdata buffers in the next patch.
> >
> > Signed-off-by: Gustavo Luiz Duarte <gustavold@gmail.com>
> > ---
> > drivers/net/netconsole.c | 73 ++++++++++++++++--------------------------------
> > 1 file changed, 24 insertions(+), 49 deletions(-)
> >
> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > index 5d8d0214786c..0a8ba7c4bc9d 100644
> > --- a/drivers/net/netconsole.c
> > +++ b/drivers/net/netconsole.c
> > @@ -1553,13 +1553,16 @@ static void send_fragmented_body(struct netconsole_target *nt,
> > const char *msgbody, int header_len,
> > int msgbody_len, int extradata_len)
> > {
> > - int sent_extradata, preceding_bytes;
> > const char *extradata = NULL;
> > int body_len, offset = 0;
> > + int extradata_offset = 0;
> > + int msgbody_offset = 0;
> >
> > #ifdef CONFIG_NETCONSOLE_DYNAMIC
> > extradata = nt->extradata_complete;
> > #endif
>
>
> extradata could be NULL at this time if CONFIG_NETCONSOLE_DYNAMIC is
> unset. Basically extradata=NULL will not be replaced.
>
> > + if (WARN_ON_ONCE(!extradata && extradata_len != 0))
> > + return;
>
> And entradata_len = 0 for CONFIG_NETCONSOLE_DYNAMIC disabled.
>
> > + /* write msgbody first */
> > + this_chunk = min(msgbody_len - msgbody_offset,
> > + MAX_PRINT_CHUNK - this_header);
> > + memcpy(nt->buf + this_header, msgbody + msgbody_offset,
> > + this_chunk);
> > + msgbody_offset += this_chunk;
> > + this_offset += this_chunk;
> > +
> > + /* after msgbody, append extradata */
> > + this_chunk = min(extradata_len - extradata_offset,
> > + MAX_PRINT_CHUNK - this_header - this_offset);
> > + memcpy(nt->buf + this_header + this_offset,
> > + extradata + extradata_offset, this_chunk);
>
> then you are going to memcpy from NULL pointer (`extradata + extradata_offset` == 0).
>
> I got this my vim LSP that printed:
>
> Null pointer passed as 2nd argument to memory copy function [unix.cstring.NullArg]
>
© 2016 - 2025 Red Hat, Inc.