[PATCH net-next v3 0/3] Use new for_each macro to create hexdumps

[PATCH net-next v3 0/3] Use new for_each macro to create hexdumps

[Nick Child]

Currently, obtaining a hexdump can be done through one of the following:
 1. hex_dump_to_buffer - takes at most 32 bytes of a buffer and returns a 
     hexdump string representation
 2. print_hex_dump - prints output of hex_dump_to_buffer iteratively over
    a large buffer

There is no functionality for iterating over a large buffer and receiving
the string representation. It seems most users of hex_dump_to_buffer are
calling hex_dump_to_buffer within the body of a loop which iterates
through a buffer.

This patchset creates a for_each macro that accepts a buffer and fills
out an output string with the converted hexdump. This loops over the
buffer and takes care of incrementing pointers. Hopefully this makes
writing sequential calls to hex_dump_to_buffer more straightforward.

From a users perspective there should be no difference in output.

The inspiration here was I wanted to use print_hex_dump in ibmvnic code
but I wanted to print through netdevice printing functions to maintain
formatting. Looking at other users of hex_dump_to_buffer it seems they had
similar intentions.

Thanks to Dave, Simon, David, and Paolo for v2 review.

Changes since v2:
 - patch1: remove unnecssary call to min, this addresses Simon's observation of
 gcc-7.5.0 warning. Other possible solutions were to change typing but it turns
 out the call to min is unnecessary since hex_dump_to_buffer has logic for
 handling len > rowlen and vice versa. So we can be honest about the len.
 - patch1: cleanup for loop i increment in response to Dave's review
 - patch3: fix ordering of [Signed-off,Reviewed]-by tags in the commit message
 - target net-next thanks to Paolo's recommendation

v2: https://lore.kernel.org/lkml/20250214162436.241359-1-nnac123@linux.ibm.com/

Changes since v1:
 - add Jacob's Reviewed-by
 - fix kernel doc typo in patch 1 noted by Simon

v1: https://lore.kernel.org/lkml/20250113221721.362093-1-nnac123@linux.ibm.com/

similar intentions.
Nick Child (3):
  hexdump: Implement macro for converting large buffers
  hexdump: Use for_each macro in print_hex_dump
  ibmvnic: Print data buffers with kernel API's

 drivers/net/ethernet/ibm/ibmvnic.c | 23 ++++++++++++++---------
 include/linux/printk.h             | 20 ++++++++++++++++++++
 lib/hexdump.c                      | 11 +++--------
 3 files changed, 37 insertions(+), 17 deletions(-)

-- 
2.48.1

[PATCH net-next v3 1/3] hexdump: Implement macro for converting large buffers

[Nick Child]

Define for_each_line_in_hex_dump which loops over a buffer and calls
hex_dump_to_buffer for each segment in the buffer. This allows the
caller to decide what to do with the resulting string and is not
limited by a specific printing format like print_hex_dump.

Signed-off-by: Nick Child <nnac123@linux.ibm.com>
---
 include/linux/printk.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index 4217a9f412b2..12e51b1cdca5 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -755,6 +755,26 @@ enum {
 extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
 			      int groupsize, char *linebuf, size_t linebuflen,
 			      bool ascii);
+/**
+ * for_each_line_in_hex_dump - iterate over buffer, converting into hex ASCII
+ * @i: offset in @buff
+ * @rowsize: number of bytes to print per line; must be 16 or 32
+ * @linebuf: where to put the converted data
+ * @linebuflen: total size of @linebuf, including space for terminating NUL
+ *		IOW >= (@rowsize * 2) + ((@rowsize - 1 / @groupsize)) + 1
+ * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
+ * @buf: data blob to dump
+ * @len: number of bytes in the @buf
+ */
+ #define for_each_line_in_hex_dump(i, rowsize, linebuf, linebuflen, groupsize, \
+				   buf, len) \
+	for ((i) = 0;							\
+	     (i) < (len) &&						\
+	     hex_dump_to_buffer((unsigned char *)(buf) + (i),		\
+				(len) - (i), (rowsize), (groupsize),	\
+				(linebuf), (linebuflen), false);	\
+	     (i) += (rowsize) == 32 ? 32 : 16				\
+	    )
 #ifdef CONFIG_PRINTK
 extern void print_hex_dump(const char *level, const char *prefix_str,
 			   int prefix_type, int rowsize, int groupsize,
-- 
2.48.1

Re: [PATCH net-next v3 1/3] hexdump: Implement macro for converting large buffers

[David Laight]

On Wed, 19 Feb 2025 15:11:00 -0600
Nick Child <nnac123@linux.ibm.com> wrote:

> Define for_each_line_in_hex_dump which loops over a buffer and calls
> hex_dump_to_buffer for each segment in the buffer. This allows the
> caller to decide what to do with the resulting string and is not
> limited by a specific printing format like print_hex_dump.
> 
> Signed-off-by: Nick Child <nnac123@linux.ibm.com>
> ---
>  include/linux/printk.h | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 4217a9f412b2..12e51b1cdca5 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -755,6 +755,26 @@ enum {
>  extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
>  			      int groupsize, char *linebuf, size_t linebuflen,
>  			      bool ascii);
> +/**
> + * for_each_line_in_hex_dump - iterate over buffer, converting into hex ASCII
> + * @i: offset in @buff
> + * @rowsize: number of bytes to print per line; must be 16 or 32
> + * @linebuf: where to put the converted data
> + * @linebuflen: total size of @linebuf, including space for terminating NUL
> + *		IOW >= (@rowsize * 2) + ((@rowsize - 1 / @groupsize)) + 1
> + * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> + * @buf: data blob to dump
> + * @len: number of bytes in the @buf
> + */
> + #define for_each_line_in_hex_dump(i, rowsize, linebuf, linebuflen, groupsize, \
> +				   buf, len) \
> +	for ((i) = 0;							\
> +	     (i) < (len) &&						\
> +	     hex_dump_to_buffer((unsigned char *)(buf) + (i),		\
> +				(len) - (i), (rowsize), (groupsize),	\
> +				(linebuf), (linebuflen), false);	\

You can avoid the compiler actually checking the function result
it you try a bit harder - see below.

> +	     (i) += (rowsize) == 32 ? 32 : 16				\
> +	    )

