[PATCH] led: triggers: Fix NULL dereference in led_trigger_set() error handling

[PATCH] led: triggers: Fix NULL dereference in led_trigger_set() error handling

[Dan Carpenter]

The problem is we set "led_cdev->trigger = NULL;" and then dereference
it when we call write_lock_irqsave():

	write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
                            ^^^^^^^^^^^^^^^^^

Fixes: 2282e125a406 ("leds: triggers: let struct led_trigger::activate() return an error code")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/leds/led-triggers.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index eff1bda8b520..13cea227277c 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -167,12 +167,12 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
 		trig->deactivate(led_cdev);
 err_activate:
 
-	led_cdev->trigger = NULL;
-	led_cdev->trigger_data = NULL;
 	write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
 	list_del(&led_cdev->trig_list);
 	write_unlock_irqrestore(&led_cdev->trigger->leddev_list_lock, flags);
+	led_cdev->trigger = NULL;
+	led_cdev->trigger_data = NULL;
 	led_set_brightness(led_cdev, LED_OFF);
 	kfree(event);
 
 	return ret;
-- 
2.20.1

[PATCH] led: triggers: Fix NULL dereference in led_trigger_set() error handling

[Dan Carpenter]

The problem is we set "led_cdev->trigger = NULL;" and then dereference
it when we call write_lock_irqsave():

	write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
                            ^^^^^^^^^^^^^^^^^

Fixes: 2282e125a406 ("leds: triggers: let struct led_trigger::activate() return an error code")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/leds/led-triggers.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index eff1bda8b520..13cea227277c 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -167,12 +167,12 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
 		trig->deactivate(led_cdev);
 err_activate:
 
-	led_cdev->trigger = NULL;
-	led_cdev->trigger_data = NULL;
 	write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
 	list_del(&led_cdev->trig_list);
 	write_unlock_irqrestore(&led_cdev->trigger->leddev_list_lock, flags);
+	led_cdev->trigger = NULL;
+	led_cdev->trigger_data = NULL;
 	led_set_brightness(led_cdev, LED_OFF);
 	kfree(event);
 
 	return ret;
-- 
2.20.1

Re: [PATCH] led: triggers: Fix NULL dereference in led_trigger_set() error handling

[Oleh Kravchenko]

Hello Dan,
If you amend my patch, please keep my Signed-of-by.

> 5 вер. 2019 р. о 12:57 пп Dan Carpenter <dan.carpenter@oracle.com> написав(ла):
> 
> The problem is we set "led_cdev->trigger = NULL;" and then dereference
> it when we call write_lock_irqsave():
> 
> 	write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
>                            ^^^^^^^^^^^^^^^^^
> 
> Fixes: 2282e125a406 ("leds: triggers: let struct led_trigger::activate() return an error code")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> drivers/leds/led-triggers.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index eff1bda8b520..13cea227277c 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -167,12 +167,12 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
> 		trig->deactivate(led_cdev);
> err_activate:
> 
> -	led_cdev->trigger = NULL;
> -	led_cdev->trigger_data = NULL;
> 	write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
> 	list_del(&led_cdev->trig_list);
> 	write_unlock_irqrestore(&led_cdev->trigger->leddev_list_lock, flags);
> +	led_cdev->trigger = NULL;
> +	led_cdev->trigger_data = NULL;
> 	led_set_brightness(led_cdev, LED_OFF);
> 	kfree(event);
> 
> 	return ret;
> -- 
> 2.20.1
> 

Re: [PATCH] led: triggers: Fix NULL dereference in led_trigger_set() error handling

[Oleh Kravchenko]

Hello Dan,
If you amend my patch, please keep my Signed-of-by.

