| 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: <1255473061.21871.40.camel@concordia>
Date: Wed, 14 Oct 2009 09:31:01 +1100
From: Michael Ellerman <michael@...erman.id.au>
To: Nathan Fontenot <nfont@...tin.ibm.com>
Cc: linuxppc-dev@...abs.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/5 v3] kernel handling of memory DLPAR
On Tue, 2009-10-13 at 13:13 -0500, Nathan Fontenot wrote:
> This adds the capability to DLPAR add and remove memory from the kernel. The
Hi Nathan,
Sorry to only get around to reviewing version 3, time is a commodity in
short supply :)
> Index: powerpc/arch/powerpc/platforms/pseries/dlpar.c
> ===================================================================
> --- powerpc.orig/arch/powerpc/platforms/pseries/dlpar.c 2009-10-08 11:08:42.000000000 -0500
> +++ powerpc/arch/powerpc/platforms/pseries/dlpar.c 2009-10-13 13:08:22.000000000 -0500
> @@ -16,6 +16,10 @@
> #include <linux/notifier.h>
> #include <linux/proc_fs.h>
> #include <linux/spinlock.h>
> +#include <linux/memory_hotplug.h>
> +#include <linux/sysdev.h>
> +#include <linux/sysfs.h>
> +
>
> #include <asm/prom.h>
> #include <asm/machdep.h>
> @@ -404,11 +408,165 @@
> return 0;
> }
>
> +#ifdef CONFIG_MEMORY_HOTPLUG
> +
> +static struct property *clone_property(struct property *old_prop)
> +{
> + struct property *new_prop;
> +
> + new_prop = kzalloc((sizeof *new_prop), GFP_KERNEL);
> + if (!new_prop)
> + return NULL;
> +
> + new_prop->name = kzalloc(strlen(old_prop->name) + 1, GFP_KERNEL);
kstrdup()?
> + new_prop->value = kzalloc(old_prop->length + 1, GFP_KERNEL);
> + if (!new_prop->name || !new_prop->value) {
> + free_property(new_prop);
> + return NULL;
> + }
> +
> + strcpy(new_prop->name, old_prop->name);
> + memcpy(new_prop->value, old_prop->value, old_prop->length);
> + new_prop->length = old_prop->length;
> +
> + return new_prop;
> +}
> +
> +int platform_probe_memory(u64 phys_addr)
> +{
> + struct device_node *dn;
> + struct property *new_prop, *old_prop;
> + struct property *lmb_sz_prop;
> + struct of_drconf_cell *drmem;
> + u64 lmb_size;
> + int num_entries, i, rc;
> +
> + if (!phys_addr)
> + return -EINVAL;
> +
> + dn = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");
> + if (!dn)
> + return -EINVAL;
> +
> + lmb_sz_prop = of_find_property(dn, "ibm,lmb-size", NULL);
> + lmb_size = *(u64 *)lmb_sz_prop->value;
of_get_property() ?
> +
> + old_prop = of_find_property(dn, "ibm,dynamic-memory", NULL);
I know we should never fail to find these properties, but it would be
nice to check just in case.
> +
> + num_entries = *(u32 *)old_prop->value;
> + drmem = (struct of_drconf_cell *)
> + ((char *)old_prop->value + sizeof(u32));
You do this dance twice (see below), a struct might make it cleaner.
> + for (i = 0; i < num_entries; i++) {
> + u64 lmb_end_addr = drmem[i].base_addr + lmb_size;
> + if (phys_addr >= drmem[i].base_addr
> + && phys_addr < lmb_end_addr)
> + break;
> + }
> +
> + if (i >= num_entries) {
> + of_node_put(dn);
> + return -EINVAL;
> + }
> +
> + if (drmem[i].flags & DRCONF_MEM_ASSIGNED) {
> + of_node_put(dn);
> + return 0;
This is the already added case?
> + }
> +
> + rc = acquire_drc(drmem[i].drc_index);
> + if (rc) {
> + of_node_put(dn);
> + return -1;
-1 ?
> + }
> +
> + new_prop = clone_property(old_prop);
> + drmem = (struct of_drconf_cell *)
> + ((char *)new_prop->value + sizeof(u32));
> +
> + drmem[i].flags |= DRCONF_MEM_ASSIGNED;
> + prom_update_property(dn, new_prop, old_prop);
> +
> + rc = blocking_notifier_call_chain(&pSeries_reconfig_chain,
> + PSERIES_DRCONF_MEM_ADD,
> + &drmem[i].base_addr);
> + if (rc == NOTIFY_BAD) {
> + prom_update_property(dn, old_prop, new_prop);
> + release_drc(drmem[i].drc_index);
> + }
> +
> + of_node_put(dn);
> + return rc == NOTIFY_BAD ? -1 : 0;
-1 ?
> +}
> +
> +static ssize_t memory_release_store(struct class *class, const char *buf,
> + size_t count)
> +{
> + unsigned long drc_index;
> + struct device_node *dn;
> + struct property *new_prop, *old_prop;
> + struct of_drconf_cell *drmem;
> + int num_entries;
> + int i, rc;
> +
> + rc = strict_strtoul(buf, 0, &drc_index);
> + if (rc)
> + return -EINVAL;
> +
> + dn = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");
> + if (!dn)
> + return 0;
0 really?
> +
> + old_prop = of_find_property(dn, "ibm,dynamic-memory", NULL);
> + new_prop = clone_property(old_prop);
> +
> + num_entries = *(u32 *)new_prop->value;
> + drmem = (struct of_drconf_cell *)
> + ((char *)new_prop->value + sizeof(u32));
> +
> + for (i = 0; i < num_entries; i++) {
> + if (drmem[i].drc_index == drc_index)
> + break;
> + }
> +
> + if (i >= num_entries) {
> + free_property(new_prop);
> + of_node_put(dn);
> + return -EINVAL;
> + }
Couldn't use old_prop up until here? They're identical aren't they, so
you can do the clone here and you can avoid the free in the above error
case.
> + drmem[i].flags &= ~DRCONF_MEM_ASSIGNED;
> + prom_update_property(dn, new_prop, old_prop);
> +
> + rc = blocking_notifier_call_chain(&pSeries_reconfig_chain,
> + PSERIES_DRCONF_MEM_REMOVE,
> + &drmem[i].base_addr);
> + if (rc != NOTIFY_BAD)
> + rc = release_drc(drc_index);
> +
> + if (rc)
> + prom_update_property(dn, old_prop, new_prop);
> +
> + of_node_put(dn);
> + return rc ? -1 : count;
-1, EPERM?
> +}
> +
> +static struct class_attribute class_attr_mem_release =
> + __ATTR(release, S_IWUSR, NULL, memory_release_store);
> +#endif
> +
> static int pseries_dlpar_init(void)
> {
> if (!machine_is(pseries))
> return 0;
>
> +#ifdef CONFIG_MEMORY_HOTPLUG
> + if (sysfs_create_file(&memory_sysdev_class.kset.kobj,
> + &class_attr_mem_release.attr))
> + printk(KERN_INFO "DLPAR: Could not create sysfs memory "
> + "release file\n");
> +#endif
> +
> return 0;
> }
> device_initcall(pseries_dlpar_init);
> Index: powerpc/arch/powerpc/mm/mem.c
> ===================================================================
> --- powerpc.orig/arch/powerpc/mm/mem.c 2009-10-08 11:07:45.000000000 -0500
> +++ powerpc/arch/powerpc/mm/mem.c 2009-10-08 11:08:54.000000000 -0500
> @@ -111,8 +111,19 @@
> #ifdef CONFIG_MEMORY_HOTPLUG
>
> #ifdef CONFIG_NUMA
> +int __attribute ((weak)) platform_probe_memory(u64 start)
__weak
Though be careful, I think this is vulnerable to a bug in some
toolchains where the compiler will inline this version. See the comment
around early_irq_init() in kernel/softirq.c for example.
This will need to be a ppc_md hook as soon as another platform supports
memory hotplug, though that may be never :)
> +{
> + return 0;
> +}
> +
> int memory_add_physaddr_to_nid(u64 start)
> {
> + int rc;
> +
> + rc = platform_probe_memory(start);
> + if (rc)
> + return rc;
> +
> return hot_add_scn_to_nid(start);
> }
> #endif
cheers
Download attachment "signature.asc" of type "application/pgp-signature" (198 bytes)
Powered by blists - more mailing lists