[PATCH 0/3] ACPI / Battery: fix NULL pointer dereference from battery

[PATCH 0/3] ACPI / Battery: fix NULL pointer dereference from battery

[Stefan Hajnoczi]

The following oops happens non-deterministically when resuming from suspend on
a recent Acer Aspire laptop.  The issue seems to be unregistering a led trigger
that was never registered.  The led_trigger->next_trig list_head fields contain
zeroes, hence the NULL pointer dereference in list_del().

Due to the non-deterministic nature of the bug I am not sure whether the
patches eliminate the oops.  However, the patches do address real problems in
drivers/acpi/battery.c.

There is a lack of error handling in battery.c so when power_supply_register()
fails the power_supply will continue be used as if there was no error.  The
reason for this is that power_supply_register() leaves a stale
power_supply->dev pointer set when it returns an error and battery.c simply
uses that field to decide whether the power_supply is registered or not.  This
is fixed in the first patch.

The second and third patches clean up the error handling in acpi_battery_add()
to prevent a use-after-free and properly release resources.

Here is the oops from a Debian 2.6.39-2-amd64 kernel.  linux-2.6/master
contains no relevant fixes AFAICT.  Any thoughts on what is going on here
appreciated:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000008 IP: [<ffffffff81263f1a>] led_trigger_unregister+0x36/0xb4
PGD 1b3d13067 PUD 118545067 PMD 0 
Oops: 0002 [#1] SMP 
last sysfs file: /sys/devices/virtual/vc/vcsa63/uevent
CPU 0 
Modules linked in: parport_pc ppdev lp parport rfcomm bnep bluetooth crc16 acpi_cpufreq mperf cpufreq_conservative cpufreq_stats cpufreq_userspace cpufreq_powersave uinput fuse ext2 loop snd_hda_codec_hdmi snd_hda_codec_realtek joydev arc4 ecb ath9k snd_hda_intel mac80211 snd_hda_codec snd_hwdep snd_pcm ath9k_common ath9k_hw snd_seq snd_timer i915 ath snd_seq_device snd uvcvideo videodev drm_kms_helper soundcore cfg80211 drm snd_page_alloc ac sparse_keymap media wmi battery rfkill v4l2_compat_ioctl32 intel_ips pcspkr evdev i2c_i801 power_supply i2c_algo_bit i2c_core psmouse video processor button serio_raw ext3 jbd mbcache sha256_generic cryptd aes_x86_64 aes_generic cbc dm_crypt dm_mod sd_mod crc_t10dif ahci libahci libata scsi_mod ehci_hcd usbcore atl1c thermal thermal_sys [last unloaded
 : scsi_wait_scan]

Pid: 29129, comm: pm-suspend Not tainted 2.6.39-2-amd64 #1 Acer Aspire 3820/JM31_CP
RIP: 0010:[<ffffffff81263f1a>]  [<ffffffff81263f1a>] led_trigger_unregister+0x36/0xb4
RSP: 0018:ffff8801187fbd68  EFLAGS: 00010246
RAX: 0000000000000000 RBX: dead000000200200 RCX: 0000000000000004
RDX: 0000000000000000 RSI: dead000000100100 RDI: ffffffff8164ede0
     ^                     ^
rdx gets dereferenced      LIST_POISON constants (haven't been used yet)

RBP: ffff8801b321aec0 R08: 0000000000000200 R09: ffffffff81683390
R10: ffff880100000010 R11: ffff8801b0e3f800 R12: 00000000ffffffff
R13: ffff8801b3d93378 R14: 0000000000000003 R15: ffffffffffffffff
FS:  00007f83b6797700(0000) GS:ffff8801bbc00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000008 CR3: 00000001b21cc000 CR4: 00000000000006f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process pm-suspend (pid: 29129, threadinfo ffff8801187fa000, task ffff8801b2880e40)
Stack:
 ffff8801b321aec0 0000000000000004 00000000ffffffff ffffffff81263fa6
 ffff8801b0e3f820 ffffffffa01716b2 ffff8801b0e3f820 ffffffffa0171035
 ffff8801b0e3f800 ffffffffa018628f ffff8801b0e3f800 ffffffffa01863ae
Call Trace:
 [<ffffffff81263fa6>] ? led_trigger_unregister_simple+0xe/0x17
 [<ffffffffa01716b2>] ? power_supply_remove_triggers+0x16/0x80 [power_supply]
 [<ffffffffa0171035>] ? power_supply_unregister+0x15/0x1f [power_supply]
 [<ffffffffa018628f>] ? sysfs_remove_battery+0x25/0x32 [battery]
 [<ffffffffa01863ae>] ? battery_notify+0x16/0x22 [battery]
 [<ffffffff81335b48>] ? notifier_call_chain+0x2e/0x5b
 [<ffffffff810634ab>] ? __blocking_notifier_call_chain+0x4c/0x63
 [<ffffffff81076439>] ? pm_notifier_call_chain+0x15/0x2a
 [<ffffffff81076ca0>] ? enter_state+0x10c/0x12b
 [<ffffffff8107638e>] ? state_store+0xb1/0xce
 [<ffffffff8114ed16>] ? sysfs_write_file+0xe0/0x11c
 [<ffffffff810fbfe8>] ? vfs_write+0xa4/0xff
 [<ffffffff810fc0f9>] ? sys_write+0x45/0x6e
 [<ffffffff81338dd2>] ? system_call_fastpath+0x16/0x1b
Code: 64 81 53 48 bb 00 02 20 00 00 00 ad de e8 fc e2 0c 00 48 8b 55 30 48 8b 45 38 48 be 00 01 10 00 00 00 ad de 48 c7 c7 e0 ed 64 81 
 89 42 08 48 89 10 48 89 75 30 48 89 5d 38 e8 60 ec df ff 48 
RIP  [<ffffffff81263f1a>] led_trigger_unregister+0x36/0xb4
 RSP <ffff8801187fbd68>
CR2: 0000000000000008
---[ end trace 099c095de50533f3 ]---

Disassembly of led_trigger_unregister:
ffffffff81263ee4 <led_trigger_unregister>:
ffffffff81263ee4:       41 54                   push   %r12
ffffffff81263ee6:       55                      push   %rbp
ffffffff81263ee7:       48 89 fd                mov    %rdi,%rbp
ffffffff81263eea:       48 c7 c7 e0 ed 64 81    mov    $0xffffffff8164ede0,%rdi
ffffffff81263ef1:       53                      push   %rbx
ffffffff81263ef2:       48 bb 00 02 20 00 00    movabs $0xdead000000200200,%rbx
ffffffff81263ef9:       00 ad de 
ffffffff81263efc:       e8 fc e2 0c 00          callq  ffffffff813321fd <down_write>
ffffffff81263f01:       48 8b 55 30             mov    0x30(%rbp),%rdx
ffffffff81263f05:       48 8b 45 38             mov    0x38(%rbp),%rax
ffffffff81263f09:       48 be 00 01 10 00 00    movabs $0xdead000000100100,%rsi
ffffffff81263f10:       00 ad de 
ffffffff81263f13:       48 c7 c7 e0 ed 64 81    mov    $0xffffffff8164ede0,%rdi
ffffffff81263f1a:       48 89 42 08             mov    %rax,0x8(%rdx)
                                                ^--- boom!

Stefan Hajnoczi (3):
  power_supply: scrub device pointer if registration fails
  ACPI / Battery: avoid acpi_battery_add() use-after-free
  ACPI / Battery: propagate sysfs error in acpi_battery_add()

 drivers/acpi/battery.c            |   29 ++++++++++++++++++++---------
 drivers/power/power_supply_core.c |    1 +
 2 files changed, 21 insertions(+), 9 deletions(-)

-- 
1.7.5.4

[PATCH 1/3] power_supply: scrub device pointer if registration fails

[Stefan Hajnoczi]

This patch makes power_supply_register() safer for callers that are not
being careful.  When the function fails it leaves the caller's psy.dev
pointer set to the stale power supply device.  A correct caller would
handle the error return and never use psy.dev but the example of
drivers/acpi/battery.c shows otherwise.

Clear the psy.dev pointer when power_supply_register() fails so the
caller either sees a valid pointer on success or NULL on failure.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 drivers/power/power_supply_core.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index 329b46b..33d4068 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -194,6 +194,7 @@ create_triggers_failed:
 kobject_set_name_failed:
 device_add_failed:
 	put_device(dev);
+	psy->dev = NULL; /* make it crystal-clear that we failed */
 success:
 	return rc;
 }
-- 
1.7.5.4

[PATCH 2/3] ACPI / Battery: avoid acpi_battery_add() use-after-free

[Stefan Hajnoczi]

When acpi_battery_add_fs() fails the error handling code does not clean
up completely.  Moreover, it does not return resulting in a
use-after-free.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 drivers/acpi/battery.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index fcc13ac..6b3aeba 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -979,21 +979,27 @@ static int acpi_battery_add(struct acpi_device *device)
 #ifdef CONFIG_ACPI_PROCFS_POWER
 	result = acpi_battery_add_fs(device);
 #endif
-	if (!result) {
-		printk(KERN_INFO PREFIX "%s Slot [%s] (battery %s)\n",
-			ACPI_BATTERY_DEVICE_NAME, acpi_device_bid(device),
-			device->status.battery_present ? "present" : "absent");
-	} else {
+	if (result) {
 #ifdef CONFIG_ACPI_PROCFS_POWER
 		acpi_battery_remove_fs(device);
 #endif
-		kfree(battery);
+		goto fail;
 	}
 
+	printk(KERN_INFO PREFIX "%s Slot [%s] (battery %s)\n",
+		ACPI_BATTERY_DEVICE_NAME, acpi_device_bid(device),
+		device->status.battery_present ? "present" : "absent");
+
 	battery->pm_nb.notifier_call = battery_notify;
 	register_pm_notifier(&battery->pm_nb);
 
 	return result;
+
+fail:
+	sysfs_remove_battery(battery);
+	mutex_destroy(&battery->lock);
+	kfree(battery);
+	return result;
 }
 
 static int acpi_battery_remove(struct acpi_device *device, int type)
-- 
1.7.5.4

[PATCH 3/3] ACPI / Battery: propagate sysfs error in acpi_battery_add()

[Stefan Hajnoczi]

Make sure the error return from sysfs_add_battery() is checked and
propagated out from acpi_battery_add().

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 drivers/acpi/battery.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 6b3aeba..2ae2fca 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -626,8 +626,11 @@ static int acpi_battery_update(struct acpi_battery *battery)
 		acpi_battery_quirks(battery);
 		acpi_battery_init_alarm(battery);
 	}