> 5 вер. 2019 р. о 12:57 пп Dan Carpenter <dan.carpenter@oracle.com> написав(ла):
> 
> The problem is we set "led_cdev->trigger = NULL;" and then dereference
> it when we call write_lock_irqsave():
> 
> 	write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
>                            ^^^^^^^^^^^^^^^^^
> 
> Fixes: 2282e125a406 ("leds: triggers: let struct led_trigger::activate() return an error code")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> drivers/leds/led-triggers.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index eff1bda8b520..13cea227277c 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -167,12 +167,12 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
> 		trig->deactivate(led_cdev);
> err_activate:
> 
> -	led_cdev->trigger = NULL;
> -	led_cdev->trigger_data = NULL;
> 	write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
> 	list_del(&led_cdev->trig_list);
> 	write_unlock_irqrestore(&led_cdev->trigger->leddev_list_lock, flags);
> +	led_cdev->trigger = NULL;
> +	led_cdev->trigger_data = NULL;
> 	led_set_brightness(led_cdev, LED_OFF);
> 	kfree(event);
> 
> 	return ret;
> -- 
> 2.20.1
> 

Re: [PATCH] led: triggers: Fix NULL dereference in led_trigger_set() error handling

[Uwe Kleine-König]

Hello,

On Thu, Sep 05, 2019 at 12:57:28PM +0300, Dan Carpenter wrote:
> The problem is we set "led_cdev->trigger = NULL;" and then dereference
> it when we call write_lock_irqsave():
> 
> 	write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
>                             ^^^^^^^^^^^^^^^^^
> 
> Fixes: 2282e125a406 ("leds: triggers: let struct led_trigger::activate() return an error code")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Obviously right. Thanks for catching.

Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Did you find this at runtime or by using some static checker?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] led: triggers: Fix NULL dereference in led_trigger_set() error handling

[Uwe Kleine-König]

Hello,

On Thu, Sep 05, 2019 at 12:57:28PM +0300, Dan Carpenter wrote:
> The problem is we set "led_cdev->trigger = NULL;" and then dereference
> it when we call write_lock_irqsave():
> 
> 	write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
>                             ^^^^^^^^^^^^^^^^^
> 
> Fixes: 2282e125a406 ("leds: triggers: let struct led_trigger::activate() return an error code")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Obviously right. Thanks for catching.

Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Did you find this at runtime or by using some static checker?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] led: triggers: Fix NULL dereference in led_trigger_set() error handling

[Oleh Kravchenko]

Hello Jacek,

> 5 вер. 2019 р. о 3:06 пп Uwe Kleine-König <u.kleine-koenig@pengutronix.de> написав(ла):
> 
> Hello,
> 
> On Thu, Sep 05, 2019 at 12:57:28PM +0300, Dan Carpenter wrote:
>> The problem is we set "led_cdev->trigger = NULL;" and then dereference
>> it when we call write_lock_irqsave():
>> 
>> 	write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
>>                            ^^^^^^^^^^^^^^^^^
>> 
>> Fixes: 2282e125a406 ("leds: triggers: let struct led_trigger::activate() return an error code")
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> Obviously right. Thanks for catching.
> 
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Did you find this at runtime or by using some static checker?

Let me summarize the chronology of the last activities below:
1. I have sent the patch for the bugs that I have found by static analyzer at PVS-Studio
    Date: Wed, 4 Sep 2019 00:18:19 +0300
    https://www.spinics.net/lists/linux-leds/msg13181.html

2. At 5 Sep 2019 12:57:19 +0300 Time Dan Cartpen has sent the patch with the same proposal
3. Uwe Kleine-König started to discuss his results of review by asking Dan on how he was found it.

Would you mine if you will keep me as a Original author of this patch based on fact 1?

Best regards,
Oleh Kravchenko

> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] led: triggers: Fix NULL dereference in led_trigger_set() error handling

[Oleh Kravchenko]

Hello Jacek,

> 5 вер. 2019 р. о 3:06 пп Uwe Kleine-König <u.kleine-koenig@pengutronix.de> написав(ла):
> 
> Hello,
> 
> On Thu, Sep 05, 2019 at 12:57:28PM +0300, Dan Carpenter wrote:
>> The problem is we set "led_cdev->trigger = NULL;" and then dereference
>> it when we call write_lock_irqsave():
>> 
>> 	write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
>>                            ^^^^^^^^^^^^^^^^^
>> 
>> Fixes: 2282e125a406 ("leds: triggers: let struct led_trigger::activate() return an error code")
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> Obviously right. Thanks for catching.
> 
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Did you find this at runtime or by using some static checker?

