Re: [PATCH net v3] ice: Add check for kzalloc

From: Jiri Pirko
Date: Thu Dec 08 2022 - 08:07:15 EST


Thu, Dec 08, 2022 at 01:58:47PM CET, jiasheng@xxxxxxxxxxx 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.

It would be nice if you change the sentence above to imperative mood.
Tell the codebase what to do.


>Moreover, use the goto-label to share the clean code.
>
>Fixes: d6b98c8d242a ("ice: add write functionality for GNSS TTY")
>Signed-off-by: Jiasheng Jiang <jiasheng@xxxxxxxxxxx>
>---
>Changelog:
>
>v2 -> v3:
>
>1. Use "while (i--)" to simplify the code.
>
>v1 -> v2:
>
>1. Use goto-label to share the clean code.
>---
> drivers/net/ethernet/intel/ice/ice_gnss.c | 23 ++++++++++++++---------
> 1 file changed, 14 insertions(+), 9 deletions(-)
>
>diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
>index b5a7f246d230..90c620c26516 100644
>--- a/drivers/net/ethernet/intel/ice/ice_gnss.c
>+++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
>@@ -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;

Move the check right after the alloc.


>+
> 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:
>+ while (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;

This looks fine now.


> }
>
> /**
>--
>2.25.1
>