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

[Nathan Chancellor <nathan@kernel.org>]

On Thu, Apr 18, 2024 at 09:21:00PM +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 ...).
> > > > > > > -		 */
> > > 
> > > 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?
> 
> I guess there is some confusion here :) My request was not to keep the

Heh, yes, my apologies for being rather dense, I was not interpreting
the comment or the thread you linked properly... :(

> comment. I'm much rather afraid that the comment is still valid; and if
> that is the case then your patch series adds three bugs, exactly what is
> described in the comment.
> 
> Right now the release function is kfree which is always within the kernel
> image, and therefore always a valid branch target. If however the code is
> changed to what you propose, then the release function would be inside of
> the module, which potentially does not exist anymore when the release
> function is called, since the module was unloaded.
> So the branch target would be invalid.

That is super subtle :/ I can understand what the comment is warning
about with that extra context. I see Arnd's suggestion which may fix
this problem and get rid of the warning but if there are other ideas, I
am all ears. I guess we could just disable -Wcast-function-type-strict
for this code since s390 does not support kCFI right now but since it
could, it seems better to resolve it properly.

Thanks a lot for the quick review and catching my mistake, cheers!
Nathan