If you are doing this as a #define you really shouldn't evaluate the
arguments more than once.
I'd also not add more code that relies on the perverse and pointless
code that enforces rowsize of 16 or 32.
Maybe document it, but there is no point changing the stride without
doing the equivalent change to the rowsize passed to hex_dump_to_buffer.

You could do:
#define for_each_line_in_hex_dump(buf_offset, rowsize, linebuf, linebuflen, groupsize, buf, len, ascii) \
for (unsigned int _offset = 0, _rowsize = (rowsize), _len = (len); \
	((offset) = _offset) < _len && (hex_dump_to_buffer((const char *)(buf) + _offset, _len - _offset, \
		_rowsize, (groupsize), (linebuf), (linebuflen), (ascii)), 1); \
	_offset += _rowsize )

(Assuming I've not mistyped it.)

As soon as 'ascii' gets replaced by 'flags' you'll need to pass it through.

	David

Re: [PATCH net-next v3 1/3] hexdump: Implement macro for converting large buffers

[Nick Child]

Hi David,

On Thu, Feb 20, 2025 at 10:00:50PM +0000, David Laight wrote:
> On Wed, 19 Feb 2025 15:11:00 -0600
> Nick Child <nnac123@linux.ibm.com> wrote:
> 
> > ---
> >  include/linux/printk.h | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/include/linux/printk.h b/include/linux/printk.h
> > index 4217a9f412b2..12e51b1cdca5 100644
> > --- a/include/linux/printk.h
> > +++ b/include/linux/printk.h
> > +				   buf, len) \
> > +	for ((i) = 0;							\
> > +	     (i) < (len) &&						\
> > +	     hex_dump_to_buffer((unsigned char *)(buf) + (i),		\
> > +				(len) - (i), (rowsize), (groupsize),	\
> > +				(linebuf), (linebuflen), false);	\
> 
> You can avoid the compiler actually checking the function result
> it you try a bit harder - see below.
> 

This was an extra precaution against infinite loops, breaking when
hex_dump_to_buffer returns 0 when len is 0. Technically this won't happen
since we check i < len first, and increment i by at least 16 (though
your proposal removes the latter assertion). 

My other thought was to check for error case by checking if
the return value was > linebuflen. But I actually prefer the behavior
of continuing with the truncated result.

I think I prefer it how it is rather than completely ignoring it.
Open to other opinons though.

> > +	     (i) += (rowsize) == 32 ? 32 : 16				\
> > +	    )
> 
> If you are doing this as a #define you really shouldn't evaluate the
> arguments more than once.
> I'd also not add more code that relies on the perverse and pointless
> code that enforces rowsize of 16 or 32.
> Maybe document it, but there is no point changing the stride without
> doing the equivalent change to the rowsize passed to hex_dump_to_buffer.
> 

The equivalent conditonal exists in hex_dump_to_buffer so doing it
again seemed unnecessary. I understand your recent patch [1] is trying
to replace the rowsize is 16 or 32 rule with rowsize is a power of 2
and multiple of groupsize. I suppose the most straightforward and
flexible thing the for_each loop can do is to just assume rowsize is
valid.

