| 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
| ||
|
Message-ID: <20131216030229.GA7408@dhcp-16-126.nay.redhat.com>
Date: Mon, 16 Dec 2013 11:02:29 +0800
From: Dave Young <dyoung@...hat.com>
To: Matt Fleming <matt@...sole-pimps.org>
Cc: Borislav Petkov <bp@...en8.de>, linux-kernel@...r.kernel.org,
linux-efi@...r.kernel.org, x86@...nel.org, mjg59@...f.ucam.org,
hpa@...or.com, James.Bottomley@...senPartnership.com,
vgoyal@...hat.com, ebiederm@...ssion.com, horms@...ge.net.au,
kexec@...ts.infradead.org, greg@...ah.com, toshi.kani@...com
Subject: Re: [PATCH v5 08/14] efi: export efi runtime memory mapping to sysfs
On 12/16/13 at 09:33am, Dave Young wrote:
> On 12/13/13 at 12:30pm, Matt Fleming wrote:
> > On Fri, 13 Dec, at 03:26:00PM, Dave Young wrote:
> > > On 12/12/13 at 09:53pm, Borislav Petkov wrote:
> > > > On Thu, Dec 12, 2013 at 10:36:17AM +0800, Dave Young wrote:
> > > > > Sorry that I forgot to explain this in changelog, should ask you before.
> > > > >
> > > > > I did try moving it to efisubsys_init but it need add extern declaration
> > > >
> > > > So what? Add it.
> > > >
> > > > > for efi_runtime_map_init. Since link order will ensure it being called after
> > > > > efisubsys_init I think it's fine with this static function.
> > > >
> > > > This is actually exactly the reason why it shouldn't be another
> > > > subsys_initcall() if it can be helped. People would need to go have a
> > > > look at drivers/firmware/efi/Makefile to realize that the link order is
> > > > ok.
> > > >
> > > > And, this is prone to future breakage if people go and move code around
> > > > and thereby change the link order.
> > > >
> > > > Please call efi_runtime_map_init from efisubsys_init.
> > >
> > > After moving to efisubsys_init, the code will looks like below, because
>
> Will call it in efi_subsys_init as what I replied previously.
Actually change a bit from your below suggestion.
>
> > > the funtion only is used in drivers/firmware/efi/efi.c thus I can not
> > > add a blank function in #else section in the efi.h header file. Creating
> > > another header file for this one function looks bad to me.
> >
> > I agree with Borislav that this should be invoked from efisubsys_init().
> > Also Dave, there's nothing inherently x86-specific about runtime-map.c -
> > we've been trying not to dump arch-specific code in
> > drivers/firmware/efi/.
> >
> > How about something like this, just to give you an idea? This would
> > allow the ARM/ia64 folks to enable this code if they want at some later
> > date. But more than that, it makes the code more self-contained and uses
> > some real interfaces instead of relying on global variables,
> >
> > (not compile tested)
>
> Thanks for the good idea, will tune and test the code.
>
> > diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> > index 94a1cbcf6e2e..f45ea41deae0 100644
> > --- a/arch/x86/platform/efi/efi.c
> > +++ b/arch/x86/platform/efi/efi.c
> > @@ -76,8 +76,8 @@ static __initdata efi_config_table_type_t arch_tables[] = {
> > {NULL_GUID, NULL, NULL},
> > };
> >
> > -void *efi_runtime_map;
> > -int nr_efi_runtime_map;
> > +static void *efi_runtime_map;
> > +static int nr_efi_runtime_map;
> > static u64 efi_setup; /* efi setup_data physical address */
> >
> > /*
> > @@ -1084,6 +1084,9 @@ void __init efi_enter_virtual_mode(void)
> > }
> > }
> >
> > + efi_runtime_map_setup(efi_runtime_map, nr_efi_runtime_map,
> > + boot_params.efi_info.efi_memdesc_size);
> > +
> > BUG_ON(!efi.systab);
> >
> > efi_setup_page_tables();
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 0a288e306252..9870eb55d0e5 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -38,8 +38,7 @@ struct efi __read_mostly efi = {
> > };
> > EXPORT_SYMBOL(efi);
> >
> > -struct kobject *efi_kobj;
> > -EXPORT_SYMBOL_GPL(efi_kobj);
> > +static struct kobject *efi_kobj;
> > static struct kobject *efivars_kobj;
> >
> > /*
> > @@ -176,6 +175,8 @@ static int __init efisubsys_init(void)
> > goto err_remove_group;
> > }
> >
> > + efi_runtime_map_init(efi_kobj);
Will add #ifdef CONFIG_EFI_RUNTIME_MAP around above line and add
error handling
Also will update the usage of gloable efi_kobj, pass it via function
parameter.
> > +
> > return 0;
> >
> > err_remove_group:
> > diff --git a/drivers/firmware/efi/runtime-map.c b/drivers/firmware/efi/runtime-map.c
> > index e5e998494d8a..ef9631e9c03e 100644
> > --- a/drivers/firmware/efi/runtime-map.c
> > +++ b/drivers/firmware/efi/runtime-map.c
> > @@ -14,6 +14,10 @@
> >
> > #include <asm/setup.h>
> >
> > +static void *efi_runtime_map;
> > +static int nr_efi_runtime_map;
> > +static u32 efi_memdesc_size;
> > +
> > struct efi_runtime_map_entry {
> > efi_memory_desc_t md;
> > struct kobject kobj; /* kobject for each entry */
> > @@ -105,7 +109,6 @@ static struct efi_runtime_map_entry *add_sysfs_runtime_map_entry(int nr)
> > {
> > int ret;
> > struct efi_runtime_map_entry *entry;
> > - struct efi_info *e = &boot_params.efi_info;
> >
> > if (!map_kset) {
> > map_kset = kset_create_and_add("runtime-map", NULL,
> > @@ -120,7 +123,7 @@ static struct efi_runtime_map_entry *add_sysfs_runtime_map_entry(int nr)
> > return entry;
> > }
> >
> > - memcpy(&entry->md, efi_runtime_map + nr * e->efi_memdesc_size,
> > + memcpy(&entry->md, efi_runtime_map + nr * efi_memdesc_size,
> > sizeof(efi_memory_desc_t));
> >
> > kobject_init(&entry->kobj, &map_ktype);
> > @@ -135,7 +138,14 @@ static struct efi_runtime_map_entry *add_sysfs_runtime_map_entry(int nr)
> > return entry;
> > }
> >
> > -static int __init efi_runtime_map_init(void)
> > +int efi_runtime_map_setup(void *map, int nr_entries, u32 desc_size)
Will change it to void instead of return int
> > +{
> > + efi_runtime_map = map;
> > + nr_efi_runtime_map = nr_entries;
> > + efi_memdesc_size = desc_size;
> > +}
> > +
> > +int __init efi_runtime_map_init(struct kobject *efi_kobj)
> > {
> > int i, j, ret = 0;
> > struct efi_runtime_map_entry *entry;
> > @@ -172,5 +182,3 @@ out_add_entry:
> > out:
> > return ret;
> > }
> > -
> > -subsys_initcall(efi_runtime_map_init);
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index 4f1651d303b8..47e067044054 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -873,9 +873,8 @@ int efivars_sysfs_init(void);
> > #endif /* CONFIG_EFI_VARS */
> >
> > #ifdef CONFIG_EFI_RUNTIME_MAP
> > -extern void *efi_runtime_map;
> > -extern int nr_efi_runtime_map;
> > -extern struct kobject *efi_kobj;
> > +int efi_runtime_map_init(struct kobject *);
> > +int efi_runtime_map_setup(void *, int, u32);
Will change to void.
> > #endif
> >
> > #endif /* _LINUX_EFI_H */
> >
> > --
> > Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists