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

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: <Z7i56s7jwc_y0cIz@li-4c4c4544-0047-5210-804b-b8c04f323634.ibm.com>
Date: Fri, 21 Feb 2025 11:37:46 -0600
From: Nick Child <nnac123@...ux.ibm.com>
To: David Laight <david.laight.linux@...il.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org, horms@...nel.org,
        nick.child@....com, pmladek@...e.com, rostedt@...dmis.org,
        john.ogness@...utronix.de, senozhatsky@...omium.org
Subject: Re: [PATCH net-next v3 1/3] hexdump: Implement macro for converting
 large buffers

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@...ux.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/