> You could do:
> #define for_each_line_in_hex_dump(buf_offset, rowsize, linebuf, linebuflen, groupsize, buf, len, ascii) \
> for (unsigned int _offset = 0, _rowsize = (rowsize), _len = (len); \
> 	((offset) = _offset) < _len && (hex_dump_to_buffer((const char *)(buf) + _offset, _len - _offset, \
> 		_rowsize, (groupsize), (linebuf), (linebuflen), (ascii)), 1); \
> 	_offset += _rowsize )
> 
> (Assuming I've not mistyped it.)
> 

Trying to understand the reasoning for declaring new tmp variables;
Is this to prevent the values from changing in the body of the loop?
I tried to avoid declaring new vars in this design because I thought it
would recive pushback due to possible name collision and variable
declaration inside for loop initializer.
I suppose both implementations come with tradeoffs.

> As soon as 'ascii' gets replaced by 'flags' you'll need to pass it through.
> 

Yes, if hex_dump_to_buffer becomes a wrapper around a new function
(which accepts flag arg), I think there is an opportunity for a lot
of confusion to clear up. Old behaviour of hex_dump_to_buffer will be
respected but the underlying function will be more flexible.

Appreciate the review!
- Nick

[1] - https://lore.kernel.org/lkml/20250216201901.161781-1-david.laight.linux@gmail.com/

Re: [PATCH net-next v3 1/3] hexdump: Implement macro for converting large buffers

[David Laight]

On Fri, 21 Feb 2025 11:37:46 -0600
Nick Child <nnac123@linux.ibm.com> wrote:

> Hi David,
> 
> On Thu, Feb 20, 2025 at 10:00:50PM +0000, David Laight wrote:
> > On Wed, 19 Feb 2025 15:11:00 -0600
> > Nick Child <nnac123@linux.ibm.com> wrote:
> >   
> > > ---
> > >  include/linux/printk.h | 20 ++++++++++++++++++++
> > >  1 file changed, 20 insertions(+)
> > > 
> > > diff --git a/include/linux/printk.h b/include/linux/printk.h
> > > index 4217a9f412b2..12e51b1cdca5 100644
> > > --- a/include/linux/printk.h
> > > +++ b/include/linux/printk.h
> > > +				   buf, len) \
> > > +	for ((i) = 0;							\
> > > +	     (i) < (len) &&						\
> > > +	     hex_dump_to_buffer((unsigned char *)(buf) + (i),		\
> > > +				(len) - (i), (rowsize), (groupsize),	\
> > > +				(linebuf), (linebuflen), false);	\  
> > 
> > You can avoid the compiler actually checking the function result
> > it you try a bit harder - see below.
> >   
> 
> This was an extra precaution against infinite loops, breaking when
> hex_dump_to_buffer returns 0 when len is 0. Technically this won't happen
> since we check i < len first, and increment i by at least 16 (though
> your proposal removes the latter assertion). 
> 
> My other thought was to check for error case by checking if
> the return value was > linebuflen. But I actually prefer the behavior
> of continuing with the truncated result.
> 
> I think I prefer it how it is rather than completely ignoring it.
> Open to other opinons though.

There are plenty of ways to generate infinite loops.
I wouldn't worry about someone passing 0 for rowsize.

> 
> > > +	     (i) += (rowsize) == 32 ? 32 : 16				\
> > > +	    )  
> > 
> > If you are doing this as a #define you really shouldn't evaluate the
> > arguments more than once.
> > I'd also not add more code that relies on the perverse and pointless
> > code that enforces rowsize of 16 or 32.
> > Maybe document it, but there is no point changing the stride without
> > doing the equivalent change to the rowsize passed to hex_dump_to_buffer.
> >   
> 
> The equivalent conditonal exists in hex_dump_to_buffer so doing it
> again seemed unnecessary. I understand your recent patch [1] is trying
> to replace the rowsize is 16 or 32 rule with rowsize is a power of 2
> and multiple of groupsize. I suppose the most straightforward and
> flexible thing the for_each loop can do is to just assume rowsize is
> valid.
> 
> > You could do:
> > #define for_each_line_in_hex_dump(buf_offset, rowsize, linebuf, linebuflen, groupsize, buf, len, ascii) \
> > for (unsigned int _offset = 0, _rowsize = (rowsize), _len = (len); \
> > 	((offset) = _offset) < _len && (hex_dump_to_buffer((const char *)(buf) + _offset, _len - _offset, \
          ^ needs to be buf_offset.

> > 		_rowsize, (groupsize), (linebuf), (linebuflen), (ascii)), 1); \
> > 	_offset += _rowsize )
> > 
> > (Assuming I've not mistyped it.)
> >   
> 
> Trying to understand the reasoning for declaring new tmp variables;
> Is this to prevent the values from changing in the body of the loop?

No, it is to prevent side-effects happening more than once.
Think about what would happen if someone passed 'foo -= 4' for len.

> I tried to avoid declaring new vars in this design because I thought it
> would recive pushback due to possible name collision and variable
> declaration inside for loop initializer.

The c std level got upped recently to allow declarations inside loops.
Usually for a 'loop iterator' - but I think you needed that to be
exposed outsize the loop.
(Otherwise you don't need _offset and buf_offset.

> I suppose both implementations come with tradeoffs.
> 
> > As soon as 'ascii' gets replaced by 'flags' you'll need to pass it through.
> >   
> 
> Yes, if hex_dump_to_buffer becomes a wrapper around a new function
> (which accepts flag arg), I think there is an opportunity for a lot
> of confusion to clear up. Old behaviour of hex_dump_to_buffer will be
> respected but the underlying function will be more flexible.

My changed version (honoring row_size) passes all the self tests.
I've not done a grep (yet) to if anything other that 16 or 32 is
actually passed.

	David

> 
> Appreciate the review!
> - Nick
> 
> [1] - https://lore.kernel.org/lkml/20250216201901.161781-1-david.laight.linux@gmail.com/

Re: [PATCH net-next v3 1/3] hexdump: Implement macro for converting large buffers

[Nick Child]

On Fri, Feb 21, 2025 at 06:04:35PM +0000, David Laight wrote:
> On Fri, 21 Feb 2025 11:37:46 -0600
> Nick Child <nnac123@linux.ibm.com> wrote:
> > On Thu, Feb 20, 2025 at 10:00:50PM +0000, David Laight wrote:
> > > You could do:
> > > #define for_each_line_in_hex_dump(buf_offset, rowsize, linebuf, linebuflen, groupsize, buf, len, ascii) \
> > > for (unsigned int _offset = 0, _rowsize = (rowsize), _len = (len); \
> > > 	((offset) = _offset) < _len && (hex_dump_to_buffer((const char *)(buf) + _offset, _len - _offset, \
>           ^ needs to be buf_offset.
> 
> > > 		_rowsize, (groupsize), (linebuf), (linebuflen), (ascii)), 1); \
> > > 	_offset += _rowsize )
> > > 
> > > (Assuming I've not mistyped it.)
> > >   
> >
> > Trying to understand the reasoning for declaring new tmp variables;
> > Is this to prevent the values from changing in the body of the loop?
>
> No, it is to prevent side-effects happening more than once.
> Think about what would happen if someone passed 'foo -= 4' for len.
>

If we are protecting against those cases then linebuf, linebuflen,
groupsize and ascii should also be stored into tmp variables since they
are referenced in the loop conditional every iteration.
At which point the loop becomes too messy IMO.
Are any other for_each implementations taking these precautions?

Not trying to come off dismissive, I genuinely appreciate all the
insight, trying to learn more for next time.

> > I tried to avoid declaring new vars in this design because I thought it
> > would recive pushback due to possible name collision and variable
> > declaration inside for loop initializer.
>
> The c std level got upped recently to allow declarations inside loops.
> Usually for a 'loop iterator' - but I think you needed that to be
> exposed outsize the loop.
> (Otherwise you don't need _offset and buf_offset.
>

As in decrementing _len and increasing a _buf var rather than tracking
offset?
I don't really care for exposing the offset, during design I figured
some caller may make use of it but I think it is worth removing to reduce
the number of arguments.

Thanks again,
Nick

Re: [PATCH net-next v3 1/3] hexdump: Implement macro for converting large buffers

[David Laight]

On Fri, 21 Feb 2025 12:50:59 -0600
Nick Child <nnac123@linux.ibm.com> wrote:

> On Fri, Feb 21, 2025 at 06:04:35PM +0000, David Laight wrote:
> > On Fri, 21 Feb 2025 11:37:46 -0600
> > Nick Child <nnac123@linux.ibm.com> wrote:  
> > > On Thu, Feb 20, 2025 at 10:00:50PM +0000, David Laight wrote:  
> > > > You could do:
> > > > #define for_each_line_in_hex_dump(buf_offset, rowsize, linebuf, linebuflen, groupsize, buf, len, ascii) \
> > > > for (unsigned int _offset = 0, _rowsize = (rowsize), _len = (len); \
> > > > 	((offset) = _offset) < _len && (hex_dump_to_buffer((const char *)(buf) + _offset, _len - _offset, \  
> >           ^ needs to be buf_offset.
> >   
> > > > 		_rowsize, (groupsize), (linebuf), (linebuflen), (ascii)), 1); \
> > > > 	_offset += _rowsize )
> > > > 
> > > > (Assuming I've not mistyped it.)
> > > >     
> > >
> > > Trying to understand the reasoning for declaring new tmp variables;
> > > Is this to prevent the values from changing in the body of the loop?  
> >
> > No, it is to prevent side-effects happening more than once.
> > Think about what would happen if someone passed 'foo -= 4' for len.
> >  
> 
> If we are protecting against those cases then linebuf, linebuflen,
> groupsize and ascii should also be stored into tmp variables since they
> are referenced in the loop conditional every iteration.
> At which point the loop becomes too messy IMO.
> Are any other for_each implementations taking these precautions?

No, it only matters if they appear in the text expansion of the #define
more than once.
It may be unlikely here, but for things like min(a,b) where:
#define min(a, b) ((a) < (b) ? (a) : (b))
causes:
	a = 3;
	b = min(a += 3, 7);
to set b to 9 it has to be avoided.

> 
> Not trying to come off dismissive, I genuinely appreciate all the
> insight, trying to learn more for next time.
> 
> > > I tried to avoid declaring new vars in this design because I thought it
> > > would recive pushback due to possible name collision and variable
> > > declaration inside for loop initializer.  
> >
> > The c std level got upped recently to allow declarations inside loops.
> > Usually for a 'loop iterator' - but I think you needed that to be
> > exposed outsize the loop.
> > (Otherwise you don't need _offset and buf_offset.
> >  
> 
> As in decrementing _len and increasing a _buf var rather than tracking
> offset?
> I don't really care for exposing the offset, during design I figured
> some caller may make use of it but I think it is worth removing to reduce
> the number of arguments.

Except the loop body needs it - so it needs to be a caller-defined name,
even if they don't declare the variable.

	David

> 
> Thanks again,
> Nick

Re: [PATCH net-next v3 1/3] hexdump: Implement macro for converting large buffers

[Nick Child]

On Fri, Feb 21, 2025 at 10:18:15PM +0000, David Laight wrote:
> On Fri, 21 Feb 2025 12:50:59 -0600
> Nick Child <nnac123@linux.ibm.com> wrote:
> 
> > On Fri, Feb 21, 2025 at 06:04:35PM +0000, David Laight wrote:
> > > On Fri, 21 Feb 2025 11:37:46 -0600
> > > Nick Child <nnac123@linux.ibm.com> wrote:  
> > > > On Thu, Feb 20, 2025 at 10:00:50PM +0000, David Laight wrote:  
> > > > > You could do:
> > > > > #define for_each_line_in_hex_dump(buf_offset, rowsize, linebuf, linebuflen, groupsize, buf, len, ascii) \
> > > > > for (unsigned int _offset = 0, _rowsize = (rowsize), _len = (len); \
> > > > > 	((offset) = _offset) < _len && (hex_dump_to_buffer((const char *)(buf) + _offset, _len - _offset, \  
> > >           ^ needs to be buf_offset.
> > >   
> > > > > 		_rowsize, (groupsize), (linebuf), (linebuflen), (ascii)), 1); \
> > > > > 	_offset += _rowsize )
> > > > > 
> > > > > (Assuming I've not mistyped it.)
> > > > >     
> > > >
> > > > Trying to understand the reasoning for declaring new tmp variables;
> > > > Is this to prevent the values from changing in the body of the loop?  
> > >
> > > No, it is to prevent side-effects happening more than once.
> > > Think about what would happen if someone passed 'foo -= 4' for len.
> > >  
> > 
> > If we are protecting against those cases then linebuf, linebuflen,
> > groupsize and ascii should also be stored into tmp variables since they
> > are referenced in the loop conditional every iteration.
> > At which point the loop becomes too messy IMO.
> > Are any other for_each implementations taking these precautions?
> 
> No, it only matters if they appear in the text expansion of the #define
> more than once.

But the operation is still executed more than once when the variable
appears in the loop conditional. This still sounds like the same type
of unexpected behaviour. For example, when I set groupsize = 1 then
invoke for_each_line_in_hex_dump with groupsize *= 2 I get:
[    4.688870][  T145] HD: 0100 0302 0504 0706 0908 0b0a 0d0c 0f0e
[    4.688949][  T145] HD: 13121110 17161514 1b1a1918 1f1e1d1c
[    4.688969][  T145] HD: 2726252423222120 2f2e2d2c2b2a2928
[    4.688983][  T145] HD: 30 31 32 33 34 35 36 37 38 39 3a 3b 3c 3d 3e 3f
Similarly if I run with buf: buf += 8:
[    5.019031][  T149] HD: 08 09 0a 0b 0c 0d 0e 0f 10 11 12 13 14 15 16 17
[    5.019057][  T149] HD: 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f
[    5.019069][  T149] HD: 38 39 3a 3b 3c 3d 3e 3f 98 1a 6a 95 de e6 9a 71
[    5.019081][  T149] HD: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

The operations are getting executed more than once. Should this be
classified as expected behaviour just because those vars are technically
only expanded once in the macro?

> > Not trying to come off dismissive, I genuinely appreciate all the
> > insight, trying to learn more for next time.
> > 
> > > > I tried to avoid declaring new vars in this design because I thought it
> > > > would recive pushback due to possible name collision and variable
> > > > declaration inside for loop initializer.  
> > >
> > > The c std level got upped recently to allow declarations inside loops.
> > > Usually for a 'loop iterator' - but I think you needed that to be
> > > exposed outsize the loop.
> > > (Otherwise you don't need _offset and buf_offset.
> > >  
> > 
> > As in decrementing _len and increasing a _buf var rather than tracking
> > offset?
> > I don't really care for exposing the offset, during design I figured
> > some caller may make use of it but I think it is worth removing to reduce
> > the number of arguments.
> 
> Except the loop body needs it - so it needs to be a caller-defined name,
> even if they don't declare the variable.
> 
> 	David
> 
> > 
> > Thanks again,
> > Nick
> 

Re: [PATCH net-next v3 1/3] hexdump: Implement macro for converting large buffers

[David Laight]

On Sat, 22 Feb 2025 12:58:48 -0600
Nick Child <nnac123@linux.ibm.com> wrote:

> On Fri, Feb 21, 2025 at 10:18:15PM +0000, David Laight wrote:
> > On Fri, 21 Feb 2025 12:50:59 -0600
> > Nick Child <nnac123@linux.ibm.com> wrote:
> >   
> > > On Fri, Feb 21, 2025 at 06:04:35PM +0000, David Laight wrote:  
> > > > On Fri, 21 Feb 2025 11:37:46 -0600
> > > > Nick Child <nnac123@linux.ibm.com> wrote:    
> > > > > On Thu, Feb 20, 2025 at 10:00:50PM +0000, David Laight wrote:    
> > > > > > You could do:
> > > > > > #define for_each_line_in_hex_dump(buf_offset, rowsize, linebuf, linebuflen, groupsize, buf, len, ascii) \
> > > > > > for (unsigned int _offset = 0, _rowsize = (rowsize), _len = (len); \
> > > > > > 	((offset) = _offset) < _len && (hex_dump_to_buffer((const char *)(buf) + _offset, _len - _offset, \    
> > > >           ^ needs to be buf_offset.
> > > >     
> > > > > > 		_rowsize, (groupsize), (linebuf), (linebuflen), (ascii)), 1); \
> > > > > > 	_offset += _rowsize )
> > > > > > 
> > > > > > (Assuming I've not mistyped it.)
> > > > > >       
> > > > >
> > > > > Trying to understand the reasoning for declaring new tmp variables;
> > > > > Is this to prevent the values from changing in the body of the loop?    
> > > >
> > > > No, it is to prevent side-effects happening more than once.
> > > > Think about what would happen if someone passed 'foo -= 4' for len.
> > > >    
> > > 
> > > If we are protecting against those cases then linebuf, linebuflen,
> > > groupsize and ascii should also be stored into tmp variables since they
> > > are referenced in the loop conditional every iteration.
> > > At which point the loop becomes too messy IMO.
> > > Are any other for_each implementations taking these precautions?  
> > 
> > No, it only matters if they appear in the text expansion of the #define
> > more than once.  
> 
> But the operation is still executed more than once when the variable
> appears in the loop conditional. This still sounds like the same type
> of unexpected behaviour. For example, when I set groupsize = 1 then
> invoke for_each_line_in_hex_dump with groupsize *= 2 I get:
> [    4.688870][  T145] HD: 0100 0302 0504 0706 0908 0b0a 0d0c 0f0e
> [    4.688949][  T145] HD: 13121110 17161514 1b1a1918 1f1e1d1c
> [    4.688969][  T145] HD: 2726252423222120 2f2e2d2c2b2a2928
> [    4.688983][  T145] HD: 30 31 32 33 34 35 36 37 38 39 3a 3b 3c 3d 3e 3f
> Similarly if I run with buf: buf += 8:
> [    5.019031][  T149] HD: 08 09 0a 0b 0c 0d 0e 0f 10 11 12 13 14 15 16 17
> [    5.019057][  T149] HD: 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f
> [    5.019069][  T149] HD: 38 39 3a 3b 3c 3d 3e 3f 98 1a 6a 95 de e6 9a 71
> [    5.019081][  T149] HD: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 
> The operations are getting executed more than once. Should this be
> classified as expected behaviour just because those vars are technically
> only expanded once in the macro?

Brain fade on my side :-(

While you can copy all the integers, all the variables defined in a for(;;)
statement have to have the same type - so you can't take a copy of the
pointers.

So maybe this is actually unwritable without having odd side effects and
probably doesn't really save much text in any place it is used.

I did a scan of the kernel sources earlier.
Everything sets rowsize to 16 or 32, so it doesn't matter if hexdump_to_buffer()
just uses the supplied value.
I didn't look at the history.

	David

[PATCH net-next v3 2/3] hexdump: Use for_each macro in print_hex_dump

[Nick Child]

The looping logic in print_hex_dump can be handled by the macro
for_each_line_in_hex_dump.

Signed-off-by: Nick Child <nnac123@linux.ibm.com>
---
 lib/hexdump.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/lib/hexdump.c b/lib/hexdump.c
index c3db7c3a7643..181b82dfe40d 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -263,19 +263,14 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
 		    const void *buf, size_t len, bool ascii)
 {
 	const u8 *ptr = buf;
-	int i, linelen, remaining = len;
+	int i;
 	unsigned char linebuf[32 * 3 + 2 + 32 + 1];
 
 	if (rowsize != 16 && rowsize != 32)
 		rowsize = 16;
 
-	for (i = 0; i < len; i += rowsize) {
-		linelen = min(remaining, rowsize);
-		remaining -= rowsize;
-
-		hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
-				   linebuf, sizeof(linebuf), ascii);
-
+	for_each_line_in_hex_dump(i, rowsize, linebuf, sizeof(linebuf),
+				  groupsize, buf, len) {
 		switch (prefix_type) {
 		case DUMP_PREFIX_ADDRESS:
 			printk("%s%s%p: %s\n",
-- 
2.48.1

Re: [PATCH net-next v3 2/3] hexdump: Use for_each macro in print_hex_dump

[Nick Child]

Hi Dave,

Thanks for reviewing.

On 2/19/25 3:54 PM, Dave Marquardt wrote:
> Nick Child <nnac123@linux.ibm.com> writes:
>
>> diff --git a/lib/hexdump.c b/lib/hexdump.c
>> index c3db7c3a7643..181b82dfe40d 100644
>> --- a/lib/hexdump.c
>> +++ b/lib/hexdump.c
>> @@ -263,19 +263,14 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
>>   		    const void *buf, size_t len, bool ascii)
>>   {
>> -	for (i = 0; i < len; i += rowsize) {
>> -		linelen = min(remaining, rowsize);
>> -		remaining -= rowsize;
>> -
>> -		hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
>> -				   linebuf, sizeof(linebuf), ascii);
>> -
>> +	for_each_line_in_hex_dump(i, rowsize, linebuf, sizeof(linebuf),
>> +				  groupsize, buf, len) {
> Several callers of print_hex_dump pass true for the ascii parameter,
> which gets passed along to hex_dump_to_buffer. But you ignore it in
> for_each_line_in_hex_dump and always use false:
>
> + #define for_each_line_in_hex_dump(i, rowsize, linebuf, linebuflen, groupsize, \
> +				   buf, len) \
> +	for ((i) = 0;							\
> +	     (i) < (len) &&						\
> +	     hex_dump_to_buffer((unsigned char *)(buf) + (i),		\
> +				(len) - (i), (rowsize), (groupsize),	\
> +				(linebuf), (linebuflen), false);	\
> +	     (i) += (rowsize) == 32 ? 32 : 16				\
> +	    )
>
> Is this behavior change intended?
>
> -Dave

Yes, for simplicity, I wanted to limit the number of parameters that the 
macro had.

Since the function does not do any printing, the user can do ascii 
conversion on their own

or even easier just ensure a \NULL term and print the string with the %s 
format specifier.


Also, allowing the user to specify the ascii argument makes it more 
difficult for them to

calculate the correct linebuflen.

For example, rowlen == 16, and groupsize == 1 with ascii = true would 
require a linebuflen

of 16 * 4 + 1 (2 chars, 1 space/NULL and 1 ascii per byte  + a extra 
space separating hexdump and ascii).

If ascii == false, linebuflen is very logically 16*3.


- Nick

Re: [PATCH net-next v3 2/3] hexdump: Use for_each macro in print_hex_dump

[David Laight]

On Thu, 20 Feb 2025 09:49:04 -0600
Nick Child <nnac123@linux.ibm.com> wrote:

> Hi Dave,
> 
> Thanks for reviewing.
> 
> On 2/19/25 3:54 PM, Dave Marquardt wrote:
> > Nick Child <nnac123@linux.ibm.com> writes:
> >  
> >> diff --git a/lib/hexdump.c b/lib/hexdump.c
> >> index c3db7c3a7643..181b82dfe40d 100644
> >> --- a/lib/hexdump.c
> >> +++ b/lib/hexdump.c
> >> @@ -263,19 +263,14 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
> >>   		    const void *buf, size_t len, bool ascii)
> >>   {
> >> -	for (i = 0; i < len; i += rowsize) {
> >> -		linelen = min(remaining, rowsize);
> >> -		remaining -= rowsize;
> >> -
> >> -		hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
> >> -				   linebuf, sizeof(linebuf), ascii);
> >> -
> >> +	for_each_line_in_hex_dump(i, rowsize, linebuf, sizeof(linebuf),
> >> +				  groupsize, buf, len) {  
> > Several callers of print_hex_dump pass true for the ascii parameter,
> > which gets passed along to hex_dump_to_buffer. But you ignore it in
> > for_each_line_in_hex_dump and always use false:
> >
> > + #define for_each_line_in_hex_dump(i, rowsize, linebuf, linebuflen, groupsize, \
> > +				   buf, len) \
> > +	for ((i) = 0;							\
> > +	     (i) < (len) &&						\
> > +	     hex_dump_to_buffer((unsigned char *)(buf) + (i),		\
> > +				(len) - (i), (rowsize), (groupsize),	\
> > +				(linebuf), (linebuflen), false);	\
> > +	     (i) += (rowsize) == 32 ? 32 : 16				\
> > +	    )
> >
> > Is this behavior change intended?
> >
> > -Dave  
> 
> Yes, for simplicity, I wanted to limit the number of parameters that the 
> macro had.
> 
> Since the function does not do any printing, the user can do ascii 
> conversion on their own
> 
> or even easier just ensure a \NULL term and print the string with the %s 
> format specifier.

That just isn't the same.
The hexdump code 'sanitises' the string for you.

> 
> 
> Also, allowing the user to specify the ascii argument makes it more 
> difficult for them to calculate the correct linebuflen.
> 
> For example, rowlen == 16, and groupsize == 1 with ascii = true would 
> require a linebuflen
> 
> of 16 * 4 + 1 (2 chars, 1 space/NULL and 1 ascii per byte  + a extra 
> space separating hexdump and ascii).
> 
> If ascii == false, linebuflen is very logically 16*3.

But the buffer only need to be 'big enough', a few spare bytes don't
matter.

You can't change the behaviour like that.

	David

> 
> 
> - Nick
> 
> 

Re: [PATCH net-next v3 2/3] hexdump: Use for_each macro in print_hex_dump

[Nick Child]

On Thu, Feb 20, 2025 at 09:41:18PM +0000, David Laight wrote:
> On Thu, 20 Feb 2025 09:49:04 -0600
> Nick Child <nnac123@linux.ibm.com> wrote:
> > On 2/19/25 3:54 PM, Dave Marquardt wrote:
> > > Nick Child <nnac123@linux.ibm.com> writes:
> > >  
> > >> diff --git a/lib/hexdump.c b/lib/hexdump.c
> > >> index c3db7c3a7643..181b82dfe40d 100644
> > >> --- a/lib/hexdump.c
> > >> +++ b/lib/hexdump.c
> > >> @@ -263,19 +263,14 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
> > >>   		    const void *buf, size_t len, bool ascii)
> > >>   {
> > >> -	for (i = 0; i < len; i += rowsize) {
> > >> -		linelen = min(remaining, rowsize);
> > >> -		remaining -= rowsize;
> > >> -
> > >> -		hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
> > >> -				   linebuf, sizeof(linebuf), ascii);
> > >> -
> > >> +	for_each_line_in_hex_dump(i, rowsize, linebuf, sizeof(linebuf),
> > >> +				  groupsize, buf, len) {  
> > > Several callers of print_hex_dump pass true for the ascii parameter,
> > > which gets passed along to hex_dump_to_buffer. But you ignore it in
> > > for_each_line_in_hex_dump and always use false:
> > >
> > > + #define for_each_line_in_hex_dump(i, rowsize, linebuf, linebuflen, groupsize, \
> > > +				   buf, len) \
> > > +	for ((i) = 0;							\
> > > +	     (i) < (len) &&						\
> > > +	     hex_dump_to_buffer((unsigned char *)(buf) + (i),		\
> > > +				(len) - (i), (rowsize), (groupsize),	\
> > > +				(linebuf), (linebuflen), false);	\
> > > +	     (i) += (rowsize) == 32 ? 32 : 16				\
> > > +	    )
> > >
> > > Is this behavior change intended?
> > >
> > > -Dave  
> > 
> > Yes, for simplicity, I wanted to limit the number of parameters that the 
> > macro had.
> > 
> > Since the function does not do any printing, the user can do ascii 
> > conversion on their own
> > 
> > or even easier just ensure a \NULL term and print the string with the %s 
> > format specifier.
> 
> That just isn't the same.
> The hexdump code 'sanitises' the string for you.
> 

I see now, I forgot about print_hex_dump's use of
for_each_line_in_hexdump in my response the morning. Yes the
ascii argument needs to be respected and this patch forces
false, this is an oversight on my end, thanks for pointing it out.

I will address this issue in the next version by passing the ascii
argument into the for_each loop. Will wait a few weeks before sending
this time.

- Nick

[PATCH net-next v3 3/3] ibmvnic: Print data buffers with kernel API's

[Nick Child]

Previously, data buffers that were to be printed were cast to 8 byte
integers and printed. This can lead to buffer overflow if the length
of the buffer is not a multiple of 8.

Simplify and safeguard printing by using kernel provided functions
to print these data blobs.

Signed-off-by: Nick Child <nnac123@linux.ibm.com>
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index e95ae0d39948..a8f1feb9a2e7 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -4834,6 +4834,7 @@ static int send_login(struct ibmvnic_adapter *adapter)
 	struct device *dev = &adapter->vdev->dev;
 	struct vnic_login_client_data *vlcd;
 	dma_addr_t rsp_buffer_token;
+	unsigned char hex_str[16 * 3];
 	dma_addr_t buffer_token;
 	size_t rsp_buffer_size;
 	union ibmvnic_crq crq;
@@ -4937,9 +4938,9 @@ static int send_login(struct ibmvnic_adapter *adapter)
 	vnic_add_client_data(adapter, vlcd);
 
 	netdev_dbg(adapter->netdev, "Login Buffer:\n");
-	for (i = 0; i < (adapter->login_buf_sz - 1) / 8 + 1; i++) {
-		netdev_dbg(adapter->netdev, "%016lx\n",
-			   ((unsigned long *)(adapter->login_buf))[i]);
+	for_each_line_in_hex_dump(i, 16, hex_str, sizeof(hex_str), 8,
+				  adapter->login_buf, adapter->login_buf_sz) {
+		netdev_dbg(adapter->netdev, "%s\n", hex_str);
 	}
 
 	memset(&crq, 0, sizeof(crq));
@@ -5317,15 +5318,17 @@ static void handle_query_ip_offload_rsp(struct ibmvnic_adapter *adapter)
 {
 	struct device *dev = &adapter->vdev->dev;
 	struct ibmvnic_query_ip_offload_buffer *buf = &adapter->ip_offload_buf;
+	unsigned char hex_str[16 * 3];
 	int i;
 
 	dma_unmap_single(dev, adapter->ip_offload_tok,
 			 sizeof(adapter->ip_offload_buf), DMA_FROM_DEVICE);
 
 	netdev_dbg(adapter->netdev, "Query IP Offload Buffer:\n");
-	for (i = 0; i < (sizeof(adapter->ip_offload_buf) - 1) / 8 + 1; i++)
-		netdev_dbg(adapter->netdev, "%016lx\n",
-			   ((unsigned long *)(buf))[i]);
+	for_each_line_in_hex_dump(i, 16, hex_str, sizeof(hex_str), 8, buf,
+				  sizeof(adapter->ip_offload_buf)) {
+		netdev_dbg(adapter->netdev, "%s\n", hex_str);
+	}
 
 	netdev_dbg(adapter->netdev, "ipv4_chksum = %d\n", buf->ipv4_chksum);
 	netdev_dbg(adapter->netdev, "ipv6_chksum = %d\n", buf->ipv6_chksum);
@@ -5518,6 +5521,7 @@ static int handle_login_rsp(union ibmvnic_crq *login_rsp_crq,
 	struct net_device *netdev = adapter->netdev;
 	struct ibmvnic_login_rsp_buffer *login_rsp = adapter->login_rsp_buf;
 	struct ibmvnic_login_buffer *login = adapter->login_buf;
+	unsigned char hex_str[16 * 3];
 	u64 *tx_handle_array;
 	u64 *rx_handle_array;
 	int num_tx_pools;
@@ -5556,9 +5560,10 @@ static int handle_login_rsp(union ibmvnic_crq *login_rsp_crq,
 	netdev->mtu = adapter->req_mtu - ETH_HLEN;
 
 	netdev_dbg(adapter->netdev, "Login Response Buffer:\n");
-	for (i = 0; i < (adapter->login_rsp_buf_sz - 1) / 8 + 1; i++) {
-		netdev_dbg(adapter->netdev, "%016lx\n",
-			   ((unsigned long *)(adapter->login_rsp_buf))[i]);
+	for_each_line_in_hex_dump(i, 16, hex_str, sizeof(hex_str), 8,
+				  adapter->login_rsp_buf,
+				  adapter->login_rsp_buf_sz) {
+		netdev_dbg(adapter->netdev, "%s\n", hex_str);
 	}
 
 	/* Sanity checks */
-- 
2.48.1