Patchwork [SRU,Oneiric/Precise,Quantal,media] Avoid sysfs oops when an rc_dev's raw device is absent

login
register
mail settings
Submitter Herton Ronaldo Krzesinski
Date Aug. 6, 2012, 5:06 p.m.
Message ID <1344272808-7953-1-git-send-email-herton.krzesinski@canonical.com>
Download mbox | patch
Permalink /patch/175401/
State New
Headers show

Comments

Herton Ronaldo Krzesinski - Aug. 6, 2012, 5:06 p.m.
From: Douglas Bagnall <douglas@paradise.net.nz>

For some reason, when the lirc daemon learns that a usb remote control
has been unplugged, it wants to read the sysfs attributes of the
disappearing device. This is useful for uncovering transient
inconsistencies, but less so for keeping the system running when such
inconsistencies exist.

Under some circumstances (like every time I unplug my dvb stick from
my laptop), lirc catches an rc_dev whose raw event handler has been
removed (presumably by ir_raw_event_unregister), and proceeds to
interrogate the raw protocols supported by the NULL pointer.

This patch avoids the NULL dereference, and ignores the issue of how
this state of affairs came about in the first place.

Version 2 incorporates changes recommended by Mauro Carvalho Chehab
(-ENODEV instead of -EINVAL, and a signed-off-by).

Signed-off-by: Douglas Bagnall <douglas@paradise.net.nz>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
(cherry picked from commit 720bb6436ff30fccad05cf5bdf961ea5b1f5686d upstream)
BugLink: http://bugs.launchpad.net/bugs/1015836
Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
---
 drivers/media/rc/rc-main.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Bug was reported against Precise, but looking at the code Oneiric seems
affected as well. Natty would be affected too, but seems its code have
more problems (for example, it misses commit 08aeb7c "rc: add locking to fix
register/show race"), and I'm not sure it's worth touching Natty at this
stage.

I did a quick verification against one of the oopses on the bug also, the
patch looks good (code patched will address crash matched with symbols from
linux-image-3.2.0-26-generic-dbgsym_3.2.0-26.41_amd64.ddeb). And seems a fix
that is worth for stable upstream too, I'll send it there as well.
Tim Gardner - Aug. 6, 2012, 6:31 p.m.

Leann Ogasawara - Aug. 6, 2012, 6:35 p.m.
Applied to Quantal master-next.

Ack for Oneiric and Precise SRU:

Acked-by: Leann Ogasawara <leann.ogasawara@canonical.com>

I assume you'll submit to upstream stable as well?

Thanks,
Leann

On 08/06/2012 10:06 AM, Herton Ronaldo Krzesinski wrote:
> From: Douglas Bagnall <douglas@paradise.net.nz>
>
> For some reason, when the lirc daemon learns that a usb remote control
> has been unplugged, it wants to read the sysfs attributes of the
> disappearing device. This is useful for uncovering transient
> inconsistencies, but less so for keeping the system running when such
> inconsistencies exist.
>
> Under some circumstances (like every time I unplug my dvb stick from
> my laptop), lirc catches an rc_dev whose raw event handler has been
> removed (presumably by ir_raw_event_unregister), and proceeds to
> interrogate the raw protocols supported by the NULL pointer.
>
> This patch avoids the NULL dereference, and ignores the issue of how
> this state of affairs came about in the first place.
>
> Version 2 incorporates changes recommended by Mauro Carvalho Chehab
> (-ENODEV instead of -EINVAL, and a signed-off-by).
>
> Signed-off-by: Douglas Bagnall <douglas@paradise.net.nz>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> (cherry picked from commit 720bb6436ff30fccad05cf5bdf961ea5b1f5686d upstream)
> BugLink: http://bugs.launchpad.net/bugs/1015836
> Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
> ---
>  drivers/media/rc/rc-main.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> Bug was reported against Precise, but looking at the code Oneiric seems
> affected as well. Natty would be affected too, but seems its code have
> more problems (for example, it misses commit 08aeb7c "rc: add locking to fix
> register/show race"), and I'm not sure it's worth touching Natty at this
> stage.
>
> I did a quick verification against one of the oopses on the bug also, the
> patch looks good (code patched will address crash matched with symbols from
> linux-image-3.2.0-26-generic-dbgsym_3.2.0-26.41_amd64.ddeb). And seems a fix
> that is worth for stable upstream too, I'll send it there as well.
>
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index 6e16b09..cabc19c 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -775,10 +775,11 @@ static ssize_t show_protocols(struct device *device,
>  	if (dev->driver_type == RC_DRIVER_SCANCODE) {
>  		enabled = dev->rc_map.rc_type;
>  		allowed = dev->allowed_protos;
> -	} else {
> +	} else if (dev->raw) {
>  		enabled = dev->raw->enabled_protocols;
>  		allowed = ir_raw_get_allowed_protocols();
> -	}
> +	} else
> +		return -ENODEV;
>  
>  	IR_dprintk(1, "allowed - 0x%llx, enabled - 0x%llx\n",
>  		   (long long)allowed,
Herton Ronaldo Krzesinski - Aug. 6, 2012, 6:58 p.m.
On Mon, Aug 06, 2012 at 11:35:45AM -0700, Leann Ogasawara wrote:
> Applied to Quantal master-next.
> 
> Ack for Oneiric and Precise SRU:
> 
> Acked-by: Leann Ogasawara <leann.ogasawara@canonical.com>
> 
> I assume you'll submit to upstream stable as well?

Yep, I asked for inclusion on stable mailing list.

> 
> Thanks,
> Leann
>
Tim Gardner - Aug. 6, 2012, 7:12 p.m.

Herton Ronaldo Krzesinski - Aug. 7, 2012, 1:47 p.m.
On Mon, Aug 06, 2012 at 02:06:48PM -0300, Herton Ronaldo Krzesinski wrote:
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index 6e16b09..cabc19c 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -775,10 +775,11 @@ static ssize_t show_protocols(struct device *device,
>  	if (dev->driver_type == RC_DRIVER_SCANCODE) {
>  		enabled = dev->rc_map.rc_type;
>  		allowed = dev->allowed_protos;
> -	} else {
> +	} else if (dev->raw) {
>  		enabled = dev->raw->enabled_protocols;
>  		allowed = ir_raw_get_allowed_protocols();
> -	}
> +	} else
> +		return -ENODEV;

This patch has an issue, thanks to Ben Hutchings that pointed it out.
It's missing an mutex_unlock on exit. I'll post shortly an patch to
address, or we can revert and later reapply with the fix that should get
into upstream.

>  
>  	IR_dprintk(1, "allowed - 0x%llx, enabled - 0x%llx\n",
>  		   (long long)allowed,
> -- 
> 1.7.9.5
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
>

Patch

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 6e16b09..cabc19c 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -775,10 +775,11 @@  static ssize_t show_protocols(struct device *device,
 	if (dev->driver_type == RC_DRIVER_SCANCODE) {
 		enabled = dev->rc_map.rc_type;
 		allowed = dev->allowed_protos;
-	} else {
+	} else if (dev->raw) {
 		enabled = dev->raw->enabled_protocols;
 		allowed = ir_raw_get_allowed_protocols();
-	}
+	} else
+		return -ENODEV;
 
 	IR_dprintk(1, "allowed - 0x%llx, enabled - 0x%llx\n",
 		   (long long)allowed,