-	if (!battery->bat.dev)
-		sysfs_add_battery(battery);
+	if (!battery->bat.dev) {
+		result = sysfs_add_battery(battery);
+		if (result)
+			return result;
+	}
 	result = acpi_battery_get_state(battery);
 	acpi_battery_quirks2(battery);
 	return result;
@@ -975,7 +978,9 @@ static int acpi_battery_add(struct acpi_device *device)
 	if (ACPI_SUCCESS(acpi_get_handle(battery->device->handle,
 			"_BIX", &handle)))
 		set_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags);
-	acpi_battery_update(battery);
+	result = acpi_battery_update(battery);
+	if (result)
+		goto fail;
 #ifdef CONFIG_ACPI_PROCFS_POWER
 	result = acpi_battery_add_fs(device);
 #endif
-- 
1.7.5.4

Re: [PATCH 1/3] power_supply: scrub device pointer if registration fails

[Anton Vorontsov]

On Tue, Jul 12, 2011 at 09:03:27AM +0100, Stefan Hajnoczi wrote:
> This patch makes power_supply_register() safer for callers that are not
> being careful.  When the function fails it leaves the caller's psy.dev
> pointer set to the stale power supply device.  A correct caller would
> handle the error return and never use psy.dev but the example of
> drivers/acpi/battery.c shows otherwise.
> 
> Clear the psy.dev pointer when power_supply_register() fails so the
> caller either sees a valid pointer on success or NULL on failure.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  drivers/power/power_supply_core.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
> index 329b46b..33d4068 100644
> --- a/drivers/power/power_supply_core.c
> +++ b/drivers/power/power_supply_core.c
> @@ -194,6 +194,7 @@ create_triggers_failed:
>  kobject_set_name_failed:
>  device_add_failed:
>  	put_device(dev);
> +	psy->dev = NULL; /* make it crystal-clear that we failed */
>  success:
>  	return rc;
>  }

