[PATCH] regulator core: fix double-free in regulator_register() error path

[PATCH] regulator core: fix double-free in regulator_register() error path

[Paul Walmsley]

During regulator registration, any error after device_register() will 
cause a double-free on the struct regulator_dev 'rdev'.  The bug is in 
drivers/regulator/core.c:regulator_register():

...
scrub:
	device_unregister(&rdev->dev);
clean:
	kfree(rdev);                           <---
	rdev = ERR_PTR(ret);
	goto out;
...

device_unregister() calls regulator_dev_release() which frees rdev.  The 
subsequent kfree corrupts memory and causes some OMAP3 systems to oops on 
boot in regulator_get().

Applies against 2.6.30-rc3.

Signed-off-by: Paul Walmsley <paul@pwsan.com>
---
 drivers/regulator/core.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 01f7702..fabd2e0 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2080,6 +2080,10 @@ out:
 
 scrub:
 	device_unregister(&rdev->dev);
+	/* device core frees rdev */
+	rdev = ERR_PTR(ret);
+	goto out;
+
 clean:
 	kfree(rdev);
 	rdev = ERR_PTR(ret);
-- 
1.6.3.rc1.51.gea0b7

Re: [PATCH] regulator core: fix double-free in regulator_register() error path

[David Brownell]

On Saturday 25 April 2009, Paul Walmsley wrote:
> 
> During regulator registration, any error after device_register() will 
> cause a double-free on the struct regulator_dev 'rdev'.  The bug is in 
> drivers/regulator/core.c:regulator_register():
> 
> ...
> scrub:
> 	device_unregister(&rdev->dev);
> clean:
> 	kfree(rdev);                           <---
> 	rdev = ERR_PTR(ret);
> 	goto out;
> ...
> 
> device_unregister() calls regulator_dev_release() which frees rdev.  The 
> subsequent kfree corrupts memory and causes some OMAP3 systems to oops on 
> boot in regulator_get().
> 
> Applies against 2.6.30-rc3.
> 
> Signed-off-by: Paul Walmsley <paul@pwsan.com>

This looks like it would address the oopsing I mentioned a
while back, since affects cleanup paths after errors during
driver probe().


> ---
>  drivers/regulator/core.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 01f7702..fabd2e0 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -2080,6 +2080,10 @@ out:
>  
>  scrub:
>  	device_unregister(&rdev->dev);
> +	/* device core frees rdev */
> +	rdev = ERR_PTR(ret);
> +	goto out;
> +
>  clean:
>  	kfree(rdev);
>  	rdev = ERR_PTR(ret);
> -- 
> 1.6.3.rc1.51.gea0b7
> 
> 

Re: [PATCH] regulator core: fix double-free in regulator_register() error path

[Paul Walmsley]

On Sat, 25 Apr 2009, Paul Walmsley wrote:

> During regulator registration, any error after device_register() will 
> cause a double-free on the struct regulator_dev 'rdev'.  The bug is in 
> drivers/regulator/core.c:regulator_register():
>
> ...
> 
> device_unregister() calls regulator_dev_release() which frees rdev.  The 
> subsequent kfree corrupts memory and causes some OMAP3 systems to oops on 
> boot in regulator_get().

For the 3430SDP users out there, this patch also fixes the boot hang after 
"regulator_init_complete: incomplete constraints, leaving VAUX3 on"
on that device.

- Paul

Re: [PATCH] regulator core: fix double-free in regulator_register() error path

[David Brownell]

On Saturday 25 April 2009, Paul Walmsley wrote:
> 
> > device_unregister() calls regulator_dev_release() which frees rdev.  The 
> > subsequent kfree corrupts memory and causes some OMAP3 systems to oops on 
> > boot in regulator_get().
> 
> For the 3430SDP users out there, this patch also fixes the boot hang after 
> "regulator_init_complete: incomplete constraints, leaving VAUX3 on"
> on that device.

For the record, that "incomplete constraints" message is bogus.
On that board, VAUX3 has a complete set of constraints:  it may
only emit 2.8V.

What it lacks is something entirely different:  driver support
for the LCD which uses the regulator framework, instead of just
bypassing it and talking directly to the PMIC.  By the time it
gets there, the LCD has probably been turned on.

