* [PATCH] hid: logitech: check the return value of create_singlethread_workqueue
@ 2019-03-09 4:43 Kangjie Lu
2019-03-11 15:08 ` Benjamin Tissoires
0 siblings, 1 reply; 6+ messages in thread
From: Kangjie Lu @ 2019-03-09 4:43 UTC (permalink / raw)
To: kjlu; +Cc: pakki001, Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel
create_singlethread_workqueue may fail and return NULL. The fix
checks if it is NULL to avoid NULL pointer dereference.
Signed-off-by: Kangjie Lu <kjlu@umn.edu>
---
drivers/hid/hid-logitech-hidpp.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 15ed6177a7a3..efbc39b92aa2 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2156,6 +2156,9 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
/* init the hardware command queue */
data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
+ if (!data->wq)
+ return -ENOMEM;
+
atomic_set(&data->workqueue_size, 0);
/* initialize with zero autocenter to get wheel in usable state */
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] hid: logitech: check the return value of create_singlethread_workqueue
2019-03-09 4:43 [PATCH] hid: logitech: check the return value of create_singlethread_workqueue Kangjie Lu
@ 2019-03-11 15:08 ` Benjamin Tissoires
2019-03-12 6:16 ` [PATCH v2] " Kangjie Lu
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Tissoires @ 2019-03-11 15:08 UTC (permalink / raw)
To: Kangjie Lu; +Cc: pakki001, Jiri Kosina, open list:HID CORE LAYER, lkml
On Sat, Mar 9, 2019 at 5:43 AM Kangjie Lu <kjlu@umn.edu> wrote:
>
> create_singlethread_workqueue may fail and return NULL. The fix
> checks if it is NULL to avoid NULL pointer dereference.
>
> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> ---
> drivers/hid/hid-logitech-hidpp.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 15ed6177a7a3..efbc39b92aa2 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -2156,6 +2156,9 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
>
> /* init the hardware command queue */
> data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
> + if (!data->wq)
> + return -ENOMEM;
> +
I agree there is something wrong with the current code, but I think
you are not fixing the issue properly.
The code just above allocates data with kzalloc and stores it in the
hid_device and set up everything. If create_singlethread_workqueue()
fails now, then we are screwed and you need to reverse all of the FF
init.
I think we should move the thread creation above so we are not leaking
memory and not create the FF elements if there is something wrong.
Cheers,
Benjamin
> atomic_set(&data->workqueue_size, 0);
>
> /* initialize with zero autocenter to get wheel in usable state */
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v2] hid: logitech: check the return value of create_singlethread_workqueue
2019-03-11 15:08 ` Benjamin Tissoires
@ 2019-03-12 6:16 ` Kangjie Lu
2019-03-12 16:02 ` Jonathan Corbet
0 siblings, 1 reply; 6+ messages in thread
From: Kangjie Lu @ 2019-03-12 6:16 UTC (permalink / raw)
To: kjlu; +Cc: pakki001, Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel
create_singlethread_workqueue may fail and return NULL. The fix
checks if it is NULL to avoid NULL pointer dereference.
Also, the fix moves the call of create_singlethread_workqueue
earlier to avoid resource-release issues.
Signed-off-by: Kangjie Lu <kjlu@umn.edu>
---
drivers/hid/hid-logitech-hidpp.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 15ed6177a7a3..1b7c336cae6d 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2106,6 +2106,12 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
data = kzalloc(sizeof(*data), GFP_KERNEL);
if (!data)
return -ENOMEM;
+
+ /* init the hardware command queue */
+ data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
+ if (!data->wq)
+ return -ENOMEM;
+
data->effect_ids = kcalloc(num_slots, sizeof(int), GFP_KERNEL);
if (!data->effect_ids) {
kfree(data);
@@ -2154,8 +2160,6 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
data->gain = error ? 0xffff : get_unaligned_be16(&response.fap.params[0]);
/* ignore boost value at response.fap.params[2] */
- /* init the hardware command queue */
- data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
atomic_set(&data->workqueue_size, 0);
/* initialize with zero autocenter to get wheel in usable state */
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2] hid: logitech: check the return value of create_singlethread_workqueue
2019-03-12 6:16 ` [PATCH v2] " Kangjie Lu
@ 2019-03-12 16:02 ` Jonathan Corbet
2019-03-14 5:24 ` [PATCH v3] " Kangjie Lu
0 siblings, 1 reply; 6+ messages in thread
From: Jonathan Corbet @ 2019-03-12 16:02 UTC (permalink / raw)
To: Kangjie Lu
Cc: pakki001, Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel
On Tue, 12 Mar 2019 01:16:28 -0500
Kangjie Lu <kjlu@umn.edu> wrote:
> create_singlethread_workqueue may fail and return NULL. The fix
> checks if it is NULL to avoid NULL pointer dereference.
> Also, the fix moves the call of create_singlethread_workqueue
> earlier to avoid resource-release issues.
>
> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
So I don't know this code at all, but...
> drivers/hid/hid-logitech-hidpp.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 15ed6177a7a3..1b7c336cae6d 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -2106,6 +2106,12 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
> data = kzalloc(sizeof(*data), GFP_KERNEL);
> if (!data)
> return -ENOMEM;
> +
> + /* init the hardware command queue */
> + data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
> + if (!data->wq)
> + return -ENOMEM;
It's clear just from the diff that this return will leak 'data'. You also
break the error handling just below:
> data->effect_ids = kcalloc(num_slots, sizeof(int), GFP_KERNEL);
> if (!data->effect_ids) {
> kfree(data);
It's also worth asking: how are you testing these error path changes?
Thanks,
jon
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v3] hid: logitech: check the return value of create_singlethread_workqueue
2019-03-12 16:02 ` Jonathan Corbet
@ 2019-03-14 5:24 ` Kangjie Lu
2019-03-19 10:48 ` Jiri Kosina
0 siblings, 1 reply; 6+ messages in thread
From: Kangjie Lu @ 2019-03-14 5:24 UTC (permalink / raw)
To: kjlu; +Cc: pakki001, Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel
create_singlethread_workqueue may fail and return NULL. The fix
checks if it is NULL to avoid NULL pointer dereference.
Also, the fix moves the call of create_singlethread_workqueue
earlier to avoid resource-release issues.
--
V3: do not introduce memory leaks.
Signed-off-by: Kangjie Lu <kjlu@umn.edu>
---
drivers/hid/hid-logitech-hidpp.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 15ed6177a7a3..0a243247b231 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2111,6 +2111,13 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
kfree(data);
return -ENOMEM;
}
+ data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
+ if (!data->wq) {
+ kfree(data->effect_ids);
+ kfree(data);
+ return -ENOMEM;
+ }
+
data->hidpp = hidpp;
data->feature_index = feature_index;
data->version = version;
@@ -2155,7 +2162,6 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
/* ignore boost value at response.fap.params[2] */
/* init the hardware command queue */
- data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
atomic_set(&data->workqueue_size, 0);
/* initialize with zero autocenter to get wheel in usable state */
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v3] hid: logitech: check the return value of create_singlethread_workqueue
2019-03-14 5:24 ` [PATCH v3] " Kangjie Lu
@ 2019-03-19 10:48 ` Jiri Kosina
0 siblings, 0 replies; 6+ messages in thread
From: Jiri Kosina @ 2019-03-19 10:48 UTC (permalink / raw)
To: Kangjie Lu; +Cc: pakki001, Benjamin Tissoires, linux-input, linux-kernel
On Thu, 14 Mar 2019, Kangjie Lu wrote:
> create_singlethread_workqueue may fail and return NULL. The fix
> checks if it is NULL to avoid NULL pointer dereference.
> Also, the fix moves the call of create_singlethread_workqueue
> earlier to avoid resource-release issues.
>
> --
> V3: do not introduce memory leaks.
>
> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
The signoff has to be in the commit log, not in the "notes" area. I've
fixed that and applied.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-03-19 10:48 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-09 4:43 [PATCH] hid: logitech: check the return value of create_singlethread_workqueue Kangjie Lu
2019-03-11 15:08 ` Benjamin Tissoires
2019-03-12 6:16 ` [PATCH v2] " Kangjie Lu
2019-03-12 16:02 ` Jonathan Corbet
2019-03-14 5:24 ` [PATCH v3] " Kangjie Lu
2019-03-19 10:48 ` Jiri Kosina
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.