| Message ID | 149332524306.32431.8964878481747905258.stgit@zeus.hardeman.nu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 27, 2017 at 10:34:03PM +0200, David Härdeman wrote: > The device core infrastructure is based on the presumption that > once a driver calls device_add(), it must be ready to accept > userspace interaction. > > This requires splitting rc_setup_rx_device() into two functions > and reorganizing rc_register_device() so that as much work > as possible is performed before calling device_add(). > With this patch applied, I'm no longer getting any scancodes from my rc devices. David, please can you test your patches before submitting. I have to go over them meticulously because I cannot assume you've tested them. Thanks Sean > Signed-off-by: David Härdeman <david@hardeman.nu> > --- > drivers/media/rc/rc-core-priv.h | 2 + > drivers/media/rc/rc-ir-raw.c | 34 ++++++++++++------ > drivers/media/rc/rc-main.c | 75 +++++++++++++++++++++++++-------------- > 3 files changed, 73 insertions(+), 38 deletions(-) > > diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h > index 0455b273c2fc..b3e7cac2c3ee 100644 > --- a/drivers/media/rc/rc-core-priv.h > +++ b/drivers/media/rc/rc-core-priv.h > @@ -263,7 +263,9 @@ int ir_raw_gen_pl(struct ir_raw_event **ev, unsigned int max, > * Routines from rc-raw.c to be used internally and by decoders > */ > u64 ir_raw_get_allowed_protocols(void); > +int ir_raw_event_prepare(struct rc_dev *dev); > int ir_raw_event_register(struct rc_dev *dev); > +void ir_raw_event_free(struct rc_dev *dev); > void ir_raw_event_unregister(struct rc_dev *dev); > int ir_raw_handler_register(struct ir_raw_handler *ir_raw_handler); > void ir_raw_handler_unregister(struct ir_raw_handler *ir_raw_handler); > diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c > index 90f66dc7c0d7..ae7785c4fbe7 100644 > --- a/drivers/media/rc/rc-ir-raw.c > +++ b/drivers/media/rc/rc-ir-raw.c > @@ -486,14 +486,18 @@ EXPORT_SYMBOL(ir_raw_encode_scancode); > /* > * Used to (un)register raw event clients > */ > -int ir_raw_event_register(struct rc_dev *dev) > +int ir_raw_event_prepare(struct rc_dev *dev) > { > - int rc; > - struct ir_raw_handler *handler; > + static bool raw_init; /* 'false' default value, raw decoders loaded? */ > > if (!dev) > return -EINVAL; > > + if (!raw_init) { > + request_module("ir-lirc-codec"); > + raw_init = true; > + } > + > dev->raw = kzalloc(sizeof(*dev->raw), GFP_KERNEL); > if (!dev->raw) > return -ENOMEM; > @@ -502,6 +506,13 @@ int ir_raw_event_register(struct rc_dev *dev) > dev->change_protocol = change_protocol; > INIT_KFIFO(dev->raw->kfifo); > > + return 0; > +} > + > +int ir_raw_event_register(struct rc_dev *dev) > +{ > + struct ir_raw_handler *handler; > + > /* > * raw transmitters do not need any event registration > * because the event is coming from userspace > @@ -510,10 +521,8 @@ int ir_raw_event_register(struct rc_dev *dev) > dev->raw->thread = kthread_run(ir_raw_event_thread, dev->raw, > "rc%u", dev->minor); > > - if (IS_ERR(dev->raw->thread)) { > - rc = PTR_ERR(dev->raw->thread); > - goto out; > - } > + if (IS_ERR(dev->raw->thread)) > + return PTR_ERR(dev->raw->thread); > } > > mutex_lock(&ir_raw_handler_lock); > @@ -524,11 +533,15 @@ int ir_raw_event_register(struct rc_dev *dev) > mutex_unlock(&ir_raw_handler_lock); > > return 0; > +} > + > +void ir_raw_event_free(struct rc_dev *dev) > +{ > + if (!dev) > + return; > > -out: > kfree(dev->raw); > dev->raw = NULL; > - return rc; > } > > void ir_raw_event_unregister(struct rc_dev *dev) > @@ -547,8 +560,7 @@ void ir_raw_event_unregister(struct rc_dev *dev) > handler->raw_unregister(dev); > mutex_unlock(&ir_raw_handler_lock); > > - kfree(dev->raw); > - dev->raw = NULL; > + ir_raw_event_free(dev); > } > > /* > diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c > index 802e559cc30e..44189366f232 100644 > --- a/drivers/media/rc/rc-main.c > +++ b/drivers/media/rc/rc-main.c > @@ -1663,7 +1663,7 @@ struct rc_dev *devm_rc_allocate_device(struct device *dev, > } > EXPORT_SYMBOL_GPL(devm_rc_allocate_device); > > -static int rc_setup_rx_device(struct rc_dev *dev) > +static int rc_prepare_rx_device(struct rc_dev *dev) > { > int rc; > struct rc_map *rc_map; > @@ -1708,10 +1708,22 @@ static int rc_setup_rx_device(struct rc_dev *dev) > dev->input_dev->phys = dev->input_phys; > dev->input_dev->name = dev->input_name; > > + return 0; > + > +out_table: > + ir_free_table(&dev->rc_map); > + > + return rc; > +} > + > +static int rc_setup_rx_device(struct rc_dev *dev) > +{ > + int rc; > + > /* rc_open will be called here */ > rc = input_register_device(dev->input_dev); > if (rc) > - goto out_table; > + return rc; > > /* > * Default delay of 250ms is too short for some protocols, especially > @@ -1729,27 +1741,23 @@ static int rc_setup_rx_device(struct rc_dev *dev) > dev->input_dev->rep[REP_PERIOD] = 125; > > return 0; > - > -out_table: > - ir_free_table(&dev->rc_map); > - > - return rc; > } > > static void rc_free_rx_device(struct rc_dev *dev) > { > - if (!dev || dev->driver_type == RC_DRIVER_IR_RAW_TX) > + if (!dev) > return; > > - ir_free_table(&dev->rc_map); > + if (dev->input_dev) { > + input_unregister_device(dev->input_dev); > + dev->input_dev = NULL; > + } > > - input_unregister_device(dev->input_dev); > - dev->input_dev = NULL; > + ir_free_table(&dev->rc_map); > } > > int rc_register_device(struct rc_dev *dev) > { > - static bool raw_init; /* 'false' default value, raw decoders loaded? */ > const char *path; > int attr = 0; > int minor; > @@ -1776,30 +1784,39 @@ int rc_register_device(struct rc_dev *dev) > dev->sysfs_groups[attr++] = &rc_dev_wakeup_filter_attr_grp; > dev->sysfs_groups[attr++] = NULL; > > + if (dev->driver_type != RC_DRIVER_IR_RAW_TX) { > + rc = rc_prepare_rx_device(dev); > + if (rc) > + goto out_minor; > + } > + > + if (dev->driver_type == RC_DRIVER_IR_RAW || > + dev->driver_type == RC_DRIVER_IR_RAW_TX) { > + rc = ir_raw_event_prepare(dev); > + if (rc < 0) > + goto out_rx_free; > + } > + > rc = device_add(&dev->dev); > if (rc) > - goto out_unlock; > + goto out_raw; > > path = kobject_get_path(&dev->dev.kobj, GFP_KERNEL); > dev_info(&dev->dev, "%s as %s\n", > dev->input_name ?: "Unspecified device", path ?: "N/A"); > kfree(path); > > + if (dev->driver_type != RC_DRIVER_IR_RAW_TX) { > + rc = rc_setup_rx_device(dev); > + if (rc) > + goto out_dev; > + } > + > if (dev->driver_type == RC_DRIVER_IR_RAW || > dev->driver_type == RC_DRIVER_IR_RAW_TX) { > - if (!raw_init) { > - request_module_nowait("ir-lirc-codec"); > - raw_init = true; > - } > rc = ir_raw_event_register(dev); > if (rc < 0) > - goto out_dev; > - } > - > - if (dev->driver_type != RC_DRIVER_IR_RAW_TX) { > - rc = rc_setup_rx_device(dev); > - if (rc) > - goto out_raw; > + goto out_rx; > } > > /* Allow the RC sysfs nodes to be accessible */ > @@ -1811,11 +1828,15 @@ int rc_register_device(struct rc_dev *dev) > > return 0; > > -out_raw: > - ir_raw_event_unregister(dev); > +out_rx: > + rc_free_rx_device(dev); > out_dev: > device_del(&dev->dev); > -out_unlock: > +out_raw: > + ir_raw_event_free(dev); > +out_rx_free: > + ir_free_table(&dev->rc_map); > +out_minor: > ida_simple_remove(&rc_ida, minor); > return rc; > }
On Mon, May 01, 2017 at 05:49:53PM +0100, Sean Young wrote: >On Thu, Apr 27, 2017 at 10:34:03PM +0200, David Härdeman wrote: >> The device core infrastructure is based on the presumption that >> once a driver calls device_add(), it must be ready to accept >> userspace interaction. >> >> This requires splitting rc_setup_rx_device() into two functions >> and reorganizing rc_register_device() so that as much work >> as possible is performed before calling device_add(). >> > >With this patch applied, I'm no longer getting any scancodes from >my rc devices. > >David, please can you test your patches before submitting. I have to go >over them meticulously because I cannot assume you've tested them. I did test this patch and I just redid the tests, both with rc-loopback and with a mceusb receiver. I'm seeing scancodes on the input device as well as pulse-space readings on the lirc device in both tests. I did the tests with only this patch applied and the lirc-use-after-free (v3). What hardware did you test with? Meanwhile, I'll try rebasing my patches to the latest version of the media-master git tree and test again.
On Mon, May 01, 2017 at 07:47:25PM +0200, David Härdeman wrote: >On Mon, May 01, 2017 at 05:49:53PM +0100, Sean Young wrote: >>On Thu, Apr 27, 2017 at 10:34:03PM +0200, David Härdeman wrote: >>> The device core infrastructure is based on the presumption that >>> once a driver calls device_add(), it must be ready to accept >>> userspace interaction. >>> >>> This requires splitting rc_setup_rx_device() into two functions >>> and reorganizing rc_register_device() so that as much work >>> as possible is performed before calling device_add(). >>> >> >>With this patch applied, I'm no longer getting any scancodes from >>my rc devices. >> >>David, please can you test your patches before submitting. I have to go >>over them meticulously because I cannot assume you've tested them. > >I did test this patch and I just redid the tests, both with rc-loopback >and with a mceusb receiver. I'm seeing scancodes on the input device as >well as pulse-space readings on the lirc device in both tests. > >I did the tests with only this patch applied and the lirc-use-after-free >(v3). What hardware did you test with? > >Meanwhile, I'll try rebasing my patches to the latest version of the >media-master git tree and test again. I rebased the patches onto media-master (commit 3622d3e77ecef090b5111e3c5423313f11711dfa) and tested again. I still can't reproduce the problems you're having :/
On Tue, May 02, 2017 at 08:53:02PM +0200, David Härdeman wrote: > On Mon, May 01, 2017 at 07:47:25PM +0200, David Härdeman wrote: > >On Mon, May 01, 2017 at 05:49:53PM +0100, Sean Young wrote: > >>On Thu, Apr 27, 2017 at 10:34:03PM +0200, David Härdeman wrote: > >>> The device core infrastructure is based on the presumption that > >>> once a driver calls device_add(), it must be ready to accept > >>> userspace interaction. > >>> > >>> This requires splitting rc_setup_rx_device() into two functions > >>> and reorganizing rc_register_device() so that as much work > >>> as possible is performed before calling device_add(). > >>> > >> > >>With this patch applied, I'm no longer getting any scancodes from > >>my rc devices. > >> > >>David, please can you test your patches before submitting. I have to go > >>over them meticulously because I cannot assume you've tested them. > > > >I did test this patch and I just redid the tests, both with rc-loopback > >and with a mceusb receiver. I'm seeing scancodes on the input device as > >well as pulse-space readings on the lirc device in both tests. > > > >I did the tests with only this patch applied and the lirc-use-after-free > >(v3). What hardware did you test with? > > > >Meanwhile, I'll try rebasing my patches to the latest version of the > >media-master git tree and test again. > > I rebased the patches onto media-master (commit > 3622d3e77ecef090b5111e3c5423313f11711dfa) and tested again. I still > can't reproduce the problems you're having :/ The protocol is not set properly. In rc_prepare_rx_device(), dev->change_protocol() is call if not null. At this point it still is null, since it will only be set up in ir_raw_event_prepare(), which is called afterwards. Presumably you have udev set up to execute ir-keytable, which sets the protocol up (again). There is another problem with your patches, you've introduced a race condition. When device_add() is called, the protocol is not set up yet. So anyone reading the protocols sysfs attribute early enough will get false information. Is it not possible to make sure that it is all setup correctly at the point of device_add()? Sean
On Tue, May 02, 2017 at 09:48:26PM +0100, Sean Young wrote: >On Tue, May 02, 2017 at 08:53:02PM +0200, David Härdeman wrote: >> On Mon, May 01, 2017 at 07:47:25PM +0200, David Härdeman wrote: >> >On Mon, May 01, 2017 at 05:49:53PM +0100, Sean Young wrote: >> >>On Thu, Apr 27, 2017 at 10:34:03PM +0200, David Härdeman wrote: >> >>> The device core infrastructure is based on the presumption that >> >>> once a driver calls device_add(), it must be ready to accept >> >>> userspace interaction. >> >>> >> >>> This requires splitting rc_setup_rx_device() into two functions >> >>> and reorganizing rc_register_device() so that as much work >> >>> as possible is performed before calling device_add(). >> >>> >> >> >> >>With this patch applied, I'm no longer getting any scancodes from >> >>my rc devices. >> >> >> >>David, please can you test your patches before submitting. I have to go >> >>over them meticulously because I cannot assume you've tested them. >> > >> >I did test this patch and I just redid the tests, both with rc-loopback >> >and with a mceusb receiver. I'm seeing scancodes on the input device as >> >well as pulse-space readings on the lirc device in both tests. >> > >> >I did the tests with only this patch applied and the lirc-use-after-free >> >(v3). What hardware did you test with? >> > >> >Meanwhile, I'll try rebasing my patches to the latest version of the >> >media-master git tree and test again. >> >> I rebased the patches onto media-master (commit >> 3622d3e77ecef090b5111e3c5423313f11711dfa) and tested again. I still >> can't reproduce the problems you're having :/ > >The protocol is not set properly. In rc_prepare_rx_device(), >dev->change_protocol() is call if not null. At this point it still is >null, since it will only be set up in ir_raw_event_prepare(), which >is called afterwards. Ah, good catch. Since ir_raw_event_prepare() only does a kmalloc() and sets the change_protocol pointer it should be fine to call ir_raw_event_prepare() before rc_prepare_rx_device(). I'll prepare a v2 of the patch. >Presumably you have udev set up to execute ir-keytable, which sets the >protocol up (again). Well, kind of. In the automated testing I use rc-loopback which has the "rc-empty" keytable so it doesn't set the protocols. In my manual testing I used mceusb with a NEC remote so I anyway needed to set the protocols manually and I missed the fact that "[rc-6]" was no longer set in sysfs. >There is another problem with your patches, you've introduced a race >condition. When device_add() is called, the protocol is not set up yet. >So anyone reading the protocols sysfs attribute early enough will get >false information. Is it not possible to make sure that it is all setup >correctly at the point of device_add()? Isn't this the same problem? If dev->change_protocol() isn't NULL when rc_prepare_rx_device() is called then the protocol will be set up (i.e. dev->enabled_protcols will be set to the right value). Or did I misunderstand you?
diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h index 0455b273c2fc..b3e7cac2c3ee 100644 --- a/drivers/media/rc/rc-core-priv.h +++ b/drivers/media/rc/rc-core-priv.h @@ -263,7 +263,9 @@ int ir_raw_gen_pl(struct ir_raw_event **ev, unsigned int max, * Routines from rc-raw.c to be used internally and by decoders */ u64 ir_raw_get_allowed_protocols(void); +int ir_raw_event_prepare(struct rc_dev *dev); int ir_raw_event_register(struct rc_dev *dev); +void ir_raw_event_free(struct rc_dev *dev); void ir_raw_event_unregister(struct rc_dev *dev); int ir_raw_handler_register(struct ir_raw_handler *ir_raw_handler); void ir_raw_handler_unregister(struct ir_raw_handler *ir_raw_handler); diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c index 90f66dc7c0d7..ae7785c4fbe7 100644 --- a/drivers/media/rc/rc-ir-raw.c +++ b/drivers/media/rc/rc-ir-raw.c @@ -486,14 +486,18 @@ EXPORT_SYMBOL(ir_raw_encode_scancode); /* * Used to (un)register raw event clients */ -int ir_raw_event_register(struct rc_dev *dev) +int ir_raw_event_prepare(struct rc_dev *dev) { - int rc; - struct ir_raw_handler *handler; + static bool raw_init; /* 'false' default value, raw decoders loaded? */ if (!dev) return -EINVAL; + if (!raw_init) { + request_module("ir-lirc-codec"); + raw_init = true; + } + dev->raw = kzalloc(sizeof(*dev->raw), GFP_KERNEL); if (!dev->raw) return -ENOMEM; @@ -502,6 +506,13 @@ int ir_raw_event_register(struct rc_dev *dev) dev->change_protocol = change_protocol; INIT_KFIFO(dev->raw->kfifo); + return 0; +} + +int ir_raw_event_register(struct rc_dev *dev) +{ + struct ir_raw_handler *handler; + /* * raw transmitters do not need any event registration * because the event is coming from userspace @@ -510,10 +521,8 @@ int ir_raw_event_register(struct rc_dev *dev) dev->raw->thread = kthread_run(ir_raw_event_thread, dev->raw, "rc%u", dev->minor); - if (IS_ERR(dev->raw->thread)) { - rc = PTR_ERR(dev->raw->thread); - goto out; - } + if (IS_ERR(dev->raw->thread)) + return PTR_ERR(dev->raw->thread); } mutex_lock(&ir_raw_handler_lock); @@ -524,11 +533,15 @@ int ir_raw_event_register(struct rc_dev *dev) mutex_unlock(&ir_raw_handler_lock); return 0; +} + +void ir_raw_event_free(struct rc_dev *dev) +{ + if (!dev) + return; -out: kfree(dev->raw); dev->raw = NULL; - return rc; } void ir_raw_event_unregister(struct rc_dev *dev) @@ -547,8 +560,7 @@ void ir_raw_event_unregister(struct rc_dev *dev) handler->raw_unregister(dev); mutex_unlock(&ir_raw_handler_lock); - kfree(dev->raw); - dev->raw = NULL; + ir_raw_event_free(dev); } /* diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c index 802e559cc30e..44189366f232 100644 --- a/drivers/media/rc/rc-main.c +++ b/drivers/media/rc/rc-main.c @@ -1663,7 +1663,7 @@ struct rc_dev *devm_rc_allocate_device(struct device *dev, } EXPORT_SYMBOL_GPL(devm_rc_allocate_device); -static int rc_setup_rx_device(struct rc_dev *dev) +static int rc_prepare_rx_device(struct rc_dev *dev) { int rc; struct rc_map *rc_map; @@ -1708,10 +1708,22 @@ static int rc_setup_rx_device(struct rc_dev *dev) dev->input_dev->phys = dev->input_phys; dev->input_dev->name = dev->input_name; + return 0; + +out_table: + ir_free_table(&dev->rc_map); + + return rc; +} + +static int rc_setup_rx_device(struct rc_dev *dev) +{ + int rc; + /* rc_open will be called here */ rc = input_register_device(dev->input_dev); if (rc) - goto out_table; + return rc; /* * Default delay of 250ms is too short for some protocols, especially @@ -1729,27 +1741,23 @@ static int rc_setup_rx_device(struct rc_dev *dev) dev->input_dev->rep[REP_PERIOD] = 125; return 0; - -out_table: - ir_free_table(&dev->rc_map); - - return rc; } static void rc_free_rx_device(struct rc_dev *dev) { - if (!dev || dev->driver_type == RC_DRIVER_IR_RAW_TX) + if (!dev) return; - ir_free_table(&dev->rc_map); + if (dev->input_dev) { + input_unregister_device(dev->input_dev); + dev->input_dev = NULL; + } - input_unregister_device(dev->input_dev); - dev->input_dev = NULL; + ir_free_table(&dev->rc_map); } int rc_register_device(struct rc_dev *dev) { - static bool raw_init; /* 'false' default value, raw decoders loaded? */ const char *path; int attr = 0; int minor; @@ -1776,30 +1784,39 @@ int rc_register_device(struct rc_dev *dev) dev->sysfs_groups[attr++] = &rc_dev_wakeup_filter_attr_grp; dev->sysfs_groups[attr++] = NULL; + if (dev->driver_type != RC_DRIVER_IR_RAW_TX) { + rc = rc_prepare_rx_device(dev); + if (rc) + goto out_minor; + } + + if (dev->driver_type == RC_DRIVER_IR_RAW || + dev->driver_type == RC_DRIVER_IR_RAW_TX) { + rc = ir_raw_event_prepare(dev); + if (rc < 0) + goto out_rx_free; + } + rc = device_add(&dev->dev); if (rc) - goto out_unlock; + goto out_raw; path = kobject_get_path(&dev->dev.kobj, GFP_KERNEL); dev_info(&dev->dev, "%s as %s\n", dev->input_name ?: "Unspecified device", path ?: "N/A"); kfree(path); + if (dev->driver_type != RC_DRIVER_IR_RAW_TX) { + rc = rc_setup_rx_device(dev); + if (rc) + goto out_dev; + } + if (dev->driver_type == RC_DRIVER_IR_RAW || dev->driver_type == RC_DRIVER_IR_RAW_TX) { - if (!raw_init) { - request_module_nowait("ir-lirc-codec"); - raw_init = true; - } rc = ir_raw_event_register(dev); if (rc < 0) - goto out_dev; - } - - if (dev->driver_type != RC_DRIVER_IR_RAW_TX) { - rc = rc_setup_rx_device(dev); - if (rc) - goto out_raw; + goto out_rx; } /* Allow the RC sysfs nodes to be accessible */ @@ -1811,11 +1828,15 @@ int rc_register_device(struct rc_dev *dev) return 0; -out_raw: - ir_raw_event_unregister(dev); +out_rx: + rc_free_rx_device(dev); out_dev: device_del(&dev->dev); -out_unlock: +out_raw: + ir_raw_event_free(dev); +out_rx_free: + ir_free_table(&dev->rc_map); +out_minor: ida_simple_remove(&rc_ida, minor); return rc; }
The device core infrastructure is based on the presumption that once a driver calls device_add(), it must be ready to accept userspace interaction. This requires splitting rc_setup_rx_device() into two functions and reorganizing rc_register_device() so that as much work as possible is performed before calling device_add(). Signed-off-by: David Härdeman <david@hardeman.nu> --- drivers/media/rc/rc-core-priv.h | 2 + drivers/media/rc/rc-ir-raw.c | 34 ++++++++++++------ drivers/media/rc/rc-main.c | 75 +++++++++++++++++++++++++-------------- 3 files changed, 73 insertions(+), 38 deletions(-)