[PATCH net v2 1/1] mlxbf_gige: Fix kernel panic at shutdown

[PATCH net v2 1/1] mlxbf_gige: Fix kernel panic at shutdown

[Asmaa Mnebhi]

There is a race condition happening during shutdown due to pending napi transactions.
Since mlxbf_gige_poll is still running, it tries to access a NULL pointer and as a
result causes a kernel panic.
To fix this during shutdown, invoke mlxbf_gige_remove to disable and dequeue napi.

Fixes: f92e1869d74e ("Add Mellanox BlueField Gigabit Ethernet driver")
Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
---
 .../net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c    | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c
index 694de9513b9f..609d038b034e 100644
--- a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c
+++ b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c
@@ -475,6 +475,9 @@ static int mlxbf_gige_remove(struct platform_device *pdev)
 {
 	struct mlxbf_gige *priv = platform_get_drvdata(pdev);
 
+	if (!priv)
+		return 0;
+
 	unregister_netdev(priv->netdev);
 	phy_disconnect(priv->netdev->phydev);
 	mlxbf_gige_mdio_remove(priv);
@@ -485,10 +488,7 @@ static int mlxbf_gige_remove(struct platform_device *pdev)
 
 static void mlxbf_gige_shutdown(struct platform_device *pdev)
 {
-	struct mlxbf_gige *priv = platform_get_drvdata(pdev);
-
-	writeq(0, priv->base + MLXBF_GIGE_INT_EN);
-	mlxbf_gige_clean_port(priv);
+	mlxbf_gige_remove(pdev);
 }
 
 static const struct acpi_device_id __maybe_unused mlxbf_gige_acpi_match[] = {
-- 
2.30.1

Re: [PATCH net v2 1/1] mlxbf_gige: Fix kernel panic at shutdown

[Samudrala, Sridhar]

On 6/7/2023 7:03 AM, Asmaa Mnebhi wrote:
> There is a race condition happening during shutdown due to pending napi transactions.
> Since mlxbf_gige_poll is still running, it tries to access a NULL pointer and as a
> result causes a kernel panic.
> To fix this during shutdown, invoke mlxbf_gige_remove to disable and dequeue napi.
> 
> Fixes: f92e1869d74e ("Add Mellanox BlueField Gigabit Ethernet driver")
> Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
> ---
>   .../net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c    | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c
> index 694de9513b9f..609d038b034e 100644
> --- a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c
> +++ b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c
> @@ -475,6 +475,9 @@ static int mlxbf_gige_remove(struct platform_device *pdev)
>   {
>   	struct mlxbf_gige *priv = platform_get_drvdata(pdev);
>   
> +	if (!priv)
> +		return 0;
> +
>   	unregister_netdev(priv->netdev);
>   	phy_disconnect(priv->netdev->phydev);
>   	mlxbf_gige_mdio_remove(priv);
> @@ -485,10 +488,7 @@ static int mlxbf_gige_remove(struct platform_device *pdev)
>   
>   static void mlxbf_gige_shutdown(struct platform_device *pdev)
>   {
> -	struct mlxbf_gige *priv = platform_get_drvdata(pdev);
> -
> -	writeq(0, priv->base + MLXBF_GIGE_INT_EN);
> -	mlxbf_gige_clean_port(priv);
> +	mlxbf_gige_remove(pdev);
>   }

With this change, do you really need mlxbf_gige_shutdown() as a separate 
function as it is only calling mlxbf_gige_remove()?


>   
>   static const struct acpi_device_id __maybe_unused mlxbf_gige_acpi_match[] = {

Re: [PATCH net v2 1/1] mlxbf_gige: Fix kernel panic at shutdown

[Leon Romanovsky]

On Wed, Jun 07, 2023 at 10:03:35AM -0400, Asmaa Mnebhi wrote:
> There is a race condition happening during shutdown due to pending napi transactions.
> Since mlxbf_gige_poll is still running, it tries to access a NULL pointer and as a
> result causes a kernel panic.
> To fix this during shutdown, invoke mlxbf_gige_remove to disable and dequeue napi.
> 
> Fixes: f92e1869d74e ("Add Mellanox BlueField Gigabit Ethernet driver")
> Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
> ---
>  .../net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c    | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c
> index 694de9513b9f..609d038b034e 100644
> --- a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c
> +++ b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c
> @@ -475,6 +475,9 @@ static int mlxbf_gige_remove(struct platform_device *pdev)
>  {
>  	struct mlxbf_gige *priv = platform_get_drvdata(pdev);
>  
> +	if (!priv)
> +		return 0;
> +

How can this check be correct? You are removing mlxbf_gige driver, priv
should be always exist here.

>  	unregister_netdev(priv->netdev);
>  	phy_disconnect(priv->netdev->phydev);
>  	mlxbf_gige_mdio_remove(priv);
> @@ -485,10 +488,7 @@ static int mlxbf_gige_remove(struct platform_device *pdev)
>  
>  static void mlxbf_gige_shutdown(struct platform_device *pdev)
>  {
> -	struct mlxbf_gige *priv = platform_get_drvdata(pdev);
> -
> -	writeq(0, priv->base + MLXBF_GIGE_INT_EN);
> -	mlxbf_gige_clean_port(priv);
> +	mlxbf_gige_remove(pdev);
>  }
>  
>  static const struct acpi_device_id __maybe_unused mlxbf_gige_acpi_match[] = {
> -- 
> 2.30.1
> 
> 

Re: [PATCH net v2 1/1] mlxbf_gige: Fix kernel panic at shutdown

[Maciej Fijalkowski]

On Sun, Jun 11, 2023 at 09:11:25PM +0300, Leon Romanovsky wrote:
> On Wed, Jun 07, 2023 at 10:03:35AM -0400, Asmaa Mnebhi wrote:
> > There is a race condition happening during shutdown due to pending napi transactions.
> > Since mlxbf_gige_poll is still running, it tries to access a NULL pointer and as a
> > result causes a kernel panic.
> > To fix this during shutdown, invoke mlxbf_gige_remove to disable and dequeue napi.
> > 
> > Fixes: f92e1869d74e ("Add Mellanox BlueField Gigabit Ethernet driver")
> > Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
> > ---
> >  .../net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c    | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c
> > index 694de9513b9f..609d038b034e 100644
> > --- a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c
> > +++ b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c
> > @@ -475,6 +475,9 @@ static int mlxbf_gige_remove(struct platform_device *pdev)
> >  {
> >  	struct mlxbf_gige *priv = platform_get_drvdata(pdev);
> >  
> > +	if (!priv)
> > +		return 0;
> > +
> 
> How can this check be correct? You are removing mlxbf_gige driver, priv
> should be always exist here.

Asmaa please include v1->v2 diff next time.

Leon, look at v1 discussion:
https://lore.kernel.org/netdev/CH2PR12MB3895172507E1D42BBD5D4AB9D753A@CH2PR12MB3895.namprd12.prod.outlook.com/

> 
> >  	unregister_netdev(priv->netdev);
> >  	phy_disconnect(priv->netdev->phydev);
> >  	mlxbf_gige_mdio_remove(priv);
> > @@ -485,10 +488,7 @@ static int mlxbf_gige_remove(struct platform_device *pdev)
> >  
> >  static void mlxbf_gige_shutdown(struct platform_device *pdev)
> >  {
> > -	struct mlxbf_gige *priv = platform_get_drvdata(pdev);
> > -
> > -	writeq(0, priv->base + MLXBF_GIGE_INT_EN);
> > -	mlxbf_gige_clean_port(priv);
> > +	mlxbf_gige_remove(pdev);
> >  }
> >  
> >  static const struct acpi_device_id __maybe_unused mlxbf_gige_acpi_match[] = {
> > -- 
> > 2.30.1
> > 
> > 
> 

Re: [PATCH net v2 1/1] mlxbf_gige: Fix kernel panic at shutdown

[Leon Romanovsky]

On Mon, Jun 12, 2023 at 01:34:49PM +0200, Maciej Fijalkowski wrote:
> On Sun, Jun 11, 2023 at 09:11:25PM +0300, Leon Romanovsky wrote:
> > On Wed, Jun 07, 2023 at 10:03:35AM -0400, Asmaa Mnebhi wrote:
> > > There is a race condition happening during shutdown due to pending napi transactions.
> > > Since mlxbf_gige_poll is still running, it tries to access a NULL pointer and as a
> > > result causes a kernel panic.
> > > To fix this during shutdown, invoke mlxbf_gige_remove to disable and dequeue napi.
> > > 
> > > Fixes: f92e1869d74e ("Add Mellanox BlueField Gigabit Ethernet driver")
> > > Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
> > > ---
> > >  .../net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c    | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c
> > > index 694de9513b9f..609d038b034e 100644
> > > --- a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c
> > > +++ b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c
> > > @@ -475,6 +475,9 @@ static int mlxbf_gige_remove(struct platform_device *pdev)
> > >  {
> > >  	struct mlxbf_gige *priv = platform_get_drvdata(pdev);
> > >  
> > > +	if (!priv)
> > > +		return 0;
> > > +
> > 
> > How can this check be correct? You are removing mlxbf_gige driver, priv
> > should be always exist here.
> 
> Asmaa please include v1->v2 diff next time.
> 
> Leon, look at v1 discussion:
> https://lore.kernel.org/netdev/CH2PR12MB3895172507E1D42BBD5D4AB9D753A@CH2PR12MB3895.namprd12.prod.outlook.com/

Thanks for the link.

As far as I can tell, the calls to .shutdown() and .remove() are
mutually exclusive. It is impossible to go twice and reach scenario
which Paolo mentioned - double call to unregister_netdevice().

Thanks

> 
> > 
> > >  	unregister_netdev(priv->netdev);
> > >  	phy_disconnect(priv->netdev->phydev);
> > >  	mlxbf_gige_mdio_remove(priv);
> > > @@ -485,10 +488,7 @@ static int mlxbf_gige_remove(struct platform_device *pdev)
> > >  
> > >  static void mlxbf_gige_shutdown(struct platform_device *pdev)
> > >  {
> > > -	struct mlxbf_gige *priv = platform_get_drvdata(pdev);
> > > -
> > > -	writeq(0, priv->base + MLXBF_GIGE_INT_EN);
> > > -	mlxbf_gige_clean_port(priv);
> > > +	mlxbf_gige_remove(pdev);
> > >  }
> > >  
> > >  static const struct acpi_device_id __maybe_unused mlxbf_gige_acpi_match[] = {
> > > -- 
> > > 2.30.1
> > > 
> > > 
> > 

Re: [PATCH net v2 1/1] mlxbf_gige: Fix kernel panic at shutdown

[Vladimir Oltean]

On Mon, Jun 12, 2023 at 02:59:25PM +0300, Leon Romanovsky wrote:
> As far as I can tell, the calls to .shutdown() and .remove() are
> mutually exclusive.

In this particular case, or in general?

In general they aren't. If the owning bus driver also implements its .shutdown()
as .remove(), then it will call the .remove() method of all devices on that bus.
That, after .shutdown() had already been called for those same children.

Re: [PATCH net v2 1/1] mlxbf_gige: Fix kernel panic at shutdown

[Leon Romanovsky]

On Mon, Jun 12, 2023 at 03:37:18PM +0300, Vladimir Oltean wrote:
> On Mon, Jun 12, 2023 at 02:59:25PM +0300, Leon Romanovsky wrote:
> > As far as I can tell, the calls to .shutdown() and .remove() are
> > mutually exclusive.
> 
> In this particular case, or in general?
> 
> In general they aren't. If the owning bus driver also implements its .shutdown()
> as .remove(), then it will call the .remove() method of all devices on that bus.
> That, after .shutdown() had already been called for those same children.

Can you please help me to see how? What is the call chain?

From what I see callback to ->shutdown() iterates over all devices in
that bus and relevant bus will check that driver is bound prior to call
to driver callback. In both cases, the driver is removed and bus won't
call to already removed device.

If it does, it is arguably bug in bus logic, which needs to prevent such
scenario.

Thanks

Re: [PATCH net v2 1/1] mlxbf_gige: Fix kernel panic at shutdown

[Vladimir Oltean]

On Mon, Jun 12, 2023 at 04:17:07PM +0300, Leon Romanovsky wrote:
> On Mon, Jun 12, 2023 at 03:37:18PM +0300, Vladimir Oltean wrote:
> > On Mon, Jun 12, 2023 at 02:59:25PM +0300, Leon Romanovsky wrote:
> > > As far as I can tell, the calls to .shutdown() and .remove() are
> > > mutually exclusive.
> > 
> > In this particular case, or in general?
> > 
> > In general they aren't. If the owning bus driver also implements its .shutdown()
> > as .remove(), then it will call the .remove() method of all devices on that bus.
> > That, after .shutdown() had already been called for those same children.
> 
> Can you please help me to see how? What is the call chain?
> 
> From what I see callback to ->shutdown() iterates over all devices in
> that bus and relevant bus will check that driver is bound prior to call
> to driver callback. In both cases, the driver is removed and bus won't
> call to already removed device.

The sequence of operations is:

* device_shutdown() walks the devices_kset backwards, thus shutting down
  children before parents
  * .shutdown() method of child gets called
  * .shutdown() method of parent gets called
    * parent implements .shutdown() as .remove()
      * the parent's .remove() logic calls device_del() on its children
        * .remove() method of child gets called

> If it does, it is arguably bug in bus logic, which needs to prevent such
> scenario.

It's just a consequence of how things work when you reuse the .remove() logic
for .shutdown() without thinking it through. It's a widespread pattern.

Re: [PATCH net v2 1/1] mlxbf_gige: Fix kernel panic at shutdown

[Leon Romanovsky]

On Mon, Jun 12, 2023 at 04:28:41PM +0300, Vladimir Oltean wrote:
> On Mon, Jun 12, 2023 at 04:17:07PM +0300, Leon Romanovsky wrote:
> > On Mon, Jun 12, 2023 at 03:37:18PM +0300, Vladimir Oltean wrote:
> > > On Mon, Jun 12, 2023 at 02:59:25PM +0300, Leon Romanovsky wrote:
> > > > As far as I can tell, the calls to .shutdown() and .remove() are
> > > > mutually exclusive.
> > > 
> > > In this particular case, or in general?
> > > 
> > > In general they aren't. If the owning bus driver also implements its .shutdown()
> > > as .remove(), then it will call the .remove() method of all devices on that bus.
> > > That, after .shutdown() had already been called for those same children.
> > 
> > Can you please help me to see how? What is the call chain?
> > 
> > From what I see callback to ->shutdown() iterates over all devices in
> > that bus and relevant bus will check that driver is bound prior to call
> > to driver callback. In both cases, the driver is removed and bus won't
> > call to already removed device.
> 
> The sequence of operations is:
> 
> * device_shutdown() walks the devices_kset backwards, thus shutting down
>   children before parents
>   * .shutdown() method of child gets called
>   * .shutdown() method of parent gets called
>     * parent implements .shutdown() as .remove()
>       * the parent's .remove() logic calls device_del() on its children
>         * .remove() method of child gets called

But both child and parent are locked so they parent can't call to
child's remove while child is performing shutdown.

BTW, I read the same device_shutdown() function before my first reply
and came to different conclusions from you.

> 
> > If it does, it is arguably bug in bus logic, which needs to prevent such
> > scenario.
> 
> It's just a consequence of how things work when you reuse the .remove() logic
> for .shutdown() without thinking it through. It's a widespread pattern.

Re: [PATCH net v2 1/1] mlxbf_gige: Fix kernel panic at shutdown

[Vladimir Oltean]

On Mon, Jun 12, 2023 at 04:38:53PM +0300, Leon Romanovsky wrote:
> On Mon, Jun 12, 2023 at 04:28:41PM +0300, Vladimir Oltean wrote:
> > The sequence of operations is:
> > 
> > * device_shutdown() walks the devices_kset backwards, thus shutting down
> >   children before parents
> >   * .shutdown() method of child gets called
> >   * .shutdown() method of parent gets called
> >     * parent implements .shutdown() as .remove()
> >       * the parent's .remove() logic calls device_del() on its children
> >         * .remove() method of child gets called
> 
> But both child and parent are locked so they parent can't call to
> child's remove while child is performing shutdown.

Please view the call chain I've posted in an email client capable of
showing the indentation correctly. The 2 lines:

   * .shutdown() method of child gets called
   * .shutdown() method of parent gets called

have the same level of indentation because they occur sequentially
within the same function.

This means 2 things:

1. when the parent runs its .shutdown(), the .shutdown() of the child
   has already finished

2. device_shutdown() only locks "dev" and "dev->parent" for the duration
   of the "dev->driver->shutdown(dev)" procedure. However, the situation
   that you deem impossible due to locking is the dev->driver->shutdown(dev)
   of the parent device. That parent wasn't found through any dev->parent
   pointer, instead it is just another device in the devices_kset list.
   The logic of locking "dev" and "dev->parent" there won't help, since
   we would be locking the parent and the parent of the parent. This
   will obviously not prevent the original parent from calling any
   method of the original child - we're already one step higher in the
   hierarchy.

So your objection above does not really apply.

> BTW, I read the same device_shutdown() function before my first reply
> and came to different conclusions from you.

Well, you could try to experiment with putting ".shutdown = xxx_remove,"
in some bus drivers and see what happens.

Admittedly it was a few years ago, but I did study this problem and I
did have to fix real issues reported by real people based on the above
observations (which here are reproduced only from memory):
https://lore.kernel.org/all/20210920214209.1733768-2-vladimir.oltean@nxp.com/

Re: [PATCH net v2 1/1] mlxbf_gige: Fix kernel panic at shutdown

[Jakub Kicinski]

On Thu, 8 Jun 2023 16:25:16 -0700 Samudrala, Sridhar wrote:
> >   static void mlxbf_gige_shutdown(struct platform_device *pdev)
> >   {
> > -	struct mlxbf_gige *priv = platform_get_drvdata(pdev);
> > -
> > -	writeq(0, priv->base + MLXBF_GIGE_INT_EN);
> > -	mlxbf_gige_clean_port(priv);
> > +	mlxbf_gige_remove(pdev);
> >   }  
> 
> With this change, do you really need mlxbf_gige_shutdown() as a separate 
> function as it is only calling mlxbf_gige_remove()?

Sounds like a fair ask.
-- 
pw-bot: cr

Re: [PATCH net v2 1/1] mlxbf_gige: Fix kernel panic at shutdown

[Leon Romanovsky]

On Mon, Jun 12, 2023 at 05:05:21PM +0300, Vladimir Oltean wrote:
> On Mon, Jun 12, 2023 at 04:38:53PM +0300, Leon Romanovsky wrote:
> > On Mon, Jun 12, 2023 at 04:28:41PM +0300, Vladimir Oltean wrote:
> > > The sequence of operations is:
> > > 
> > > * device_shutdown() walks the devices_kset backwards, thus shutting down
> > >   children before parents
> > >   * .shutdown() method of child gets called
> > >   * .shutdown() method of parent gets called
> > >     * parent implements .shutdown() as .remove()
> > >       * the parent's .remove() logic calls device_del() on its children
> > >         * .remove() method of child gets called
> > 
> > But both child and parent are locked so they parent can't call to
> > child's remove while child is performing shutdown.
> 
> Please view the call chain I've posted in an email client capable of
> showing the indentation correctly. 

Thanks for the suggestion, right now I'm using mutt and lore to read
emails. Should I use another email client?

> The 2 lines:
> 
>    * .shutdown() method of child gets called
>    * .shutdown() method of parent gets called
> 
> have the same level of indentation because they occur sequentially
> within the same function.

Right

> 
> This means 2 things:
> 
> 1. when the parent runs its .shutdown(), the .shutdown() of the child
>    has already finished

Right, it is done to make sure we release childs before parents.

> 
> 2. device_shutdown() only locks "dev" and "dev->parent" for the duration
>    of the "dev->driver->shutdown(dev)" procedure. However, the situation
>    that you deem impossible due to locking is the dev->driver->shutdown(dev)
>    of the parent device. That parent wasn't found through any dev->parent
>    pointer, instead it is just another device in the devices_kset list.
>    The logic of locking "dev" and "dev->parent" there won't help, since
>    we would be locking the parent and the parent of the parent. This
>    will obviously not prevent the original parent from calling any
>    method of the original child - we're already one step higher in the
>    hierarchy.

But once child finishes device_shutdown(), it will be removed from devices_kset
list and dev->driver should be NULL at that point for the child. In driver core,
dev->driver is the marker if driver is bound. It means parent/bus won't/shouldn't
call to anything driver related to child which doesn't have valid dev->driver pointer.

> 
> So your objection above does not really apply.

We have a different opinion here.

> 
> > BTW, I read the same device_shutdown() function before my first reply
> > and came to different conclusions from you.
> 
> Well, you could try to experiment with putting ".shutdown = xxx_remove,"
> in some bus drivers and see what happens.

Like I said, this is a bug in bus logic which allows calls to device
which doesn't have driver bound to it.

> 
> Admittedly it was a few years ago, but I did study this problem and I
> did have to fix real issues reported by real people based on the above
> observations (which here are reproduced only from memory):
> https://lore.kernel.org/all/20210920214209.1733768-2-vladimir.oltean@nxp.com/

I believe you, just think that behaviour found in i2c/spi isn't how
device model works.

Thanks

Re: [PATCH net v2 1/1] mlxbf_gige: Fix kernel panic at shutdown

[Vladimir Oltean]

On Tue, Jun 13, 2023 at 10:19:59AM +0300, Leon Romanovsky wrote:
> But once child finishes device_shutdown(), it will be removed from devices_kset
> list and dev->driver should be NULL at that point for the child.

What piece of code would make dev->driver be NULL for devices that have
been shut down by device_shutdown()?

Re: [PATCH net v2 1/1] mlxbf_gige: Fix kernel panic at shutdown

[Leon Romanovsky]

On Tue, Jun 13, 2023 at 11:30:02AM +0300, Vladimir Oltean wrote:
> On Tue, Jun 13, 2023 at 10:19:59AM +0300, Leon Romanovsky wrote:
> > But once child finishes device_shutdown(), it will be removed from devices_kset
> > list and dev->driver should be NULL at that point for the child.
> 
> What piece of code would make dev->driver be NULL for devices that have
> been shut down by device_shutdown()?

You are right here and I'm wrong on that point, dev->driver is set to
NULL in all other places where the device is going to be reused and not
in device_shutdown().

Unfortunately, it doesn't change a lot in our conversation, as device_shutdown()
is very specific call which is called in two flows: kernel_halt() and kernel_restart().

In both flows, it is end game.

Thanks

Re: [PATCH net v2 1/1] mlxbf_gige: Fix kernel panic at shutdown

[Vladimir Oltean]

On Tue, Jun 13, 2023 at 12:09:20PM +0300, Leon Romanovsky wrote:
> On Tue, Jun 13, 2023 at 11:30:02AM +0300, Vladimir Oltean wrote:
> > On Tue, Jun 13, 2023 at 10:19:59AM +0300, Leon Romanovsky wrote:
> > > But once child finishes device_shutdown(), it will be removed from devices_kset
> > > list and dev->driver should be NULL at that point for the child.
> > 
> > What piece of code would make dev->driver be NULL for devices that have
> > been shut down by device_shutdown()?
> 
> You are right here and I'm wrong on that point, dev->driver is set to
> NULL in all other places where the device is going to be reused and not
> in device_shutdown().
> 
> Unfortunately, it doesn't change a lot in our conversation, as device_shutdown()
> is very specific call which is called in two flows: kernel_halt() and kernel_restart().
> 
> In both flows, it is end game.
> 
> Thanks

Except for the fact that, as mentioned in my first reply to this thread,
bus drivers may implement .shutdown() the same way as .remove(), so in
that case, someone *will* unbind the drivers from those child devices,
*after* .shutdown() was called on the child - and if the child device
driver isn't prepared to handle that, it can dereference NULL pointers
and bye bye reboot - the kernel hangs.

Not really sure where you're aiming with your replies at this stage.

Re: [PATCH net v2 1/1] mlxbf_gige: Fix kernel panic at shutdown

[Leon Romanovsky]

On Tue, Jun 13, 2023 at 12:35:01PM +0300, Vladimir Oltean wrote:
> On Tue, Jun 13, 2023 at 12:09:20PM +0300, Leon Romanovsky wrote:
> > On Tue, Jun 13, 2023 at 11:30:02AM +0300, Vladimir Oltean wrote:
> > > On Tue, Jun 13, 2023 at 10:19:59AM +0300, Leon Romanovsky wrote:
> > > > But once child finishes device_shutdown(), it will be removed from devices_kset
> > > > list and dev->driver should be NULL at that point for the child.
> > > 
> > > What piece of code would make dev->driver be NULL for devices that have
> > > been shut down by device_shutdown()?
> > 
> > You are right here and I'm wrong on that point, dev->driver is set to
> > NULL in all other places where the device is going to be reused and not
> > in device_shutdown().
> > 
> > Unfortunately, it doesn't change a lot in our conversation, as device_shutdown()
> > is very specific call which is called in two flows: kernel_halt() and kernel_restart().
> > 
> > In both flows, it is end game.
> > 
> > Thanks
> 
> Except for the fact that, as mentioned in my first reply to this thread,
> bus drivers may implement .shutdown() the same way as .remove(), so in
> that case, someone *will* unbind the drivers from those child devices,
> *after* .shutdown() was called on the child - and if the child device
> driver isn't prepared to handle that, it can dereference NULL pointers
> and bye bye reboot - the kernel hangs.
> 
> Not really sure where you're aiming with your replies at this stage.

My goal is to explain that "bus drivers may implement .shutdown() 
the same way as .remove()" is wrong implementation and expectation
that all drivers will add "if (!priv) return ..." now is not viable.

Thanks

Re: [PATCH net v2 1/1] mlxbf_gige: Fix kernel panic at shutdown

[Vladimir Oltean]

On Tue, Jun 13, 2023 at 01:10:38PM +0300, Leon Romanovsky wrote:
> On Tue, Jun 13, 2023 at 12:35:01PM +0300, Vladimir Oltean wrote:
> > Not really sure where you're aiming with your replies at this stage.
> 
> My goal is to explain that "bus drivers may implement .shutdown() 
> the same way as .remove()" is wrong implementation and expectation
> that all drivers will add "if (!priv) return ..." now is not viable.

I never said that all drivers should guard against that - just that it's
possible and that there is no mechanism to reject such a thing - which
is something you've incorrectly claimed.

The top-level platform bus for MMIO devices should not be do this, at
least as of now - so drivers written for just that should be fine.
However, platform devices created and probed by other drivers, such as
MFD, are worth double-checking.

Re: [PATCH net v2 1/1] mlxbf_gige: Fix kernel panic at shutdown

[Leon Romanovsky]

On Tue, Jun 13, 2023 at 01:34:22PM +0300, Vladimir Oltean wrote:
> On Tue, Jun 13, 2023 at 01:10:38PM +0300, Leon Romanovsky wrote:
> > On Tue, Jun 13, 2023 at 12:35:01PM +0300, Vladimir Oltean wrote:
> > > Not really sure where you're aiming with your replies at this stage.
> > 
> > My goal is to explain that "bus drivers may implement .shutdown() 
> > the same way as .remove()" is wrong implementation and expectation
> > that all drivers will add "if (!priv) return ..." now is not viable.
> 
> I never said that all drivers should guard against that - just that it's
> possible and that there is no mechanism to reject such a thing - which
> is something you've incorrectly claimed.

I was wrong in details, but in general I was correct by saying that call
to .shutdown() and .remove() callbacks are impossible to be performed
at the same time.

https://lore.kernel.org/all/20230612115925.GR12152@unreal

Thanks

Re: [PATCH net v2 1/1] mlxbf_gige: Fix kernel panic at shutdown

[Vladimir Oltean]

On Tue, Jun 13, 2023 at 02:28:41PM +0300, Leon Romanovsky wrote:
> On Tue, Jun 13, 2023 at 01:34:22PM +0300, Vladimir Oltean wrote:
> > On Tue, Jun 13, 2023 at 01:10:38PM +0300, Leon Romanovsky wrote:
> > > On Tue, Jun 13, 2023 at 12:35:01PM +0300, Vladimir Oltean wrote:
> > > > Not really sure where you're aiming with your replies at this stage.
> > > 
> > > My goal is to explain that "bus drivers may implement .shutdown() 
> > > the same way as .remove()" is wrong implementation and expectation
> > > that all drivers will add "if (!priv) return ..." now is not viable.
> > 
> > I never said that all drivers should guard against that - just that it's
> > possible and that there is no mechanism to reject such a thing - which
> > is something you've incorrectly claimed.
> 
> I was wrong in details, but in general I was correct by saying that call
> to .shutdown() and .remove() callbacks are impossible to be performed
> at the same time.
> 
> https://lore.kernel.org/all/20230612115925.GR12152@unreal
> 
> Thanks

Let's stop the conversation here, it is going nowhere. You've given me a
link to a message to which I've responded with exactly the same text as
I would respond now.