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

[Nathan Chancellor <nathan@kernel.org>]

Hi Heiko,

On Thu, Apr 18, 2024 at 12:25:49PM +0200, Heiko Carstens wrote:
> On Thu, Apr 18, 2024 at 11:54:38AM +0200, Heiko Carstens wrote:
> > On Wed, Apr 17, 2024 at 11:24:35AM -0700, Nathan Chancellor wrote:
> > > Clang warns (or errors with CONFIG_WERROR) after enabling
> > > -Wcast-function-type-strict by default:
> > > 
> > >   drivers/s390/char/vmlogrdr.c:746:18: error: cast from 'void (*)(const void *)' to 'void (*)(struct device *)' converts to incompatible function type [-Werror,-Wcast-function-type-strict]
> > >     746 |                 dev->release = (void (*)(struct device *))kfree;
> > >         |                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >   1 error generated.
> > > 
> > > Add a standalone function to fix the warning properly, which addresses
> > > the root of the warning that these casts are not safe for kCFI. The
> > > comment is not really relevant after this change, so remove it.
> > > 
> > > Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> > > ---
> > >  drivers/s390/char/vmlogrdr.c | 13 +++++--------
> > >  1 file changed, 5 insertions(+), 8 deletions(-)
> > 
> > > @@ -736,14 +740,7 @@ static int vmlogrdr_register_device(struct vmlogrdr_priv_t *priv)
> > >  		dev->driver = &vmlogrdr_driver;
> > >  		dev->groups = vmlogrdr_attr_groups;
> > >  		dev_set_drvdata(dev, priv);
> > > -		/*
> > > -		 * 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.

Cheers,
Nathan