Let me summarize the chronology of the last activities below:
1. I have sent the patch for the bugs that I have found by static analyzer at PVS-Studio
    Date: Wed, 4 Sep 2019 00:18:19 +0300
    https://www.spinics.net/lists/linux-leds/msg13181.html

2. At 5 Sep 2019 12:57:19 +0300 Time Dan Cartpen has sent the patch with the same proposal
3. Uwe Kleine-König started to discuss his results of review by asking Dan on how he was found it.

Would you mine if you will keep me as a Original author of this patch based on fact 1?

Best regards,
Oleh Kravchenko

> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] led: triggers: Fix NULL dereference in led_trigger_set() error handling

[Oleh Kravchenko]

Hello Jacek,

> 5 вер. 2019 р. о 3:06 пп Uwe Kleine-König <u.kleine-koenig@pengutronix.de> написав(ла):
> 
> Hello,
> 
> On Thu, Sep 05, 2019 at 12:57:28PM +0300, Dan Carpenter wrote:
>> The problem is we set "led_cdev->trigger = NULL;" and then dereference
>> it when we call write_lock_irqsave():
>> 
>> 	write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
>>                            ^^^^^^^^^^^^^^^^^
>> 
>> Fixes: 2282e125a406 ("leds: triggers: let struct led_trigger::activate() return an error code")
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> Obviously right. Thanks for catching.
> 
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Did you find this at runtime or by using some static checker?

Let me summarize the chronology of the last activities below:
1. I have sent the patch for the bugs that I have found by static
analyzer at PVS-Studio
    Date: Wed, 4 Sep 2019 00:18:19 +0300
    https://www.spinics.net/lists/linux-leds/msg13181.html

2. At 5 Sep 2019 12:57:19 +0300 Time Dan Cartpen has sent the patch with
the same proposal

3. Uwe Kleine-König started to discuss his results of review by asking
Dan on how he was found it.

Would you mine if you will keep me as a Original author of this patch
based on fact 1?

Best regards,
Oleh Kravchenko

> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] led: triggers: Fix NULL dereference in led_trigger_set() error handling

[Oleh Kravchenko]

Hello Jacek,

> 5 вер. 2019 р. о 3:06 пп Uwe Kleine-König <u.kleine-koenig@pengutronix.de> написав(ла):
> 
> Hello,
> 
> On Thu, Sep 05, 2019 at 12:57:28PM +0300, Dan Carpenter wrote:
>> The problem is we set "led_cdev->trigger = NULL;" and then dereference
>> it when we call write_lock_irqsave():
>> 
>> 	write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
>>                            ^^^^^^^^^^^^^^^^^
>> 
>> Fixes: 2282e125a406 ("leds: triggers: let struct led_trigger::activate() return an error code")
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> Obviously right. Thanks for catching.
> 
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Did you find this at runtime or by using some static checker?

Let me summarize the chronology of the last activities below:
1. I have sent the patch for the bugs that I have found by static
analyzer at PVS-Studio
    Date: Wed, 4 Sep 2019 00:18:19 +0300
    https://www.spinics.net/lists/linux-leds/msg13181.html

2. At 5 Sep 2019 12:57:19 +0300 Time Dan Cartpen has sent the patch with
the same proposal

3. Uwe Kleine-König started to discuss his results of review by asking
Dan on how he was found it.

Would you mine if you will keep me as a Original author of this patch
based on fact 1?

Best regards,
Oleh Kravchenko

> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] led: triggers: Fix NULL dereference in led_trigger_set() error handling

[Uwe Kleine-König]

