[patch] gpiolib: fix oops in gpio_get_value_cansleep()

[patch] gpiolib: fix oops in gpio_get_value_cansleep()

[David Brownell]

From: David Brownell <dbrownell@users.sourceforge.net>

We can get the following oops from gpio_get_value_cansleep()
when a GPIO controller doesn't provide a get() callback:

 Unable to handle kernel paging request for instruction fetch
 Faulting instruction address: 0x00000000
 Oops: Kernel access of bad area, sig: 11 [#1]
 [...]
 NIP [00000000] 0x0
 LR [c0182fb0] gpio_get_value_cansleep+0x40/0x50
 Call Trace:
 [c7b79e80] [c0183f28] gpio_value_show+0x5c/0x94
 [c7b79ea0] [c01a584c] dev_attr_show+0x30/0x7c
 [c7b79eb0] [c00d6b48] fill_read_buffer+0x68/0xe0
 [c7b79ed0] [c00d6c54] sysfs_read_file+0x94/0xbc
 [c7b79ef0] [c008f24c] vfs_read+0xb4/0x16c
 [c7b79f10] [c008f580] sys_read+0x4c/0x90
 [c7b79f40] [c0013a14] ret_from_syscall+0x0/0x38

It's OK to request the value of *any* GPIO; most GPIOs are
bidirectional, so configuring them as outputs just enables an
output driver and doesn't disable the input logic.

So the problem is that gpio_get_value_cansleep() isn't making
the same sanity check that gpio_get_value() does:  making sure
this GPIO isn't one of the atypical "no input logic" cases.

Reported-by: Anton Vorontsov <avorontsov@ru.mvista.com>
Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/gpio/gpiolib.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1056,7 +1056,7 @@ int gpio_get_value_cansleep(unsigned gpi
 
 	might_sleep_if(extra_checks);
 	chip = gpio_to_chip(gpio);
-	return chip->get(chip, gpio - chip->base);
+	return chip->get ? chip->get(chip, gpio - chip->base) : 0;
 }
 EXPORT_SYMBOL_GPL(gpio_get_value_cansleep);
 

Re: [patch] gpiolib: fix oops in gpio_get_value_cansleep()

[Anton Vorontsov]

On Thu, Oct 16, 2008 at 08:45:22AM -0700, David Brownell wrote:
> From: David Brownell <dbrownell@users.sourceforge.net>
[...]
> So the problem is that gpio_get_value_cansleep() isn't making
> the same sanity check that gpio_get_value() does:  making sure
> this GPIO isn't one of the atypical "no input logic" cases.
> 
> Reported-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> ---
>  drivers/gpio/gpiolib.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1056,7 +1056,7 @@ int gpio_get_value_cansleep(unsigned gpi
>  
>  	might_sleep_if(extra_checks);
>  	chip = gpio_to_chip(gpio);
> -	return chip->get(chip, gpio - chip->base);
> +	return chip->get ? chip->get(chip, gpio - chip->base) : 0;

Why don't we check the .set in the gpio_set_value? Because
we must always call gpio_direction_output()? It is not exactly the
same we work with the input direction.. is this documented anywhere?

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [patch] gpiolib: fix oops in gpio_get_value_cansleep()

[David Brownell]

On Thursday 16 October 2008, Anton Vorontsov wrote:
> > -     return chip->get(chip, gpio - chip->base);
> > +     return chip->get ? chip->get(chip, gpio - chip->base) : 0;
> 
> Why don't we check the .set in the gpio_set_value? Because
> we must always call gpio_direction_output()?

Yes.  The output driver needs to be explicitly enabled,
to avoid misbehaving electic circuitry.


> It is not exactly the 
> same we work with the input direction.. is this documented anywhere?

In Documentation/gpio.txt since the very first versions.

See the section on "Spinlock-Safe GPIO access", where
special cases for reading the value of output GPIOs are
desribed.  Other aspects are mentioned in other spots.

For example, open drain signals -- as used with I2C and
other protocols -- only drive the "low" signal level,
and if code wants a "high" level it's got to verify that
nobody else is driving that shared line to low.  By
reading it back, even though it's configured as output.

- Dave

Re: [patch] gpiolib: fix oops in gpio_get_value_cansleep()

[Anton Vorontsov]

On Thu, Oct 16, 2008 at 10:06:21AM -0700, David Brownell wrote:
> On Thursday 16 October 2008, Anton Vorontsov wrote:
> > > -     return chip->get(chip, gpio - chip->base);
> > > +     return chip->get ? chip->get(chip, gpio - chip->base) : 0;
> > 
> > Why don't we check the .set in the gpio_set_value? Because
> > we must always call gpio_direction_output()?
> 
> Yes.  The output driver needs to be explicitly enabled,
> to avoid misbehaving electic circuitry.
> 
> 
> > It is not exactly the 
> > same we work with the input direction.. is this documented anywhere?
> 
> In Documentation/gpio.txt since the very first versions.
> 
> See the section on "Spinlock-Safe GPIO access", where
> special cases for reading the value of output GPIOs are
> desribed.  Other aspects are mentioned in other spots.
> 
> For example, open drain signals -- as used with I2C and
> other protocols -- only drive the "low" signal level,
> and if code wants a "high" level it's got to verify that
> nobody else is driving that shared line to low.  By
> reading it back, even though it's configured as output.

I see. Much thanks for the explanations.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [patch] gpiolib: fix oops in gpio_get_value_cansleep()

[Andrew Morton]

On Thu, 16 Oct 2008 08:45:22 -0700
David Brownell <david-b@pacbell.net> wrote:

> From: David Brownell <dbrownell@users.sourceforge.net>
> 
> We can get the following oops from gpio_get_value_cansleep()
> when a GPIO controller doesn't provide a get() callback:

We can, but do we? ;)

iow: is this needed in any -stable release?

>  Unable to handle kernel paging request for instruction fetch
>  Faulting instruction address: 0x00000000
>  Oops: Kernel access of bad area, sig: 11 [#1]
>  [...]
>  NIP [00000000] 0x0
>  LR [c0182fb0] gpio_get_value_cansleep+0x40/0x50
>  Call Trace:
>  [c7b79e80] [c0183f28] gpio_value_show+0x5c/0x94
>  [c7b79ea0] [c01a584c] dev_attr_show+0x30/0x7c
>  [c7b79eb0] [c00d6b48] fill_read_buffer+0x68/0xe0
>  [c7b79ed0] [c00d6c54] sysfs_read_file+0x94/0xbc
>  [c7b79ef0] [c008f24c] vfs_read+0xb4/0x16c
>  [c7b79f10] [c008f580] sys_read+0x4c/0x90
>  [c7b79f40] [c0013a14] ret_from_syscall+0x0/0x38
> 
> It's OK to request the value of *any* GPIO; most GPIOs are
> bidirectional, so configuring them as outputs just enables an
> output driver and doesn't disable the input logic.
> 
> So the problem is that gpio_get_value_cansleep() isn't making
> the same sanity check that gpio_get_value() does:  making sure
> this GPIO isn't one of the atypical "no input logic" cases.
> 
> Reported-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> ---
>  drivers/gpio/gpiolib.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1056,7 +1056,7 @@ int gpio_get_value_cansleep(unsigned gpi
>  
>  	might_sleep_if(extra_checks);
>  	chip = gpio_to_chip(gpio);
> -	return chip->get(chip, gpio - chip->base);
> +	return chip->get ? chip->get(chip, gpio - chip->base) : 0;
>  }
>  EXPORT_SYMBOL_GPL(gpio_get_value_cansleep);
>  

Re: [patch] gpiolib: fix oops in gpio_get_value_cansleep()

[David Brownell]

On Thursday 16 October 2008, Andrew Morton wrote:
> > From: David Brownell <dbrownell@users.sourceforge.net>
> > 
> > We can get the following oops from gpio_get_value_cansleep()
> > when a GPIO controller doesn't provide a get() callback:
> 
> We can, but do we? ;)

I think it's unlikely without the sysfs interface.


> iow: is this needed in any -stable release?

The bug has been there since 2.6.25 but nobody else seems
to have reported it.  Is the general policy to fix all
oopses that *could* appear?  I'd send it for 2.6.27-stable,
since that's got the sysfs hooks.  And older kernels if
bug likelihood isn't a major concern.

- Dave
 

Re: [patch] gpiolib: fix oops in gpio_get_value_cansleep()

[Andrew Morton]

On Thu, 16 Oct 2008 17:44:33 -0700 David Brownell <david-b@pacbell.net> wrote:

> On Thursday 16 October 2008, Andrew Morton wrote:
> > > From: David Brownell <dbrownell@users.sourceforge.net>
> > > 
> > > We can get the following oops from gpio_get_value_cansleep()
> > > when a GPIO controller doesn't provide a get() callback:
> > 
> > We can, but do we? ;)
> 
> I think it's unlikely without the sysfs interface.
> 
> 
> > iow: is this needed in any -stable release?
> 
> The bug has been there since 2.6.25 but nobody else seems
> to have reported it.  Is the general policy to fix all
> oopses that *could* appear?  I'd send it for 2.6.27-stable,
> since that's got the sysfs hooks.  And older kernels if
> bug likelihood isn't a major concern.