Mark and/or Liam ... you might want to fix that diagnostic, to
avoid leading more developers astray!

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] regulator core: fix double-free in regulator_register() error path

[Mark Brown]

On Mon, Apr 27, 2009 at 08:08:50PM -0700, David Brownell wrote:
> On Saturday 25 April 2009, Paul Walmsley wrote:

> > For the 3430SDP users out there, this patch also fixes the boot hang after 
> > "regulator_init_complete: incomplete constraints, leaving VAUX3 on"
> > on that device.

> For the record, that "incomplete constraints" message is bogus.
> On that board, VAUX3 has a complete set of constraints:  it may
> only emit 2.8V.

It's not VAUX3 that it's saying has incomplete constraints, it's the
board as a whole - if the constraints for the board were fully specified
(and the core had been told about it) then it would power off VAUX3 at
that point.

> Mark and/or Liam ... you might want to fix that diagnostic, to
> avoid leading more developers astray!

Probably shove a "board has" in there or something I guess.

Re: [PATCH] regulator core: fix double-free in regulator_register() error path

[David Brownell]

On Tuesday 28 April 2009, Mark Brown wrote:
> > For the record, that "incomplete constraints" message is bogus.
> > On that board, VAUX3 has a complete set of constraints:  it may
> > only emit 2.8V.
> 
> It's not VAUX3 that it's saying has incomplete constraints, it's the
> board as a whole - if the constraints for the board were fully specified

No; driver support != constraint.  Only one of the
issues is packaged as a "constraint".


> (and the core had been told about it) then it would power off VAUX3 at
> that point.
>
> > Mark and/or Liam ... you might want to fix that diagnostic, to
> > avoid leading more developers astray!
> 
> Probably shove a "board has" in there or something I guess.

How about:  "VAUX3 board support is incomplete".
That's accurate.

A comment there might suggest constraints and
driver support as two common issues ... I'd not
be surprised to see a few more idioms pop up.

- Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] regulator core: fix double-free in regulator_register() error path

[Mark Brown]

On Tue, Apr 28, 2009 at 02:32:56AM -0700, David Brownell wrote:
> On Tuesday 28 April 2009, Mark Brown wrote:

> > It's not VAUX3 that it's saying has incomplete constraints, it's the
> > board as a whole - if the constraints for the board were fully specified

> No; driver support != constraint.  Only one of the
> issues is packaged as a "constraint".

Driver support isn't particularly relevant here.  If there are devices
that don't have drivers or don't have drivers with regulator support but
need a regulator enabling then the board should mark the regulator as
always_on when setting full constraints; the always_on flag can be
removed when the required consumers are added.

> > > Mark and/or Liam ... you might want to fix that diagnostic, to
> > > avoid leading more developers astray!

> > Probably shove a "board has" in there or something I guess.

> How about:  "VAUX3 board support is incomplete".
> That's accurate.

No.  The constraints being complete is a property of the board as a
whole and not the particular regulator.

Re: [PATCH] regulator core: fix double-free in regulator_register() error path

[David Brownell]

On Tuesday 28 April 2009, Mark Brown wrote:
> On Tue, Apr 28, 2009 at 02:32:56AM -0700, David Brownell wrote:
> > On Tuesday 28 April 2009, Mark Brown wrote:
> > > >
> > > > For the record, that "incomplete constraints" message is bogus.
> > > > On that board, VAUX3 has a complete set of constraints:  it may
> > > > only emit 2.8V.
> > > > 
> > > > What it lacks is something entirely different:  driver support
> > > > for the LCD which uses the regulator framework,
> > > 
> > > It's not VAUX3 that it's saying has incomplete constraints, it's the
> > > board as a whole - if the constraints for the board were fully specified
> >
> > No; driver support != constraint.  Only one of the
> > issues is packaged as a "constraint".
> 
> Driver support isn't particularly relevant here.

It's the *entire* point.  The driver is talking directly
to the regulator, bypassing this framework.  The constraints
on that regulator are fully defined ... and then bypassed.


> > > > Mark and/or Liam ... you might want to fix that diagnostic, to
> > > > avoid leading more developers astray!
> 
> > > Probably shove a "board has" in there or something I guess.
> 
> > How about:  "VAUX3 board support is incomplete".
> > That's accurate.
> 
> No.  The constraints being complete is a property of the board as a
> whole and not the particular regulator.

