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

[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@xxxxxxxxxxxxxxxxxx>
> > ---
> >  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/