Re: [PATCH 1/3] s390/vmlogrdr: Remove function pointer cast

From: Nathan Chancellor <nathan@kernel.org>
To: Heiko Carstens <hca@linux.ibm.com>
Cc: akpm@linux-foundation.org, arnd@arndb.de, gor@linux.ibm.com,
	agordeev@linux.ibm.com, borntraeger@linux.ibm.com,
	svens@linux.ibm.com, wintera@linux.ibm.com,
	twinkler@linux.ibm.com, linux-s390@vger.kernel.org,
	netdev@vger.kernel.org, llvm@lists.linux.dev,
	patches@lists.linux.dev
Subject: Re: [PATCH 1/3] s390/vmlogrdr: Remove function pointer cast
Date: Thu, 18 Apr 2024 08:34:06 -0700	[thread overview]
Message-ID: <20240418153406.GC1435416@dev-arch.thelio-3990X> (raw)
In-Reply-To: <20240418151501.6056-C-hca@linux.ibm.com>

On Thu, Apr 18, 2024 at 05:15:01PM +0200, Heiko Carstens wrote:
> Hi Nathan,
> 
> > > > > -		/*
> > > > > -		 * The release function could be called after the
> > > > > -		 * module has been unloaded. It's _only_ task is to
> > > > > -		 * free the struct. Therefore, we specify kfree()
> > > > > -		 * directly here. (Probably a little bit obfuscating
> > > > > -		 * but legitime ...).
> > > > > -		 */
> > > > 
> > > > Why is the comment not relevant after this change? Or better: why is it not
> > > > valid before this change, which is why the code was introduced a very long
> > > > time ago? Any reference?
> > > > 
> > > > I've seen the warning since quite some time, but didn't change the code
> > > > before sure that this doesn't introduce the bug described in the comment.
> > > 
> > > From only 20 years ago:
> > > 
> > > https://lore.kernel.org/all/20040316170812.GA14971@kroah.com/
> > > 
> > > The particular code (zfcp) was changed, so it doesn't have this code
> > > (or never did?)  anymore, but for the rest this may or may not still
> > > be valid.
> > 
> > I guess relevant may not have been the correct word. Maybe obvious? I
> > can keep the comment but I do not really see what it adds, although
> > reading the above thread, I suppose it was added as justification for
> > calling kfree() as ->release() for a 'struct device'? Kind of seems like
> > that ship has sailed since I see this all over the place as a
> > ->release() function. I do not see how this patch could have a function
> > change beyond that but I may be misreading or misinterpreting your full
> > comment.
> 
> That doesn't answer my question what prevents the release function
> from being called after the module has been unloaded.
> 
> At least back then when the code was added it was a real bug.

I do not know the answer to that question (and I suspect there is
nothing preventing ->release() from being called after module unload),
so I'll just bring back the comment (although I'll need to adjust it
since kfree() is not being used there directly anymore). Andrew, would
you prefer a diff from what's in -mm or a v2?

Cheers,
Nathan