Re: [PATCH 2/6] rc-core: cleanup rc_register_device

[David Härdeman]

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?

-- 
David Härdeman

Re: [PATCH 2/6] rc-core: cleanup rc_register_device

[Sean Young]

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

Re: [PATCH 2/6] rc-core: cleanup rc_register_device

[David Härdeman]

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 :/

-- 
David Härdeman

Re: [PATCH 2/6] rc-core: cleanup rc_register_device

[David Härdeman]

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.

-- 
David Härdeman

Re: [PATCH 2/6] rc-core: cleanup rc_register_device

[Sean Young]

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 
> ---
>  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(_raw_handler_lock);
> @@ -524,11 +533,15 @@ int ir_raw_event_register(struct rc_dev *dev)
>   mutex_unlock(_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(_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(>rc_map);
> +
> + return rc;
> +}
> +
> +static int rc_setup_rx_device(struct rc_dev *dev)
> +{
> + int rc;
> +
>   /* rc_open will be