On Thu, Sep 05, 2019 at 03:23:21PM +0300, Oleh Kravchenko wrote:
> Hello Jacek,
> 
> > 5 вер. 2019 р. о 3:06 пп Uwe Kleine-König <u.kleine-koenig@pengutronix.de> написав(ла):
> > 
> > Hello,
> > 
> > On Thu, Sep 05, 2019 at 12:57:28PM +0300, Dan Carpenter wrote:
> >> The problem is we set "led_cdev->trigger = NULL;" and then dereference
> >> it when we call write_lock_irqsave():
> >> 
> >> 	write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
> >>                            ^^^^^^^^^^^^^^^^^
> >> 
> >> Fixes: 2282e125a406 ("leds: triggers: let struct led_trigger::activate() return an error code")
> >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > Obviously right. Thanks for catching.
> > 
> > Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > 
> > Did you find this at runtime or by using some static checker?
> 
> Let me summarize the chronology of the last activities below:
> 1. I have sent the patch for the bugs that I have found by static analyzer at PVS-Studio
>     Date: Wed, 4 Sep 2019 00:18:19 +0300
>     https://www.spinics.net/lists/linux-leds/msg13181.html
> 
> 2. At 5 Sep 2019 12:57:19 +0300 Time Dan Cartpen has sent the patch with the same proposal
> 3. Uwe Kleine-König started to discuss his results of review by asking Dan on how he was found it.
> 
> Would you mine if you will keep me as a Original author of this patch based on fact 1?

I don't care much personally but it seems fair to take Oleh's version. I
didn't see Oleh's patch before as only Dan's was Cc:d to me.

Feel free to add my Reviewed-by also to Oleh's patch of course.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] led: triggers: Fix NULL dereference in led_trigger_set() error handling

[Uwe Kleine-König]

On Thu, Sep 05, 2019 at 03:23:21PM +0300, Oleh Kravchenko wrote:
> Hello Jacek,
> 
> > 5 вер. 2019 р. о 3:06 пп Uwe Kleine-König <u.kleine-koenig@pengutronix.de> написав(ла):
> > 
> > Hello,
> > 
> > On Thu, Sep 05, 2019 at 12:57:28PM +0300, Dan Carpenter wrote:
> >> The problem is we set "led_cdev->trigger = NULL;" and then dereference
> >> it when we call write_lock_irqsave():
> >> 
> >> 	write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
> >>                            ^^^^^^^^^^^^^^^^^
> >> 
> >> Fixes: 2282e125a406 ("leds: triggers: let struct led_trigger::activate() return an error code")
> >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > Obviously right. Thanks for catching.
> > 
> > Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > 
> > Did you find this at runtime or by using some static checker?
> 
> Let me summarize the chronology of the last activities below:
> 1. I have sent the patch for the bugs that I have found by static analyzer at PVS-Studio
>     Date: Wed, 4 Sep 2019 00:18:19 +0300
>     https://www.spinics.net/lists/linux-leds/msg13181.html
> 
> 2. At 5 Sep 2019 12:57:19 +0300 Time Dan Cartpen has sent the patch with the same proposal
> 3. Uwe Kleine-König started to discuss his results of review by asking Dan on how he was found it.
> 
> Would you mine if you will keep me as a Original author of this patch based on fact 1?

I don't care much personally but it seems fair to take Oleh's version. I
didn't see Oleh's patch before as only Dan's was Cc:d to me.

Feel free to add my Reviewed-by also to Oleh's patch of course.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] led: triggers: Fix NULL dereference in led_trigger_set() error handling

[Dan Carpenter]

On Thu, Sep 05, 2019 at 02:06:26PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Thu, Sep 05, 2019 at 12:57:28PM +0300, Dan Carpenter wrote:
> > The problem is we set "led_cdev->trigger = NULL;" and then dereference
> > it when we call write_lock_irqsave():
> > 
> > 	write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
> >                             ^^^^^^^^^^^^^^^^^
> > 
> > Fixes: 2282e125a406 ("leds: triggers: let struct led_trigger::activate() return an error code")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> Obviously right. Thanks for catching.
> 
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Did you find this at runtime or by using some static checker?

Yes.  It's a new one that I'm working on.

