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

[Jiri Pirko]

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
>