I think this may easily cause races. I.e.

- ACPI calls power_supply_register, it allocates dev, sets
  psy->dev;
- Someone calls acpi_battery_notify() or acpi_battery_update(),
  which tests for psy->dev;
- power_supply_register fails, it frees dev, and then clears psy->dev;
  but it's too late, as acpi_battery_notify/acpi_battery_update thinks
  that we're fine.

I believe the whole ACPI battery logic is overcomplicated, and
needs a bit of rework. In the meantime, we could move 'psy->dev =
dev;' assignment into the end of the function, where _register
could not fail, i.e. something like this:

diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index 329b46b..9f85b70 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -169,7 +169,6 @@ int power_supply_register(struct device *parent, struct power_supply *psy)
 	dev->parent = parent;
 	dev->release = power_supply_dev_release;
 	dev_set_drvdata(dev, psy);
-	psy->dev = dev;
 
 	INIT_WORK(&psy->changed_work, power_supply_changed_work);
 
@@ -185,6 +184,8 @@ int power_supply_register(struct device *parent, struct power_supply *psy)
 	if (rc)
 		goto create_triggers_failed;
 
+	psy->dev = dev;
+
 	power_supply_changed(psy);
 
 	goto success;


But still, I don't see how this will save us from the same issue
when ACPI calls power_supply_unregister, which doesn't clear psy->dev:

