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

Nathan Chancellor
2024-04-18T14:51:23Z

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