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

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: <20110714060751.GA5386@stefanha-thinkpad.localdomain>
Date:	Thu, 14 Jul 2011 07:07:51 +0100
From:	Stefan Hajnoczi <stefanha@...ux.vnet.ibm.com>
To:	Anton Vorontsov <cbouatmailru@...il.com>
Cc:	Len Brown <lenb@...nel.org>, David Woodhouse <dwmw2@...radead.org>,
	linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] power_supply: scrub device pointer if registration
 fails

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
--
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/