Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Sun, 20 Mar 2022 15:50:56 +0000
From:   Jonathan Cameron <jic23@kernel.org>
To:     xkernel.wang@foxmail.com
Cc:     jic23@jic23.retrosnub.co.uk, lars@metafoo.de,
        linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] iio: dummy: iio_simple_dummy: check the return value
 of kstrdup()
Message-ID: <20220320155056.5e365972@jic23-huawei>
In-Reply-To: <tencent_C920CFCC33B9CC1C63141FE1334A39FF8508@qq.com>
References: <tencent_C920CFCC33B9CC1C63141FE1334A39FF8508@qq.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Sat,  5 Mar 2022 11:14:05 +0800
xkernel.wang@foxmail.com wrote:

> From: Xiaoke Wang <xkernel.wang@foxmail.com>
> 
> kstrdup() is also a memory allocation-related function, it returns NULL
> when some memory errors happen. So it is better to check the return
> value of it so to catch the memory error in time. Besides, there should
> have a kfree() to clear up the allocation if we get a failure later in
> this function to prevent memory leak.
> 
> Signed-off-by: Xiaoke Wang <xkernel.wang@foxmail.com>

I made a small tweak whilst applying.  It added an extra label but
I think makes the flow easier to follow than relying on the kfree(NULL)
so two error paths can share the same handling.

Applied with that tweak to the togreg branch of iio.git and pushed out
as testing for 0-day to see if it can find anything we missed.

Note I will rebase that tree on rc1 once available so it is not a
stable base to use for anything else.

Thanks,

Jonathan


> ---
> I am sorry that I forgot to send this.
> Changelogs:
> v1->v2 add kfree() on the error path.
> v2->v3 change the err lable.
>  drivers/iio/dummy/iio_simple_dummy.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/dummy/iio_simple_dummy.c b/drivers/iio/dummy/iio_simple_dummy.c
> index c0b7ef9..99e7731 100644
> --- a/drivers/iio/dummy/iio_simple_dummy.c
> +++ b/drivers/iio/dummy/iio_simple_dummy.c
> @@ -575,10 +575,9 @@ static struct iio_sw_device *iio_dummy_probe(const char *name)
>  	 */
>  
>  	swd = kzalloc(sizeof(*swd), GFP_KERNEL);
> -	if (!swd) {
> -		ret = -ENOMEM;
> -		goto error_kzalloc;
> -	}
> +	if (!swd)
> +		return ERR_PTR(-ENOMEM);
> +
>  	/*
>  	 * Allocate an IIO device.
>  	 *
> @@ -590,7 +589,7 @@ static struct iio_sw_device *iio_dummy_probe(const char *name)
>  	indio_dev = iio_device_alloc(parent, sizeof(*st));
>  	if (!indio_dev) {
>  		ret = -ENOMEM;
> -		goto error_ret;
> +		goto error_free_swd;
>  	}
>  
>  	st = iio_priv(indio_dev);
> @@ -616,6 +615,10 @@ static struct iio_sw_device *iio_dummy_probe(const char *name)
>  	 *    indio_dev->name = spi_get_device_id(spi)->name;
>  	 */
>  	indio_dev->name = kstrdup(name, GFP_KERNEL);
> +	if (!indio_dev->name) {
> +		ret = -ENOMEM;
> +		goto error_free_device;
> +	}
>  
>  	/* Provide description of available channels */
>  	indio_dev->channels = iio_dummy_channels;
> @@ -650,10 +653,10 @@ static struct iio_sw_device *iio_dummy_probe(const char *name)
>  error_unregister_events:
>  	iio_simple_dummy_events_unregister(indio_dev);
>  error_free_device:
> +	kfree(indio_dev->name);
I'm going to tweak this a tiny bit whilst applying and have separate label for
the two bits of cleanup.

Whilst it is of course correct to kfree(NULL) the flow isn't as obvious as it will be with
the extra label.

>  	iio_device_free(indio_dev);
> -error_ret:
> +error_free_swd:
>  	kfree(swd);
> -error_kzalloc:
>  	return ERR_PTR(ret);
>  }
>