It's a tricky thing because it turns out Smatch thinks a whole lot of
pointers are definitely NULL when they aren't.  For example, if the
struct is allocated with kzalloc() and Smatch doesn't see where the
pointer is assigned.

regards,
dan carpenter

Re: [PATCH] led: triggers: Fix NULL dereference in led_trigger_set() error handling

[Dan Carpenter]

On Thu, Sep 05, 2019 at 02:06:26PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Thu, Sep 05, 2019 at 12:57:28PM +0300, Dan Carpenter wrote:
> > The problem is we set "led_cdev->trigger = NULL;" and then dereference
> > it when we call write_lock_irqsave():
> > 
> > 	write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
> >                             ^^^^^^^^^^^^^^^^^
> > 
> > Fixes: 2282e125a406 ("leds: triggers: let struct led_trigger::activate() return an error code")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> Obviously right. Thanks for catching.
> 
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Did you find this at runtime or by using some static checker?

Yes.  It's a new one that I'm working on.

It's a tricky thing because it turns out Smatch thinks a whole lot of
pointers are definitely NULL when they aren't.  For example, if the
struct is allocated with kzalloc() and Smatch doesn't see where the
pointer is assigned.

regards,
dan carpenter

Re: [PATCH] led: triggers: Fix NULL dereference in led_trigger_set() error handling

[Dan Carpenter]

On Thu, Sep 05, 2019 at 03:23:21PM +0300, Oleh Kravchenko wrote:
> Let me summarize the chronology of the last activities below:
> 1. I have sent the patch for the bugs that I have found by static analyzer at PVS-Studio
>     Date: Wed, 4 Sep 2019 00:18:19 +0300
>     https://www.spinics.net/lists/linux-leds/msg13181.html
> 
> 2. At 5 Sep 2019 12:57:19 +0300 Time Dan Cartpen has sent the patch with the same proposal
> 3. Uwe Kleine-König started to discuss his results of review by asking Dan on how he was found it.
> 
> Would you mine if you will keep me as a Original author of this patch based on fact 1?

Heh.

No, I didn't steal your patch.  :P  I am the author of the Smatch static
analysis tool and mostly fix things found by Smatch.  I don't use other
static analysis tools except to do a final QC of my patches.

It's super common for people to send duplicate fixes when it's based on
static analysis.  Most of the static analysis people hang out on
kernel-janitors so we don't send duplicate patches.  For a while people
were getting annoyed by all the duplicates but now they accept it as
their punishment for introducing a bug in the first place.

Anyway, the rule for kernel development is that normally the first
person's patch goes in, so we will take your patch.

regards,
dan carpenter

Re: [PATCH] led: triggers: Fix NULL dereference in led_trigger_set() error handling

[Dan Carpenter]

On Thu, Sep 05, 2019 at 03:23:21PM +0300, Oleh Kravchenko wrote:
> Let me summarize the chronology of the last activities below:
> 1. I have sent the patch for the bugs that I have found by static analyzer at PVS-Studio
>     Date: Wed, 4 Sep 2019 00:18:19 +0300
>     https://www.spinics.net/lists/linux-leds/msg13181.html
> 
> 2. At 5 Sep 2019 12:57:19 +0300 Time Dan Cartpen has sent the patch with the same proposal
> 3. Uwe Kleine-König started to discuss his results of review by asking Dan on how he was found it.
> 
> Would you mine if you will keep me as a Original author of this patch based on fact 1?

Heh.

No, I didn't steal your patch.  :P  I am the author of the Smatch static
analysis tool and mostly fix things found by Smatch.  I don't use other
static analysis tools except to do a final QC of my patches.

It's super common for people to send duplicate fixes when it's based on
static analysis.  Most of the static analysis people hang out on
kernel-janitors so we don't send duplicate patches.  For a while people
were getting annoyed by all the duplicates but now they accept it as
their punishment for introducing a bug in the first place.

Anyway, the rule for kernel development is that normally the first
person's patch goes in, so we will take your patch.

regards,
dan carpenter

Re: [PATCH] led: triggers: Fix NULL dereference in led_trigger_set() error handling

