Received-SPF: pass (google.com: domain of linux-kernel+bounces-60931-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Sun, 11 Feb 2024 19:37:11 +0000
From: Jonathan Cameron <jic23@kernel.org>
To: andy.shevchenko@gmail.com
Cc: Dinghao Liu <dinghao.liu@zju.edu.cn>, Lars-Peter Clausen
 <lars@metafoo.de>, Alexandru Ardelean <alexandru.ardelean@analog.com>,
 linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] iio: core: fix memleak in iio_device_register_sysfs
Message-ID: <20240211193711.518b76d7@jic23-huawei>
In-Reply-To: <ZckdEDbAqin1Fsgt@surfacebook.localdomain>
References: <20231208073119.29283-1-dinghao.liu@zju.edu.cn>
	<20231210133228.5fd425ea@jic23-huawei>
	<20231217132800.27e83a01@jic23-huawei>
	<ZckdEDbAqin1Fsgt@surfacebook.localdomain>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Sun, 11 Feb 2024 21:16:32 +0200
andy.shevchenko@gmail.com wrote:

> Sun, Dec 17, 2023 at 01:28:00PM +0000, Jonathan Cameron kirjoitti:
> > On Sun, 10 Dec 2023 13:32:28 +0000
> > Jonathan Cameron <jic23@kernel.org> wrote:
> >   
> > > On Fri,  8 Dec 2023 15:31:19 +0800
> > > Dinghao Liu <dinghao.liu@zju.edu.cn> wrote:
> > >   
> > > > When iio_device_register_sysfs_group() fails, we should
> > > > free iio_dev_opaque->chan_attr_group.attrs to prevent
> > > > potential memleak.
> > > > 
> > > > Fixes: 32f171724e5c ("iio: core: rework iio device group creation")
> > > > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>    
> > > Hi.
> > > 
> > > Looks good to me, but I'd like to leave this one on the list a little
> > > longer to see if anyone else has comments.
> > >   
> > Guess no comments!  
> 
> This patch does not fix anything.
> 
> Yet, it might be considered as one that increases robustness, but with this applied the 
> goto
> https://elixir.bootlin.com/linux/v6.8-rc3/source/drivers/iio/industrialio-core.c#L2007
> can be amended, right?
> 

I'm lost.  That goto results in a call to 
iio_buffers_free_sysfs_and_mask(indio_dev);
which continues to undo stuff from before that call.
Now if it did
iio_device_unregister_sysfs(indio_dev);
(as per the label above it in the cleanup) then I'd agree.

Perhaps I'm misreading the code flow here?

All this code is supposed to be side effect free on error so I'm
keen on the change even if there is another path where it gets cleaned
up that I'm missing.

Jonathan