linux-kernel - Re: [PATCH hotfix 6.11] minmax: reduce egregious min/max macro expansion

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: <0a73fda5-e6b5-446c-832a-e6df8d3c0c90@lucifer.local>
Date: Wed, 11 Sep 2024 18:36:04 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: Hans de Goede <hdegoede@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Richard Narron <richard@...zen.com>,
        Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Marcin Wojtas <marcin.s.wojtas@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S . Miller" <davem@...emloft.net>,
        Arnd Bergmann <arnd@...nel.org>,
        Linus Torvalds <torvalds@...uxfoundation.org>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
        linux-staging@...ts.linux.dev, linux-mm@...ck.org,
        stable@...r.kernel.org
Subject: Re: [PATCH hotfix 6.11] minmax: reduce egregious min/max macro
 expansion

On Wed, Sep 11, 2024 at 08:25:33PM GMT, Dan Carpenter wrote:
> On Wed, Sep 11, 2024 at 06:24:54PM +0200, Hans de Goede wrote:
> > Hi Lorenzo,
> >
> > On 9/11/24 5:34 PM, Lorenzo Stoakes wrote:
> > > Avoid nested min()/max() which results in egregious macro expansion.
> > >
> > > This issue was introduced by commit 867046cc7027 ("minmax: relax check to
> > > allow comparison between unsigned arguments and signed constants") [2].
> > >
> > > Work has been done to address the issue of egregious min()/max() macro
> > > expansion in commit 22f546873149 ("minmax: improve macro expansion and type
> > > checking") and related, however it appears that some issues remain on more
> > > tightly constrained systems.
> > >
> > > Adjust a few known-bad cases of deeply nested macros to avoid doing so to
> > > mitigate this. Porting the patch first proposed in [1] to Linus's tree.
> > >
> > > Running an allmodconfig build using the methodology described in [2] we
> > > observe a 35 MiB reduction in generated code.
> > >
> > > The difference is much more significant prior to recent minmax fixes which
> > > were not backported. As per [1] prior these the reduction is more like 200
> > > MiB.
> > >
> > > This resolves an issue with slackware 15.0 32-bit compilation as reported
> > > by Richard Narron.
> > >
> > > Presumably the min/max fixups would be difficult to backport, this patch
> > > should be easier and fix's Richard's problem in 5.15.
> > >
> > > [0]:https://lore.kernel.org/all/b97faef60ad24922b530241c5d7c933c@AcuMS.aculab.com/
> > > [1]:https://lore.kernel.org/lkml/5882b96e-1287-4390-8174-3316d39038ef@lucifer.local/
> > > [2]:https://lore.kernel.org/linux-mm/36aa2cad-1db1-4abf-8dd2-fb20484aabc3@lucifer.local/
> > >
> > > Reported-by: Richard Narron <richard@...zen.com>
> > > Closes: https://lore.kernel.org/all/4a5321bd-b1f-1832-f0c-cea8694dc5aa@aaazen.com/
> > > Fixes: 867046cc7027 ("minmax: relax check to allow comparison between unsigned arguments and signed constants")
> > > Cc: stable@...r.kernel.org
> > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> >
> > Thank you for your patch.
> >
> > I must say that I'm not a fan of that this is patching 3 totally
> > unrelated files here in a single patch.
> >
> > This is e.g. going to be a problem if we need to revert one of
> > the changes because of regressions...
>
> These kinds of thing also complicates backporting to stable.  The stable kernel
> developers like whole, unmodified patches.  So if we have to fix something in
> sDIGIT_FITTING() then we'd want to pull this back instead of re-writing the fix
> on top of the original define (unmodified patches).  But now we have to backport
> the chunk which changes mvpp2 as well (whole patches).

Sure absolutely, as I said to Hans I did it all as one as I wanted to get
this out quickly as a favour to Richard, but this was a mistake, very
obviously it's much easier to have these separate.

About to send out a v2 with this done. Cheers!

>
> regards,
> dan carpenter
>