This error message doesn't really add any information. If modprobe
fails then the user will already know what the error code is. In the
case of kmalloc() it's a style violation to print an error message for
that because kmalloc has it's own better error messages built in.
Signed-off-by: Dan Carpenter <[email protected]>
---
drivers/hid/hid-steam.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index b3c4e50e248a..59df6ead7b54 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -1109,10 +1109,9 @@ static int steam_probe(struct hid_device *hdev,
return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
steam = devm_kzalloc(&hdev->dev, sizeof(*steam), GFP_KERNEL);
- if (!steam) {
- ret = -ENOMEM;
- goto steam_alloc_fail;
- }
+ if (!steam)
+ return -ENOMEM;
+
steam->hdev = hdev;
hid_set_drvdata(hdev, steam);
spin_lock_init(&steam->lock);
@@ -1179,9 +1178,6 @@ static int steam_probe(struct hid_device *hdev,
cancel_work_sync(&steam->work_connect);
cancel_delayed_work_sync(&steam->mode_switch);
cancel_work_sync(&steam->rumble_work);
-steam_alloc_fail:
- hid_err(hdev, "%s: failed with error %d\n",
- __func__, ret);
return ret;
}
--
2.43.0
There are a number of issues in this code. First of all if
steam_create_client_hid() fails then it leads to an error pointer
dereference when we call hid_destroy_device(steam->client_hdev).
Also there are a number of leaks. hid_hw_stop() is not called if
hid_hw_open() fails for example. And it doesn't call steam_unregister()
or hid_hw_close().
Fixes: 691ead124a0c ("HID: hid-steam: Clean up locking")
Signed-off-by: Dan Carpenter <[email protected]>
---
This is just from static analysis and code review. I haven't tested
it. I only included the fixes tag for the error pointer dereference.
drivers/hid/hid-steam.c | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 59df6ead7b54..b08a5ab58528 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -1128,14 +1128,14 @@ static int steam_probe(struct hid_device *hdev,
*/
ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDRAW);
if (ret)
- goto hid_hw_start_fail;
+ goto err_cancel_work;
ret = hid_hw_open(hdev);
if (ret) {
hid_err(hdev,
"%s:hid_hw_open\n",
__func__);
- goto hid_hw_open_fail;
+ goto err_hw_stop;
}
if (steam->quirks & STEAM_QUIRK_WIRELESS) {
@@ -1151,33 +1151,37 @@ static int steam_probe(struct hid_device *hdev,
hid_err(hdev,
"%s:steam_register failed with error %d\n",
__func__, ret);
- goto input_register_fail;
+ goto err_hw_close;
}
}
steam->client_hdev = steam_create_client_hid(hdev);
if (IS_ERR(steam->client_hdev)) {
ret = PTR_ERR(steam->client_hdev);
- goto client_hdev_fail;
+ goto err_stream_unregister;
}
steam->client_hdev->driver_data = steam;
ret = hid_add_device(steam->client_hdev);
if (ret)
- goto client_hdev_add_fail;
+ goto err_destroy;
return 0;
-client_hdev_add_fail:
- hid_hw_stop(hdev);
-client_hdev_fail:
+err_destroy:
hid_destroy_device(steam->client_hdev);
-input_register_fail:
-hid_hw_open_fail:
-hid_hw_start_fail:
+err_stream_unregister:
+ if (steam->connected)
+ steam_unregister(steam);
+err_hw_close:
+ hid_hw_close(hdev);
+err_hw_stop:
+ hid_hw_stop(hdev);
+err_cancel_work:
cancel_work_sync(&steam->work_connect);
cancel_delayed_work_sync(&steam->mode_switch);
cancel_work_sync(&steam->rumble_work);
+
return ret;
}
--
2.43.0
I have applied this to our downstream and made sure it compiles and runs
without obvious issues.
Reviewed-by: Vicki Pfau <[email protected]>
Thanks,
Vicki
On 1/12/24 06:35, Dan Carpenter wrote:
> There are a number of issues in this code. First of all if
> steam_create_client_hid() fails then it leads to an error pointer
> dereference when we call hid_destroy_device(steam->client_hdev).
>
> Also there are a number of leaks. hid_hw_stop() is not called if
> hid_hw_open() fails for example. And it doesn't call steam_unregister()
> or hid_hw_close().
>
> Fixes: 691ead124a0c ("HID: hid-steam: Clean up locking")
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> This is just from static analysis and code review. I haven't tested
> it. I only included the fixes tag for the error pointer dereference.
>
> drivers/hid/hid-steam.c | 26 +++++++++++++++-----------
> 1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 59df6ead7b54..b08a5ab58528 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -1128,14 +1128,14 @@ static int steam_probe(struct hid_device *hdev,
> */
> ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDRAW);
> if (ret)
> - goto hid_hw_start_fail;
> + goto err_cancel_work;
>
> ret = hid_hw_open(hdev);
> if (ret) {
> hid_err(hdev,
> "%s:hid_hw_open\n",
> __func__);
> - goto hid_hw_open_fail;
> + goto err_hw_stop;
> }
>
> if (steam->quirks & STEAM_QUIRK_WIRELESS) {
> @@ -1151,33 +1151,37 @@ static int steam_probe(struct hid_device *hdev,
> hid_err(hdev,
> "%s:steam_register failed with error %d\n",
> __func__, ret);
> - goto input_register_fail;
> + goto err_hw_close;
> }
> }
>
> steam->client_hdev = steam_create_client_hid(hdev);
> if (IS_ERR(steam->client_hdev)) {
> ret = PTR_ERR(steam->client_hdev);
> - goto client_hdev_fail;
> + goto err_stream_unregister;
> }
> steam->client_hdev->driver_data = steam;
>
> ret = hid_add_device(steam->client_hdev);
> if (ret)
> - goto client_hdev_add_fail;
> + goto err_destroy;
>
> return 0;
>
> -client_hdev_add_fail:
> - hid_hw_stop(hdev);
> -client_hdev_fail:
> +err_destroy:
> hid_destroy_device(steam->client_hdev);
> -input_register_fail:
> -hid_hw_open_fail:
> -hid_hw_start_fail:
> +err_stream_unregister:
> + if (steam->connected)
> + steam_unregister(steam);
> +err_hw_close:
> + hid_hw_close(hdev);
> +err_hw_stop:
> + hid_hw_stop(hdev);
> +err_cancel_work:
> cancel_work_sync(&steam->work_connect);
> cancel_delayed_work_sync(&steam->mode_switch);
> cancel_work_sync(&steam->rumble_work);
> +
> return ret;
> }
>
On Fri, 12 Jan 2024 17:34:14 +0300, Dan Carpenter wrote:
> This error message doesn't really add any information. If modprobe
> fails then the user will already know what the error code is. In the
> case of kmalloc() it's a style violation to print an error message for
> that because kmalloc has it's own better error messages built in.
>
>
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git (for-6.8/upstream-fixes), thanks!
[1/2] HID: hid-steam: remove pointless error message
https://git.kernel.org/hid/hid/c/a96681699611
[2/2] HID: hid-steam: Fix cleanup in probe()
https://git.kernel.org/hid/hid/c/a9f1da09c69f
Cheers,
--
Benjamin Tissoires <[email protected]>