[PATCH 0/3] drivers/s390: Fix instances of -Wcast-function-type-strict

[PATCH 0/3] drivers/s390: Fix instances of -Wcast-function-type-strict

[Nathan Chancellor]

Hi all,

This series resolves the instances of -Wcast-function-type-strict that
show up in my s390 builds on -next, which has this warning enabled by
default.

The patches should be fairly uncontroversial, as this is the direction
that the kernel as a whole has been taking to resolve these warnings.
They are based on Andrew's mm-nonmm-unstable branch [1], as that has the
patch that enables -Wcast-function-type-strict. There should be no
conflicts if the s390 folks want to take the series but it may make more
sense for Andrew to take them with s390 acks.

[1]: https://git.kernel.org/akpm/mm/l/mm-nonmm-unstable

---
Nathan Chancellor (3):
      s390/vmlogrdr: Remove function pointer cast
      s390/smsgiucv_app: Remove function pointer cast
      s390/netiucv: Remove function pointer cast

 drivers/s390/char/vmlogrdr.c    | 13 +++++--------
 drivers/s390/net/netiucv.c      | 14 ++++++--------
 drivers/s390/net/smsgiucv_app.c |  7 ++++++-
 3 files changed, 17 insertions(+), 17 deletions(-)
---
base-commit: 75c44169c080221080be73998466d2e9c130caa7
change-id: 20240417-s390-drivers-fix-cast-function-type-61ae81afee7b

Best regards,
-- 
Nathan Chancellor <nathan@kernel.org>

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

[Nathan Chancellor]

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(-)

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);

-- 
2.44.0

[PATCH 2/3] s390/smsgiucv_app: Remove function pointer cast

[Nathan Chancellor]

Clang warns (or errors with CONFIG_WERROR) after enabling
-Wcast-function-type-strict by default:

  drivers/s390/net/smsgiucv_app.c:176:26: error: cast from 'void (*)(const void *)' to 'void (*)(struct device *)' converts to incompatible function type [-Werror,-Wcast-function-type-strict]
    176 |         smsg_app_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.

Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
 drivers/s390/net/smsgiucv_app.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/net/smsgiucv_app.c b/drivers/s390/net/smsgiucv_app.c
index 0a263999f7ae..390aba4ef7ad 100644
--- a/drivers/s390/net/smsgiucv_app.c
+++ b/drivers/s390/net/smsgiucv_app.c
@@ -64,6 +64,11 @@ static void smsg_app_event_free(struct smsg_app_event *ev)
 	kfree(ev);
 }
 
+static void smsg_app_free_dev(struct device *dev)
+{
+	kfree(dev);
+}
+
 static struct smsg_app_event *smsg_app_event_alloc(const char *from,
 						   const char *msg)
 {
@@ -173,7 +178,7 @@ static int __init smsgiucv_app_init(void)
 	}
 	smsg_app_dev->bus = &iucv_bus;
 	smsg_app_dev->parent = iucv_root;
-	smsg_app_dev->release = (void (*)(struct device *)) kfree;
+	smsg_app_dev->release = smsg_app_free_dev;
 	smsg_app_dev->driver = smsgiucv_drv;
 	rc = device_register(smsg_app_dev);
 	if (rc) {

-- 
2.44.0

[PATCH 3/3] s390/netiucv: Remove function pointer cast

[Nathan Chancellor]

Clang warns (or errors with CONFIG_WERROR) after enabling
-Wcast-function-type-strict by default:

  drivers/s390/net/netiucv.c:1716:18: error: cast from 'void (*)(const void *)' to 'void (*)(struct device *)' converts to incompatible function type [-Werror,-Wcast-function-type-strict]
   1716 |                 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/net/netiucv.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/s390/net/netiucv.c b/drivers/s390/net/netiucv.c
index 8852b03f943b..11df20bfc9fa 100644
--- a/drivers/s390/net/netiucv.c
+++ b/drivers/s390/net/netiucv.c
@@ -1693,6 +1693,11 @@ static const struct attribute_group *netiucv_attr_groups[] = {
 	NULL,
 };
 
+static void netiucv_free_dev(struct device *dev)
+{
+	kfree(dev);
+}
+
 static int netiucv_register_device(struct net_device *ndev)
 {
 	struct netiucv_priv *priv = netdev_priv(ndev);
@@ -1706,14 +1711,7 @@ static int netiucv_register_device(struct net_device *ndev)
 		dev->bus = &iucv_bus;
 		dev->parent = iucv_root;
 		dev->groups = netiucv_attr_groups;
-		/*
-		 * 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 = netiucv_free_dev;
 		dev->driver = &netiucv_driver;
 	} else
 		return -ENOMEM;

-- 
2.44.0

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

[Arnd Bergmann]

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>

Re: [PATCH 2/3] s390/smsgiucv_app: Remove function pointer cast

[Arnd Bergmann]

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/net/smsgiucv_app.c:176:26: error: cast from 'void 
> (*)(const void *)' to 'void (*)(struct device *)' converts to 
> incompatible function type [-Werror,-Wcast-function-type-strict]
>     176 |         smsg_app_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.
>
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>


Reviewed-by: Arnd Bergmann <arnd@arndb.de>

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

[Arnd Bergmann]

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/net/netiucv.c:1716:18: error: cast from 'void (*)(const 
> void *)' to 'void (*)(struct device *)' converts to incompatible 
> function type [-Werror,-Wcast-function-type-strict]
>    1716 |                 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>

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

[Thomas Huth]

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>

Re: [PATCH 2/3] s390/smsgiucv_app: Remove function pointer cast

[Thomas Huth]

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/net/smsgiucv_app.c:176:26: error: cast from 'void (*)(const void *)' to 'void (*)(struct device *)' converts to incompatible function type [-Werror,-Wcast-function-type-strict]
>      176 |         smsg_app_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.
> 
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
>   drivers/s390/net/smsgiucv_app.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

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

[Thomas Huth]

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/net/netiucv.c:1716:18: error: cast from 'void (*)(const void *)' to 'void (*)(struct device *)' converts to incompatible function type [-Werror,-Wcast-function-type-strict]
>     1716 |                 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/net/netiucv.c | 14 ++++++--------
>   1 file changed, 6 insertions(+), 8 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

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

[Heiko Carstens]

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.

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

[Heiko Carstens]

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.

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

[Nathan Chancellor]

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

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

[Heiko Carstens]

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.

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

[Nathan Chancellor]

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

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

[Heiko Carstens]

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.

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

[Arnd Bergmann]

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

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

[Nathan Chancellor]

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

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

[Heiko Carstens]

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?

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

[Arnd Bergmann]

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

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

[Heiko Carstens]

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.

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

[Alexandra Winter]

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

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

[Heiko Carstens]

On Thu, Apr 18, 2024 at 09:46:18PM +0200, Arnd Bergmann wrote:
> On Thu, Apr 18, 2024, at 17:15, Heiko Carstens wrote:
> > 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.

So, I finally had a deeper look at this, and it looks like the comment
that says that the release function can be called after the module is
unloaded is not correct (anymore?).

I couldn't find any put_device() calls where not also the module reference
count is increased. So I guess Nathan's patches are just fine.

However given your above suggestion, I implemented an iucv_alloc_device()
function to get rid of quite some code duplication, and the casts as well.

I'll send the series for review.