OK.  2.6.27 definitely (major distros are basing on that).

As for earlier kernels: I'd say so.  An oops is farily serious. 
Although an oops in a sysfs handler tends to be fairly tame, as the
code usually doesn't hold locks or many allocated resources.

Anyway - making decisions like this is why we pay stable@kernel.org the
big bucks :)

Re: [stable] [patch] gpiolib: fix oops in gpio_get_value_cansleep()

[Greg KH]

On Thu, Oct 16, 2008 at 05:54:49PM -0700, Andrew Morton wrote:
> On Thu, 16 Oct 2008 17:44:33 -0700 David Brownell <david-b@pacbell.net> wrote:
> 
> > On Thursday 16 October 2008, Andrew Morton wrote:
> > > > From: David Brownell <dbrownell@users.sourceforge.net>
> > > > 
> > > > We can get the following oops from gpio_get_value_cansleep()
> > > > when a GPIO controller doesn't provide a get() callback:
> > > 
> > > We can, but do we? ;)
> > 
> > I think it's unlikely without the sysfs interface.
> > 
> > 
> > > iow: is this needed in any -stable release?
> > 
> > The bug has been there since 2.6.25 but nobody else seems
> > to have reported it.  Is the general policy to fix all
> > oopses that *could* appear?  I'd send it for 2.6.27-stable,
> > since that's got the sysfs hooks.  And older kernels if
> > bug likelihood isn't a major concern.
> 
> OK.  2.6.27 definitely (major distros are basing on that).
> 
> As for earlier kernels: I'd say so.  An oops is farily serious. 
> Although an oops in a sysfs handler tends to be fairly tame, as the
> code usually doesn't hold locks or many allocated resources.
> 
> Anyway - making decisions like this is why we pay stable@kernel.org the
> big bucks :)

"Pay"?  We get paid for this in something becides a full inbox?

I'm not holding my breath :)

Send us the git commit id when it's in Linus's tree and we'll see what
is needed to backport this or not.

thanks,

greg k-h

Re: [stable] [patch] gpiolib: fix oops in gpio_get_value_cansleep()

[David Brownell]

On Thursday 16 October 2008, Greg KH wrote:
> "Pay"?  We get paid for this in something becides a full inbox?

Yours is just "full"??  Lucky you!  Mine's overflowing.

Bits are falling on the floor, running out the end of cat5
cables, radiating into the 802.11g aether, cluttering up
magnetic media, and worse ... ;)


> I'm not holding my breath :)