Except ... that "constraint" isn't the issue, it's
unexpected driver behavior.  And "board" is exactly
what I said, so I don't know why you're arguing.
(For the "fun" of it?)

Board support includes full driver support, as well
as board setup (constraints).  That's the common way
to factor it, at any rate -- a "board support package"
addresses both, and they need to work together.

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] regulator core: fix double-free in regulator_register() error path

[Mark Brown]

On Tue, Apr 28, 2009 at 04:47:52AM -0700, David Brownell wrote:
> On Tuesday 28 April 2009, Mark Brown wrote:
> > On Tue, Apr 28, 2009 at 02:32:56AM -0700, David Brownell wrote:

> > > > > What it lacks is something entirely different: ?driver support
> > > > > for the LCD which uses the regulator framework,

> > > > It's not VAUX3 that it's saying has incomplete constraints, it's the
> > > > board as a whole - if the constraints for the board were fully specified

> > > No; driver support != constraint.  Only one of the
> > > issues is packaged as a "constraint".

> > Driver support isn't particularly relevant here.

> It's the *entire* point.  The driver is talking directly
> to the regulator, bypassing this framework.  The constraints
> on that regulator are fully defined ... and then bypassed.

Ah, I see - I didn't realise that there was driver support that doesn't
use the regulator API, that had been dropped out of context by that
point in the discussion.  I guess the LCD driver will be converted to
use the API in due course, hopefully there aren't too many other such
drivers.

This is the sort of use case that made me not want to have the API
disable unused regulators by default - there's stuff going on that the
regulator API and the code using it doesn't know about (since it's going
through some other interface to do the job in this case) so the API
can't safely do anything to that regulator.

> > > How about:  "VAUX3 board support is incomplete".
> > > That's accurate.

> > No.  The constraints being complete is a property of the board as a
> > whole and not the particular regulator.

> Except ... that "constraint" isn't the issue, it's
> unexpected driver behavior.  And "board" is exactly

This is sufficiently unexpected that it's the first time I've seen
anything bypassing the regulator API; in any case, it's just a question
of where you see the problem coming from.

My terminology comes from the fact that as far as the core is concerned
the issue is that the board didn't say it had given the core complete
constraints allowing it to fully control the regulators that are visible
to it.  The reason the board didn't do that is that is that the LCD
driver is bypassing the regulator API and the regulator API doesn't have
any way to express that possibility.

> what I said, so I don't know why you're arguing.
> (For the "fun" of it?)

David, that is really not necessary or constructive.  This sort of
disparaging remark has been a constant feature of your interactions on
regulator API issues and it is not achieving anything useful.

> Board support includes full driver support, as well
> as board setup (constraints).  That's the common way
> to factor it, at any rate -- a "board support package"
> addresses both, and they need to work together.

s/board/machine driver/; in an ideal world for Linux there is no BSP,
it's all mainline.

Re: [PATCH] regulator core: fix double-free in regulator_register() error path

[Mark Brown]

On Sat, Apr 25, 2009 at 05:28:36AM -0600, Paul Walmsley wrote:

> During regulator registration, any error after device_register() will 
> cause a double-free on the struct regulator_dev 'rdev'.  The bug is in 
> drivers/regulator/core.c:regulator_register():

Looks good:

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Re: [PATCH] regulator core: fix double-free in regulator_register() error path

[Liam Girdwood]

On Sat, 2009-04-25 at 05:28 -0600, Paul Walmsley wrote:
> During regulator registration, any error after device_register() will 
> cause a double-free on the struct regulator_dev 'rdev'.  The bug is in 
> drivers/regulator/core.c:regulator_register():
> 
> ...
> scrub:
> 	device_unregister(&rdev->dev);
> clean:
> 	kfree(rdev);                           <---
> 	rdev = ERR_PTR(ret);
> 	goto out;
> ...
> 
> device_unregister() calls regulator_dev_release() which frees rdev.  The 
> subsequent kfree corrupts memory and causes some OMAP3 systems to oops on 
> boot in regulator_get().
> 
> Applies against 2.6.30-rc3.
> 
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> ---
>  drivers/regulator/core.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 

Applied.

Thanks

Liam