static void acpi_battery_refresh(struct acpi_battery *battery)
{
        if (!battery->bat.dev)
                return;

        acpi_battery_get_info(battery);
        /* The battery may have changed its reporting units. */
        sysfs_remove_battery(battery);
        sysfs_add_battery(battery);
}

Really, ACPI battery needs some proper fixing and locking. :-/

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [PATCH 1/3] power_supply: scrub device pointer if registration fails

[Stefan Hajnoczi]

On Tue, Jul 12, 2011 at 07:09:42PM +0400, Anton Vorontsov wrote:
> On Tue, Jul 12, 2011 at 09:03:27AM +0100, Stefan Hajnoczi wrote:
> > This patch makes power_supply_register() safer for callers that are not
> > being careful.  When the function fails it leaves the caller's psy.dev
> > pointer set to the stale power supply device.  A correct caller would
> > handle the error return and never use psy.dev but the example of
> > drivers/acpi/battery.c shows otherwise.
> > 
> > Clear the psy.dev pointer when power_supply_register() fails so the
> > caller either sees a valid pointer on success or NULL on failure.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> > ---
> >  drivers/power/power_supply_core.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
> > index 329b46b..33d4068 100644
> > --- a/drivers/power/power_supply_core.c
> > +++ b/drivers/power/power_supply_core.c
> > @@ -194,6 +194,7 @@ create_triggers_failed:
> >  kobject_set_name_failed:
> >  device_add_failed:
> >  	put_device(dev);
> > +	psy->dev = NULL; /* make it crystal-clear that we failed */
> >  success:
> >  	return rc;
> >  }
> 
> I think this may easily cause races. I.e.
> 
> - ACPI calls power_supply_register, it allocates dev, sets
>   psy->dev;
> - Someone calls acpi_battery_notify() or acpi_battery_update(),
>   which tests for psy->dev;
> - power_supply_register fails, it frees dev, and then clears psy->dev;
>   but it's too late, as acpi_battery_notify/acpi_battery_update thinks
>   that we're fine.
> 
> I believe the whole ACPI battery logic is overcomplicated, and
> needs a bit of rework. In the meantime, we could move 'psy->dev =
> dev;' assignment into the end of the function, where _register
> could not fail, i.e. something like this:

