[PATCH] Avoid sysfs oops when an rc_dev's raw device is absent

[PATCH] Avoid sysfs oops when an rc_dev's raw device is absent

[Douglas Bagnall]

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.
---
 drivers/media/rc/rc-main.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 6e16b09..58789c9 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -775,10 +775,12 @@ 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 -EINVAL;
 
 	IR_dprintk(1, "allowed - 0x%llx, enabled - 0x%llx\n",
 		   (long long)allowed,
-- 
1.7.9.5

Re: [PATCH] Avoid sysfs oops when an rc_dev's raw device is absent

[Douglas Bagnall]

hi,

I probably should have sent that in reply to 
http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49740
which is the problem it fixes.

Some things which might be of interest:

1. I innocently followed the instructions on
   http://www.linuxtv.org/wiki/index.php/Maintaining_Git_trees (i.e.,
   use v4l-dvb tree on top of linus tree) and spent a while looking at
   IR/ir-sysfs.c instead of rc/rc-main.c. How stable it seemed! no
   patches in years! So I added a warning at the top of the wiki page,
   though a fix from someone who knows would be preferable.

2. From the above, I ended up reading a lot of ancient history and saw
   that this was inadvertently sort of fixed for a few weeks in 2010
   between a08c7c68f702e2a2797a4035b and d8b4b5822f51e2142b731b42.

3. I wrote:

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

Would, in rc_unregister_device(), putting device_del(&dev->dev) before
ir_raw_event_unregister(dev) help? I've only been a kernel hacker for
two hours so I am honestly clueless, but it seems like that might
avert the race by hiding the structure from sysfs before it is pulled
apart.

regards,

Douglas

Re: [PATCH] Avoid sysfs oops when an rc_dev's raw device is absent

[Mauro Carvalho Chehab]

Em 24-06-2012 21:00, Douglas Bagnall escreveu:
> 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.

Please add your Signed-off-by: as described at:
	http://www.linuxtv.org/wiki/index.php/Development:_Submitting_Patches

> ---
>   drivers/media/rc/rc-main.c |    4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index 6e16b09..58789c9 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -775,10 +775,12 @@ 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 -EINVAL;

The return code there should be -ENODEV, as the device got removed.

Regards,
Mauro

Re: [PATCH] Avoid sysfs oops when an rc_dev's raw device is absent

[Mauro Carvalho Chehab]

Em 24-06-2012 22:44, Douglas Bagnall escreveu:
> hi,
> 
> I probably should have sent that in reply to
> http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49740
> which is the problem it fixes.
> 
> Some things which might be of interest:
> 
> 1. I innocently followed the instructions on
>     http://www.linuxtv.org/wiki/index.php/Maintaining_Git_trees (i.e.,
>     use v4l-dvb tree on top of linus tree) and spent a while looking at
>     IR/ir-sysfs.c instead of rc/rc-main.c. How stable it seemed! no
>     patches in years! So I added a warning at the top of the wiki page,
>     though a fix from someone who knows would be preferable.

Please help us updating the wiki pages. Unfortunately, almost all developers 
have no time to update wiki pages, and expect community help on that.

> 
> 2. From the above, I ended up reading a lot of ancient history and saw
>     that this was inadvertently sort of fixed for a few weeks in 2010
>     between a08c7c68f702e2a2797a4035b and d8b4b5822f51e2142b731b42.
> 
> 3. I wrote:
> 
>> This patch avoids the NULL dereference, and ignores the issue of how
>> this state of affairs came about in the first place.
> 
> Would, in rc_unregister_device(), putting device_del(&dev->dev) before
> ir_raw_event_unregister(dev) help? I've only been a kernel hacker for
> two hours so I am honestly clueless, but it seems like that might
> avert the race by hiding the structure from sysfs before it is pulled
> apart.

Reverting the order might help on your specific case, but device_del() 
does more than just removing the driver: it also can free some used
structures, with might lead into an OOPS, if the driver is polling for
IR.

So, your patch is likely less risky to cause side effects.

Regards,
Mauro

Re: [PATCH v2] Avoid sysfs oops when an rc_dev's raw device is absent

[Douglas Bagnall]

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>
---
 drivers/media/rc/rc-main.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 6e16b09..9880926 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -775,10 +775,12 @@ 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,