linux-kernel - Re: [PATCH 1/5] fbdev/efifb: Use stack memory for screeninfo structs

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: <b50388e8-fa5a-40aa-98f8-2759045cbfaa@t-8ch.de>
Date: Thu, 29 Aug 2024 09:52:17 +0200
From: Thomas Weißschuh <linux@...ssschuh.net>
To: Helge Deller <deller@....de>
Cc: Peter Jones <pjones@...hat.com>, Daniel Vetter <daniel@...ll.ch>, 
	linux-fbdev@...r.kernel.org, dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/5] fbdev/efifb: Use stack memory for screeninfo structs

On 2024-08-28 19:42:51+0000, Helge Deller wrote:
> On 8/27/24 17:25, Thomas Weißschuh wrote:
> > These variables are only used inside efifb_probe().
> > Afterwards they are using memory unnecessarily.
> 
> Did you check if this change really saves some memory?

Nope...

> With your change, the compiler will either create a hidden
> structure which it uses then, or it generates assembly
> instructions to fill the struct at runtime.
> Both options may not actually reduce the memory footprint...

Thanks for the explanation, it makes sense.

On advantage of the on-stack data would be future-proofing.
Efi efifb_probe() overrides some fields in these structs only in certain
codepaths then the globally shared data could become inconsistent.
But that's not the case today.

> Another option might be to mark the static struct __initdata
> if you expect another card to take over before the memory is
> freed at runtime. But I'm not sure if it's worth possible
> implications.

Agreed.

> Helge
> 
> > Signed-off-by: Thomas Weißschuh <linux@...ssschuh.net>
> > ---
> >   drivers/video/fbdev/efifb.c | 36 ++++++++++++++++++------------------
> >   1 file changed, 18 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> > index 8dd82afb3452..8bfe0ccbc67a 100644
> > --- a/drivers/video/fbdev/efifb.c
> > +++ b/drivers/video/fbdev/efifb.c
> > @@ -52,24 +52,6 @@ struct efifb_par {
> >   	resource_size_t size;
> >   };
> > 
> > -static struct fb_var_screeninfo efifb_defined = {
> > -	.activate		= FB_ACTIVATE_NOW,
> > -	.height			= -1,
> > -	.width			= -1,
> > -	.right_margin		= 32,
> > -	.upper_margin		= 16,
> > -	.lower_margin		= 4,
> > -	.vsync_len		= 4,
> > -	.vmode			= FB_VMODE_NONINTERLACED,
> > -};
> > -
> > -static struct fb_fix_screeninfo efifb_fix = {
> > -	.id			= "EFI VGA",
> > -	.type			= FB_TYPE_PACKED_PIXELS,
> > -	.accel			= FB_ACCEL_NONE,
> > -	.visual			= FB_VISUAL_TRUECOLOR,
> > -};
> > -
> >   static int efifb_setcolreg(unsigned regno, unsigned red, unsigned green,
> >   			   unsigned blue, unsigned transp,
> >   			   struct fb_info *info)
> > @@ -357,6 +339,24 @@ static int efifb_probe(struct platform_device *dev)
> >   	char *option = NULL;
> >   	efi_memory_desc_t md;
> > 
> > +	struct fb_var_screeninfo efifb_defined = {
> > +		.activate		= FB_ACTIVATE_NOW,
> > +		.height			= -1,
> > +		.width			= -1,
> > +		.right_margin		= 32,
> > +		.upper_margin		= 16,
> > +		.lower_margin		= 4,
> > +		.vsync_len		= 4,
> > +		.vmode			= FB_VMODE_NONINTERLACED,
> > +	};
> > +
> > +	struct fb_fix_screeninfo efifb_fix = {
> > +		.id			= "EFI VGA",
> > +		.type			= FB_TYPE_PACKED_PIXELS,
> > +		.accel			= FB_ACCEL_NONE,
> > +		.visual			= FB_VISUAL_TRUECOLOR,
> > +	};
> > +
> >   	/*
> >   	 * If we fail probing the device, the kernel might try a different
> >   	 * driver. We get a copy of the attached screen_info, so that we can
> > 
>