Aha!  I didn't do this is because I don't know the code and was afraid
some other function somewhere would use psy->dev.  If you think it is
safer this way I'll resend the patch.

> But still, I don't see how this will save us from the same issue
> when ACPI calls power_supply_unregister, which doesn't clear psy->dev:
> 
> static void acpi_battery_refresh(struct acpi_battery *battery)
> {
>         if (!battery->bat.dev)
>                 return;
> 
>         acpi_battery_get_info(battery);
>         /* The battery may have changed its reporting units. */
>         sysfs_remove_battery(battery);
>         sysfs_add_battery(battery);
> }
> 
> Really, ACPI battery needs some proper fixing and locking. :-/

Yeah.

Stefan

Re: [PATCH 1/3] power_supply: scrub device pointer if registration fails

[Anton Vorontsov]

On Tue, Jul 12, 2011 at 04:30:17PM +0100, Stefan Hajnoczi wrote:
[...]
> > I believe the whole ACPI battery logic is overcomplicated, and
> > needs a bit of rework. In the meantime, we could move 'psy->dev =
> > dev;' assignment into the end of the function, where _register
> > could not fail, i.e. something like this:
> 
> Aha!  I didn't do this is because I don't know the code and was afraid
> some other function somewhere would use psy->dev.  If you think it is
> safer this way I'll resend the patch.

Neither is a proper fix, unfortunately. :-( Without some external lock
you can't use psy->dev as a flag to check if the registration was
successful. There is really no point trying to force core functions
to keep psy->dev in a sane state after registration has failed.

So, instead of patching power_supply core, I'd suggest the
following patch (on top of 2/3 and 3/3, so far only compile-
tested). How does it look?

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 2ae2fca..475e17c 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -101,6 +101,7 @@ enum {
 
 struct acpi_battery {
 	struct mutex lock;
+	struct mutex bat_lock;
 	struct power_supply bat;
 	struct acpi_device *device;
 	struct notifier_block pm_nb;
@@ -559,8 +560,10 @@ static int sysfs_add_battery(struct acpi_battery *battery)
 	battery->bat.get_property = acpi_battery_get_property;
 
 	result = power_supply_register(&battery->device->dev, &battery->bat);
-	if (result)
+	if (result) {
+		battery->bat.dev = NULL;
 		return result;
+	}
 	return device_create_file(battery->bat.dev, &alarm_attr);
 }
 
@@ -613,31 +616,40 @@ static int acpi_battery_update(struct acpi_battery *battery)
 	result = acpi_battery_get_status(battery);
 	if (result)
 		return result;
+
+	mutex_lock(&battery->bat_lock);
+
 	if (!acpi_battery_present(battery)) {
 		sysfs_remove_battery(battery);
 		battery->update_time = 0;
-		return 0;
+		result = 0;
+		goto out_unlock;
 	}
 	if (!battery->update_time ||
 	    old_present != acpi_battery_present(battery)) {
 		result = acpi_battery_get_info(battery);
 		if (result)
-			return result;
+			goto out_unlock;
 		acpi_battery_quirks(battery);
 		acpi_battery_init_alarm(battery);
 	}
 	if (!battery->bat.dev) {
 		result = sysfs_add_battery(battery);
 		if (result)
-			return result;
+			goto out_unlock;
 	}
 	result = acpi_battery_get_state(battery);
 	acpi_battery_quirks2(battery);
+
+out_unlock:
+	mutex_unlock(&battery->bat_lock);
 	return result;
 }
 
-static void acpi_battery_refresh(struct acpi_battery *battery)
+static void sysfs_readd_battery(struct acpi_battery *battery)
 {
+	mutex_lock(&battery->bat_lock);
+
 	if (!battery->bat.dev)
 		return;
 
@@ -645,6 +657,13 @@ static void acpi_battery_refresh(struct acpi_battery *battery)
 	/* The battery may have changed its reporting units. */
 	sysfs_remove_battery(battery);
 	sysfs_add_battery(battery);
+
+	mutex_unlock(&battery->bat_lock);
+}
+
+static void acpi_battery_refresh(struct acpi_battery *battery)
+{
+	sysfs_readd_battery(battery);
 }
 
 /* --------------------------------------------------------------------------
@@ -941,8 +960,10 @@ static void acpi_battery_notify(struct acpi_device *device, u32 event)
 					dev_name(&device->dev), event,
 					acpi_battery_present(battery));
 	/* acpi_battery_update could remove power_supply object */
+	mutex_lock(&battery->bat_lock);
 	if (old && battery->bat.dev)
 		power_supply_changed(&battery->bat);
+	mutex_unlock(&battery->bat_lock);
 }
 
 static int battery_notify(struct notifier_block *nb,
@@ -952,8 +973,7 @@ static int battery_notify(struct notifier_block *nb,
 						    pm_nb);
 	switch (mode) {
 	case PM_POST_SUSPEND:
-		sysfs_remove_battery(battery);
-		sysfs_add_battery(battery);
+		sysfs_readd_battery(battery);
 		break;
 	}
 
@@ -975,6 +995,7 @@ static int acpi_battery_add(struct acpi_device *device)
 	strcpy(acpi_device_class(device), ACPI_BATTERY_CLASS);
 	device->driver_data = battery;
 	mutex_init(&battery->lock);
+	mutex_init(&battery->bat_lock);
 	if (ACPI_SUCCESS(acpi_get_handle(battery->device->handle,
 			"_BIX", &handle)))
 		set_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags);
