linux-kernel - Re: [PATCH v2 4/4] serial: Add kserial_rs485 to avoid wasted space due to .padding

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] [day] [month] [year] [list]

Message-ID: <55e7b5d8-9bf7-4186-fa5d-29e1e1b3a1d3@linux.intel.com>
Date:   Tue, 30 Aug 2022 13:14:27 +0300 (EEST)
From:   Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
cc:     Jiri Slaby <jirislaby@...nel.org>,
        linux-serial <linux-serial@...r.kernel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Jonathan Corbet <corbet@....net>,
        Vladimir Zapolskiy <vz@...ia.com>,
        Russell King <linux@...linux.org.uk>,
        Richard Genoud <richard.genoud@...il.com>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Claudiu Beznea <claudiu.beznea@...rochip.com>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        NXP Linux Team <linux-imx@....com>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        linux-doc@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
        linux-arm-kernel@...ts.infradead.org,
        linux-stm32@...md-mailman.stormreply.com,
        Lino Sanfilippo <LinoSanfilippo@....de>
Subject: Re: [PATCH v2 4/4] serial: Add kserial_rs485 to avoid wasted space
 due to .padding

On Tue, 30 Aug 2022, Greg Kroah-Hartman wrote:

> On Tue, Aug 30, 2022 at 12:26:29PM +0300, Ilpo Järvinen wrote:
> > On Tue, 30 Aug 2022, Greg Kroah-Hartman wrote:

> > > Ah, you are mapping this on top of the existing structure, so there was
> > > no padding in the original one, why say that?
> > 
> > While I'm not exactly sure what you tried to say with this, I'll try to 
> > answer regardless.
> > 
> > It's the opposite, there's padding in rs485_user, and therefore also in 
> > rs485_uapi. Struct serial_rs485 has padding and is part of uapi so it 
> > cannot be changed to remove the extra padding.
> > 
> > I cannot directly copy_from_user into *rs485 because it lacks the padding. 
> > Thus, the immediate rs485_uapi and then assign to rs485.
> 
> Padding could be in the middle of the structure, it's not obvious that
> it is not there.  You are just trying to drop the trailing "unused
> space", while all of the fields are identical otherwise.
> 
> So be specific about that, as padding is often in the middle of a
> structure.

Ah, sorry. I didn't realize there would be such a way to misunderstand
the message because I knew too well where the padding with this particular 
struct is.

> > If you feel ~32B per uart_port too little to be useful (and a little 
> > more per driver), I can just drop this patch.
> 
> I think 32 bytes per serial port is totally lost in the noise and would
> not even be able to be measured at all due to how slabs are aligned
> (meaning you are not actually saving any memory at all.)
>
> Can you notice any measurable savings on your systems?

It's not that straightforward. Many uart_ports are embedded into arrays
like this:

static struct ...[N];

...But then one could again say that, e.g., module alignment eats up all 
potential benefits, etc.

Obviously with big systems and small number of ports, this would never 
matter much so while I believe likely could get some small looking number 
for you I don't feel the effort needed to be anymore justified.

> And what is the code increase overall with this patch series?  :)

The series was mostly shuffling existing code around, the only thing added 
was that those struct copies so probably less than it looked.

> I'm all for making things const, to prevent errors, but that could
> probably be done without this type of change, right?

OK, I'll drop this last patch. For the first three though, there's useful 
stuff in them making some things more cleaner/consistent, I'll sort that 
out.

Thanks for your comments.


-- 
 i.