[Jacek Anaszewski]

On 9/5/19 2:46 PM, Uwe Kleine-König wrote:
> On Thu, Sep 05, 2019 at 03:23:21PM +0300, Oleh Kravchenko wrote:
>> Hello Jacek,
>>
>>> 5 вер. 2019 р. о 3:06 пп Uwe Kleine-König <u.kleine-koenig@pengutronix.de> написав(ла):
>>>
>>> Hello,
>>>
>>> On Thu, Sep 05, 2019 at 12:57:28PM +0300, Dan Carpenter wrote:
>>>> The problem is we set "led_cdev->trigger = NULL;" and then dereference
>>>> it when we call write_lock_irqsave():
>>>>
>>>> 	write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
>>>>                            ^^^^^^^^^^^^^^^^^
>>>>
>>>> Fixes: 2282e125a406 ("leds: triggers: let struct led_trigger::activate() return an error code")
>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>
>>> Obviously right. Thanks for catching.
>>>
>>> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>>
>>> Did you find this at runtime or by using some static checker?
>>
>> Let me summarize the chronology of the last activities below:
>> 1. I have sent the patch for the bugs that I have found by static analyzer at PVS-Studio
>>     Date: Wed, 4 Sep 2019 00:18:19 +0300
>>     https://www.spinics.net/lists/linux-leds/msg13181.html
>>
>> 2. At 5 Sep 2019 12:57:19 +0300 Time Dan Cartpen has sent the patch with the same proposal
>> 3. Uwe Kleine-König started to discuss his results of review by asking Dan on how he was found it.
>>
>> Would you mine if you will keep me as a Original author of this patch based on fact 1?
> 
> I don't care much personally but it seems fair to take Oleh's version. I
> didn't see Oleh's patch before as only Dan's was Cc:d to me.
> 
> Feel free to add my Reviewed-by also to Oleh's patch of course.

I've applied the patch from Oleh then, and added your Reviewed-by.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] led: triggers: Fix NULL dereference in led_trigger_set() error handling

[Jacek Anaszewski]

On 9/5/19 2:46 PM, Uwe Kleine-König wrote:
> On Thu, Sep 05, 2019 at 03:23:21PM +0300, Oleh Kravchenko wrote:
>> Hello Jacek,
>>
>>> 5 вер. 2019 р. о 3:06 пп Uwe Kleine-König <u.kleine-koenig@pengutronix.de> написав(ла):
>>>
>>> Hello,
>>>
>>> On Thu, Sep 05, 2019 at 12:57:28PM +0300, Dan Carpenter wrote:
>>>> The problem is we set "led_cdev->trigger = NULL;" and then dereference
>>>> it when we call write_lock_irqsave():
>>>>
>>>> 	write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
>>>>                            ^^^^^^^^^^^^^^^^^
>>>>
>>>> Fixes: 2282e125a406 ("leds: triggers: let struct led_trigger::activate() return an error code")
>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>
>>> Obviously right. Thanks for catching.
>>>
>>> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>>
>>> Did you find this at runtime or by using some static checker?
>>
>> Let me summarize the chronology of the last activities below:
>> 1. I have sent the patch for the bugs that I have found by static analyzer at PVS-Studio
>>     Date: Wed, 4 Sep 2019 00:18:19 +0300
>>     https://www.spinics.net/lists/linux-leds/msg13181.html
>>
>> 2. At 5 Sep 2019 12:57:19 +0300 Time Dan Cartpen has sent the patch with the same proposal
>> 3. Uwe Kleine-König started to discuss his results of review by asking Dan on how he was found it.
>>
>> Would you mine if you will keep me as a Original author of this patch based on fact 1?
> 
> I don't care much personally but it seems fair to take Oleh's version. I
> didn't see Oleh's patch before as only Dan's was Cc:d to me.
> 
> Feel free to add my Reviewed-by also to Oleh's patch of course.

I've applied the patch from Oleh then, and added your Reviewed-by.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] led: triggers: Fix NULL dereference in led_trigger_set() error handling

