Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 8 Dec 2022 11:24:02 +0200
From:   Leon Romanovsky <leon@kernel.org>
To:     Jiasheng Jiang <jiasheng@iscas.ac.cn>
Cc:     jiri@resnulli.us, jesse.brandeburg@intel.com,
        anthony.l.nguyen@intel.com, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
        intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net v2] ice: Add check for kzalloc
Message-ID: <Y5GtMk64Zg+tnbMS@unreal>
References: <20221208011936.47943-1-jiasheng@iscas.ac.cn>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221208011936.47943-1-jiasheng@iscas.ac.cn>
Precedence: bulk

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 <jiasheng@iscas.ac.cn>
> ---
> 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
>