@@ -1019,6 +1040,7 @@ static int acpi_battery_remove(struct acpi_device *device, int type)
 	acpi_battery_remove_fs(device);
 #endif
 	sysfs_remove_battery(battery);
+	mutex_destroy(&battery->bat_lock);
 	mutex_destroy(&battery->lock);
 	kfree(battery);
 	return 0;

Re: [PATCH 1/3] power_supply: scrub device pointer if registration fails

[Stefan Hajnoczi]

On Tue, Jul 12, 2011 at 09:23:31PM +0400, Anton Vorontsov wrote:
> On Tue, Jul 12, 2011 at 04:30:17PM +0100, Stefan Hajnoczi wrote:
> [...]
> > > I believe the whole ACPI battery logic is overcomplicated, and
> > > needs a bit of rework. In the meantime, we could move 'psy->dev =
> > > dev;' assignment into the end of the function, where _register
> > > could not fail, i.e. something like this:
> > 
> > Aha!  I didn't do this is because I don't know the code and was afraid
> > some other function somewhere would use psy->dev.  If you think it is
> > safer this way I'll resend the patch.
> 
> Neither is a proper fix, unfortunately. :-( Without some external lock
> you can't use psy->dev as a flag to check if the registration was
> successful. There is really no point trying to force core functions
> to keep psy->dev in a sane state after registration has failed.
> 
> So, instead of patching power_supply core, I'd suggest the
> following patch (on top of 2/3 and 3/3, so far only compile-
> tested). How does it look?

Comments below.  Thanks for adding this lock.

> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index 2ae2fca..475e17c 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -101,6 +101,7 @@ enum {
> 
>  struct acpi_battery {
>  	struct mutex lock;
> +	struct mutex bat_lock;
>  	struct power_supply bat;
>  	struct acpi_device *device;
>  	struct notifier_block pm_nb;
> @@ -559,8 +560,10 @@ static int sysfs_add_battery(struct acpi_battery *battery)
>  	battery->bat.get_property = acpi_battery_get_property;
> 
>  	result = power_supply_register(&battery->device->dev, &battery->bat);
> -	if (result)
> +	if (result) {
> +		battery->bat.dev = NULL;
>  		return result;
> +	}
>  	return device_create_file(battery->bat.dev, &alarm_attr);
>  }
> 
> @@ -613,31 +616,40 @@ static int acpi_battery_update(struct acpi_battery *battery)
>  	result = acpi_battery_get_status(battery);
>  	if (result)
>  		return result;
> +
> +	mutex_lock(&battery->bat_lock);
> +
>  	if (!acpi_battery_present(battery)) {
>  		sysfs_remove_battery(battery);
>  		battery->update_time = 0;
> -		return 0;
> +		result = 0;
> +		goto out_unlock;
>  	}
>  	if (!battery->update_time ||
>  	    old_present != acpi_battery_present(battery)) {
>  		result = acpi_battery_get_info(battery);
>  		if (result)
> -			return result;
> +			goto out_unlock;
>  		acpi_battery_quirks(battery);
>  		acpi_battery_init_alarm(battery);
>  	}
>  	if (!battery->bat.dev) {
>  		result = sysfs_add_battery(battery);
>  		if (result)
> -			return result;
> +			goto out_unlock;
>  	}
>  	result = acpi_battery_get_state(battery);
>  	acpi_battery_quirks2(battery);
> +
> +out_unlock:
> +	mutex_unlock(&battery->bat_lock);
>  	return result;
>  }
> 
> -static void acpi_battery_refresh(struct acpi_battery *battery)
> +static void sysfs_readd_battery(struct acpi_battery *battery)

s/readd/read/?

>  {
> +	mutex_lock(&battery->bat_lock);
> +
>  	if (!battery->bat.dev)
>  		return;

Remember to unlock before return:

if (!battery->bat.dev)
    goto out;

> 
> @@ -645,6 +657,13 @@ static void acpi_battery_refresh(struct acpi_battery *battery)
>  	/* The battery may have changed its reporting units. */
>  	sysfs_remove_battery(battery);
>  	sysfs_add_battery(battery);
> +
out:
> +	mutex_unlock(&battery->bat_lock);
> +}

Stefan

Re: [PATCH 2/3] ACPI / Battery: avoid acpi_battery_add() use-after-free

[Len Brown]

applied to acpi-test
(on top of existing battery patches)


thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 3/3] ACPI / Battery: propagate sysfs error in acpi_battery_add()

