| Message ID | 20240417-s390-drivers-fix-cast-function-type-v1-1-fd048c9903b0@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | drivers/s390: Fix instances of -Wcast-function-type-strict | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Wed, Apr 17, 2024, at 20:24, 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> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
On 17/04/2024 20.24, 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(-) Reviewed-by: Thomas Huth <thuth@redhat.com>
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.
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.
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
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.
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
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 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.
On Thu, Apr 18, 2024, at 17:15, Heiko Carstens wrote: >> > > > - /* >> > > > - * 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 think the way this should work is to have the allocation and the release function in the iucv bus driver, with a function roughly like struct device *iucv_alloc_device(char *name, const struct attribute_group *attrs, void *priv) { dev = kzalloc(sizeof(struct device), GFP_KERNEL); if (!dev) return NULL; dev_set_name(dev, "%s", name); dev->bus = &iucv_bus; dev->parent = iucv_root; dev->groups = attrs; dev_set_drvdata(dev, priv); dev->release = iucv_free_dev; return dev; } Now the release function cannot go away as long as any module is loaded that links against it, and those modules cannot go away as long as the devices are in use. I don't remember how iucv works, but if there is a way to detect which system services exist, then the actual device creation should also be separate from the driver using those services, with another driver responsible for enumerating the existing services and creating those devices. Arnd
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
On Thu, Apr 18, 2024 at 09:46:18PM +0200, Arnd Bergmann wrote: > On Thu, Apr 18, 2024, at 17:15, Heiko Carstens wrote: > >> > > > - /* > >> > > > - * 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 ...). > >> > > > - */ > > I think the way this should work is to have the allocation and > the release function in the iucv bus driver, with a function > roughly like > > struct device *iucv_alloc_device(char *name, > const struct attribute_group *attrs, > void *priv) > { > dev = kzalloc(sizeof(struct device), GFP_KERNEL); > if (!dev) > return NULL; > > dev_set_name(dev, "%s", name); > dev->bus = &iucv_bus; > dev->parent = iucv_root; > dev->groups = attrs; > dev_set_drvdata(dev, priv); > dev->release = iucv_free_dev; > > return dev; > } > > Now the release function cannot go away as long as any module > is loaded that links against it, and those modules cannot > go away as long as the devices are in use. > > I don't remember how iucv works, but if there is a way to > detect which system services exist, then the actual device > creation should also be separate from the driver using those > services, with another driver responsible for enumerating > the existing services and creating those devices. I have the impression we have the same discussion like it happened 20 years ago: https://lore.kernel.org/all/OF876C2271.59086B92-ONC1256E5A.00409933-C1256E5A.00427853@de.ibm.com/ Adding extra module dependencies won't help, since it just moves the potential races. However what could easily solve this problem is to make CONFIG_IUCV a boolean config option instead of tristate. If it would be compiled in, the release function cannot go away. We have already "def_tristate y if S390" for IUCV, so it looks like this wouldn't change anything in real life. In addition with something like your proposed change, we should be fine. Plus we need to fix the potential bug you introduced with commit 42af6bcbc351 ("tty: hvc-iucv: fix function pointer casts"). But at least this is also iucv_bus related. Alexandra, Thorsten, any objections if CONFIG_IUCV would be changed so it can only be compiled in or out, but not as a module anymore?
On Fri, Apr 19, 2024, at 14:15, Heiko Carstens wrote: > > Plus we need to fix the potential bug you introduced with commit > 42af6bcbc351 ("tty: hvc-iucv: fix function pointer casts"). But at > least this is also iucv_bus related. > > Alexandra, Thorsten, any objections if CONFIG_IUCV would be changed so > it can only be compiled in or out, but not as a module anymore? You can also just drop the iucv_exit() function, making the module non-removable when it has an init function but no exit. Arnd
On Fri, Apr 19, 2024 at 02:19:14PM +0200, Arnd Bergmann wrote: > On Fri, Apr 19, 2024, at 14:15, Heiko Carstens wrote: > > > > Plus we need to fix the potential bug you introduced with commit > > 42af6bcbc351 ("tty: hvc-iucv: fix function pointer casts"). But at > > least this is also iucv_bus related. > > > > Alexandra, Thorsten, any objections if CONFIG_IUCV would be changed so > > it can only be compiled in or out, but not as a module anymore? > > You can also just drop the iucv_exit() function, making the > module non-removable when it has an init function but no exit. Right, that's better, and also what I did back then for the zfcp module for the same reason.
On 19.04.24 16:12, Heiko Carstens wrote: > On Fri, Apr 19, 2024 at 02:19:14PM +0200, Arnd Bergmann wrote: >> On Fri, Apr 19, 2024, at 14:15, Heiko Carstens wrote: >>> >>> Plus we need to fix the potential bug you introduced with commit >>> 42af6bcbc351 ("tty: hvc-iucv: fix function pointer casts"). But at >>> least this is also iucv_bus related. >>> >>> Alexandra, Thorsten, any objections if CONFIG_IUCV would be changed so >>> it can only be compiled in or out, but not as a module anymore? >> >> You can also just drop the iucv_exit() function, making the >> module non-removable when it has an init function but no exit. > > Right, that's better, and also what I did back then for the zfcp > module for the same reason. > Heiko, as discussed f2f: 'no module' or 'non-removable module' both options are fine with me. I would prefer non-removable. Both are better than calling removed functions. This also applies to patches 2 and 3 of this series: drivers/s390/net/smsgiucv_app.c drivers/s390/net/netiucv.c Thank you Alexandra
diff --git a/drivers/s390/char/vmlogrdr.c b/drivers/s390/char/vmlogrdr.c index d7e408c8d0b8..8f90f58b680a 100644 --- a/drivers/s390/char/vmlogrdr.c +++ b/drivers/s390/char/vmlogrdr.c @@ -722,6 +722,10 @@ static void vmlogrdr_unregister_driver(void) iucv_unregister(&vmlogrdr_iucv_handler, 1); } +static void vmlogrdr_free_dev(struct device *dev) +{ + kfree(dev); +} static int vmlogrdr_register_device(struct vmlogrdr_priv_t *priv) { @@ -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 ...). - */ - dev->release = (void (*)(struct device *))kfree; + dev->release = vmlogrdr_free_dev; } else return -ENOMEM; ret = device_register(dev);
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(-)