[Oleh Kravchenko]

Hello Dan,

> 5 вер. 2019 р. о 4:32 пп Dan Carpenter <dan.carpenter@oracle.com> написав(ла):
> 
> On Thu, Sep 05, 2019 at 03:23:21PM +0300, Oleh Kravchenko wrote:
>> Let me summarize the chronology of the last activities below:
>> 1. I have sent the patch for the bugs that I have found by static analyzer at PVS-Studio
>>    Date: Wed, 4 Sep 2019 00:18:19 +0300
>>    https://www.spinics.net/lists/linux-leds/msg13181.html
>> 
>> 2. At 5 Sep 2019 12:57:19 +0300 Time Dan Cartpen has sent the patch with the same proposal
>> 3. Uwe Kleine-König started to discuss his results of review by asking Dan on how he was found it.
>> 
>> Would you mine if you will keep me as a Original author of this patch based on fact 1?
> 
> Heh.
> 
> No, I didn't steal your patch.  :P  I am the author of the Smatch static
> analysis tool and mostly fix things found by Smatch.  I don't use other
> static analysis tools except to do a final QC of my patches.
> 

Thanks!
I didn’t know this tool. Will take a look on it.

By the way, you can use PVS-Studio freely
https://www.viva64.com/en/b/0600/
They provide free license for open source projects.

> It's super common for people to send duplicate fixes when it's based on
> static analysis.  Most of the static analysis people hang out on
> kernel-janitors so we don't send duplicate patches.  For a while people
> were getting annoyed by all the duplicates but now they accept it as
> their punishment for introducing a bug in the first place.
> 

No problem.
We all doing good things!

> Anyway, the rule for kernel development is that normally the first
> person's patch goes in, so we will take your patch.
> 

I think next time you will take a look at mail list before sending patches ;-)

> regards,
> dan carpenter
> 

P.S.:
resent because was in non plain-text format. 

-- 
Best regards,
Oleh Kravchenko

Re: [PATCH] led: triggers: Fix NULL dereference in led_trigger_set() error handling

[Oleh Kravchenko]

Hello Dan,

> 5 вер. 2019 р. о 4:32 пп Dan Carpenter <dan.carpenter@oracle.com> написав(ла):
> 
> On Thu, Sep 05, 2019 at 03:23:21PM +0300, Oleh Kravchenko wrote:
>> Let me summarize the chronology of the last activities below:
>> 1. I have sent the patch for the bugs that I have found by static analyzer at PVS-Studio
>>    Date: Wed, 4 Sep 2019 00:18:19 +0300
>>    https://www.spinics.net/lists/linux-leds/msg13181.html
>> 
>> 2. At 5 Sep 2019 12:57:19 +0300 Time Dan Cartpen has sent the patch with the same proposal
>> 3. Uwe Kleine-König started to discuss his results of review by asking Dan on how he was found it.
>> 
>> Would you mine if you will keep me as a Original author of this patch based on fact 1?
> 
> Heh.
> 
> No, I didn't steal your patch.  :P  I am the author of the Smatch static
> analysis tool and mostly fix things found by Smatch.  I don't use other
> static analysis tools except to do a final QC of my patches.
> 

Thanks!
I didn’t know this tool. Will take a look on it.

By the way, you can use PVS-Studio freely
https://www.viva64.com/en/b/0600/
They provide free license for open source projects.

> It's super common for people to send duplicate fixes when it's based on
> static analysis.  Most of the static analysis people hang out on
> kernel-janitors so we don't send duplicate patches.  For a while people
> were getting annoyed by all the duplicates but now they accept it as
> their punishment for introducing a bug in the first place.
> 

No problem.
We all doing good things!

> Anyway, the rule for kernel development is that normally the first
> person's patch goes in, so we will take your patch.
> 

I think next time you will take a look at mail list before sending patches ;-)

> regards,
> dan carpenter
> 

P.S.:
resent because was in non plain-text format. 

-- 
Best regards,
Oleh Kravchenko