linux-kernel - Re: [PATCH 2/2] regmap: debugfs: improve regmap_reg_ranges_read

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: <20150929183024.GC30445@sirena.org.uk>
Date:	Tue, 29 Sep 2015 19:30:24 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Rasmus Villemoes <linux@...musvillemoes.dk>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] regmap: debugfs: improve
 regmap_reg_ranges_read_file()

On Tue, Sep 29, 2015 at 12:29:02AM +0200, Rasmus Villemoes wrote:

This patch is an example of why SubmittingPatches recommends splitting
things up into one change per patch, it would be much easier to read and
review as a series (especially given that there's very few collisions).

> * A page is a bit much for two integers and a bit of punctuation. 64
>   bytes should suffice.

Right, the reason PAGE_SIZE was chosen is that it's a natural unit for
the allocator and is clearly absurdly large for the data.  For 64 bytes
I have to think for a moment if it's suitably large.  Please leave it at
PAGE_SIZE, it's not like this hangs around for any length of time.

> * Calling strlen() on entry no less than three times is silly,
>   especially when snprintf() has returned that value (which was just
>   thrown away).

I think we were expecting the compiler would figure out that strlen() is
a pure function and do the right thing here (though I do see it's
missing an annotation).

> * Transferring entry to the output buffer using snprintf is silly,
>   when we know the length. Use memcpy instead.

Right, that's a legacy of a previous version transferring the maximum
amount of data possible (which got abandoned due to complexity).
However with the changes to use the return value of snprinf() it seems
like the best thing to do here is to go back to this and just fill up as
much of the buffer as we can by using snprintf() to do the transfer.

That said I think memcpy() is going to be the best way of getting to
that since one of the issues there (which currently doesn't work) is
slicing things off the front and memcpy() handles that nicely.

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)