[Len Brown]

applied, after merging with acpi-test.

merged version appended below.

thanks,
Len Brown, Intel Open Source Technology Center

>From eb03cb02b74df6dd0b653d5f6d976f16a434dfaf Mon Sep 17 00:00:00 2001
From: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Date: Tue, 12 Jul 2011 09:03:29 +0100
Subject: [PATCH] ACPI / Battery: propagate sysfs error in acpi_battery_add()
X-Patchwork-Hint: ignore

Make sure the error return from sysfs_add_battery() is checked and
propagated out from acpi_battery_add().

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Signed-off-by: Len Brown <len.brown@intel.com>
---
 drivers/acpi/battery.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index c771768..ffce2f0 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -630,8 +630,11 @@ static int acpi_battery_update(struct acpi_battery *battery)
 			return result;
 		acpi_battery_init_alarm(battery);
 	}
-	if (!battery->bat.dev)
-		sysfs_add_battery(battery);
+	if (!battery->bat.dev) {
+		result = sysfs_add_battery(battery);
+		if (result)
+			return result;
+	}
 	result = acpi_battery_get_state(battery);
 	acpi_battery_quirks(battery);
 	return result;
@@ -982,7 +985,9 @@ static int acpi_battery_add(struct acpi_device *device)
 	if (ACPI_SUCCESS(acpi_get_handle(battery->device->handle,
 			"_BIX", &handle)))
 		set_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags);
-	acpi_battery_update(battery);
+	result = acpi_battery_update(battery);
+	if (result)
+		goto fail;
 #ifdef CONFIG_ACPI_PROCFS_POWER
 	result = acpi_battery_add_fs(device);
 #endif
-- 
1.7.6.178.g55272