As kzalloc may return NULL pointer, the return value should
be checked and return error if fails in order to avoid the
NULL pointer dereference.
Moreover, use the goto-label to share the clean code.
Fixes: d6b98c8d242a ("ice: add write functionality for GNSS TTY")
Signed-off-by: Jiasheng Jiang <[email protected]>
---
Changelog:
v1 -> v2:
1. Use goto-label to share the clean code.
---
drivers/net/ethernet/intel/ice/ice_gnss.c | 25 ++++++++++++++---------
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
index b5a7f246d230..7bd3452a16d2 100644
--- a/drivers/net/ethernet/intel/ice/ice_gnss.c
+++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
@@ -421,7 +421,7 @@ static struct tty_driver *ice_gnss_create_tty_driver(struct ice_pf *pf)
const int ICE_TTYDRV_NAME_MAX = 14;
struct tty_driver *tty_driver;
char *ttydrv_name;
- unsigned int i;
+ unsigned int i, j;
int err;
tty_driver = tty_alloc_driver(ICE_GNSS_TTY_MINOR_DEVICES,
@@ -462,6 +462,9 @@ static struct tty_driver *ice_gnss_create_tty_driver(struct ice_pf *pf)
GFP_KERNEL);
pf->gnss_serial[i] = NULL;
+ if (!pf->gnss_tty_port[i])
+ goto err_out;
+
tty_port_init(pf->gnss_tty_port[i]);
tty_port_link_device(pf->gnss_tty_port[i], tty_driver, i);
}
@@ -469,21 +472,23 @@ static struct tty_driver *ice_gnss_create_tty_driver(struct ice_pf *pf)
err = tty_register_driver(tty_driver);
if (err) {
dev_err(dev, "Failed to register TTY driver err=%d\n", err);
-
- for (i = 0; i < ICE_GNSS_TTY_MINOR_DEVICES; i++) {
- tty_port_destroy(pf->gnss_tty_port[i]);
- kfree(pf->gnss_tty_port[i]);
- }
- kfree(ttydrv_name);
- tty_driver_kref_put(pf->ice_gnss_tty_driver);
-
- return NULL;
+ goto err_out;
}
for (i = 0; i < ICE_GNSS_TTY_MINOR_DEVICES; i++)
dev_info(dev, "%s%d registered\n", ttydrv_name, i);
return tty_driver;
+
+err_out:
+ for (j = 0; j < i; j++) {
+ tty_port_destroy(pf->gnss_tty_port[j]);
+ kfree(pf->gnss_tty_port[j]);
+ }
+ kfree(ttydrv_name);
+ tty_driver_kref_put(pf->ice_gnss_tty_driver);
+
+ return NULL;
}
/**
--
2.25.1
On Thu, Dec 08, 2022 at 09:19:36AM +0800, Jiasheng Jiang wrote:
> As kzalloc may return NULL pointer, the return value should
> be checked and return error if fails in order to avoid the
> NULL pointer dereference.
> Moreover, use the goto-label to share the clean code.
>
> Fixes: d6b98c8d242a ("ice: add write functionality for GNSS TTY")
> Signed-off-by: Jiasheng Jiang <[email protected]>
> ---
> Changelog:
>
> v1 -> v2:
>
> 1. Use goto-label to share the clean code.
> ---
> drivers/net/ethernet/intel/ice/ice_gnss.c | 25 ++++++++++++++---------
> 1 file changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
> index b5a7f246d230..7bd3452a16d2 100644
> --- a/drivers/net/ethernet/intel/ice/ice_gnss.c
> +++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
> @@ -421,7 +421,7 @@ static struct tty_driver *ice_gnss_create_tty_driver(struct ice_pf *pf)
> const int ICE_TTYDRV_NAME_MAX = 14;
> struct tty_driver *tty_driver;
> char *ttydrv_name;
> - unsigned int i;
> + unsigned int i, j;
> int err;
>
> tty_driver = tty_alloc_driver(ICE_GNSS_TTY_MINOR_DEVICES,
> @@ -462,6 +462,9 @@ static struct tty_driver *ice_gnss_create_tty_driver(struct ice_pf *pf)
> GFP_KERNEL);
> pf->gnss_serial[i] = NULL;
>
> + if (!pf->gnss_tty_port[i])
> + goto err_out;
> +
> tty_port_init(pf->gnss_tty_port[i]);
> tty_port_link_device(pf->gnss_tty_port[i], tty_driver, i);
> }
> @@ -469,21 +472,23 @@ static struct tty_driver *ice_gnss_create_tty_driver(struct ice_pf *pf)
> err = tty_register_driver(tty_driver);
> if (err) {
> dev_err(dev, "Failed to register TTY driver err=%d\n", err);
> -
> - for (i = 0; i < ICE_GNSS_TTY_MINOR_DEVICES; i++) {
> - tty_port_destroy(pf->gnss_tty_port[i]);
> - kfree(pf->gnss_tty_port[i]);
> - }
> - kfree(ttydrv_name);
> - tty_driver_kref_put(pf->ice_gnss_tty_driver);
> -
> - return NULL;
> + goto err_out;
> }
>
> for (i = 0; i < ICE_GNSS_TTY_MINOR_DEVICES; i++)
> dev_info(dev, "%s%d registered\n", ttydrv_name, i);
>
> return tty_driver;
> +
> +err_out:
> + for (j = 0; j < i; j++) {
You don't need an extra variable, "while(i--)" will do the trick.
Thanks
> + tty_port_destroy(pf->gnss_tty_port[j]);
> + kfree(pf->gnss_tty_port[j]);
> + }
> + kfree(ttydrv_name);
> + tty_driver_kref_put(pf->ice_gnss_tty_driver);
> +
> + return NULL;
> }
>
> /**
> --
> 2.25.1
>
On Thu, Dec 08, 2022 at 05:25:02PM +0800, Leon Romanovsky wrote:
>> +err_out:
>> + for (j = 0; j < i; j++) {
>
> You don't need an extra variable, "while(i--)" will do the trick.
No, the right range is [0, i - 1], but the "while(i--)" is [1, i].
If using "while(i--)", the code should be "tty_port_destroy(pf->gnss_tty_port[i - 1]);".
It will be more complex.
Therefore, it is worthwhile to use an extra varaible.
Thanks,
Jiang
Thu, Dec 08, 2022 at 11:06:03AM CET, [email protected] wrote:
>On Thu, Dec 08, 2022 at 05:25:02PM +0800, Leon Romanovsky wrote:
>>> +err_out:
>>> + for (j = 0; j < i; j++) {
>>
>> You don't need an extra variable, "while(i--)" will do the trick.
>
>No, the right range is [0, i - 1], but the "while(i--)" is [1, i].
Are you sure??
>If using "while(i--)", the code should be "tty_port_destroy(pf->gnss_tty_port[i - 1]);".
>It will be more complex.
>Therefore, it is worthwhile to use an extra varaible.
>
>Thanks,
>Jiang
>
On Thu, Dec 08, 2022 at 07:17:59PM +0800, Leon Romanovsky wrote:
>>>> +err_out:
>>>> + for (j = 0; j < i; j++) {
>>>
>>> You don't need an extra variable, "while(i--)" will do the trick.
>>
>>No, the right range is [0, i - 1], but the "while(i--)" is [1, i].
>
> Are you sure??
Yes, you are right.
I will submit a v3 to add this change.
Thanks,
Jiang