[PATCH v2] block: mtip32xx: Prioritize state cleanup over memory freeing in the mtip_pci_probe error path.

[PATCH v2] block: mtip32xx: Prioritize state cleanup over memory freeing in the mtip_pci_probe error path.

[Zhang Heng]

The original sequence kfree(dd); pci_set_drvdata(pdev, NULL); creates a
theoretical race condition window. Between these two calls, the pci_dev
structure retains a dangling pointer to the already-freed device private
data (dd). Any concurrent access to the drvdata (e.g., from an interrupt
handler or an unexpected call to remove) would lead to a use-after-free
kernel oops.

Changes made:
1. `pci_set_drvdata(pdev, NULL);` - First, atomically sever the link
from the pci_dev.
2. `kfree(dd);` - Then, safely free the private memory.

This ensures the kernel state is always consistent before resources
are released, adhering to defensive programming principles.

Signed-off-by: Zhang Heng <zhangheng@kylinos.cn>
---
 drivers/block/mtip32xx/mtip32xx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c
index 8fc7761397bd..f228363e6b1c 100644
--- a/drivers/block/mtip32xx/mtip32xx.c
+++ b/drivers/block/mtip32xx/mtip32xx.c
@@ -3839,9 +3839,8 @@ static int mtip_pci_probe(struct pci_dev *pdev,
 	}
 
 iomap_err:
-	kfree(dd);
 	pci_set_drvdata(pdev, NULL);
-	return rv;
+	kfree(dd);
 done:
 	return rv;
 }
-- 
2.47.1

Re: [PATCH v2] block: mtip32xx: Prioritize state cleanup over memory freeing in the mtip_pci_probe error path.

[Andy Shevchenko]

On Sat, Aug 23, 2025 at 04:32:22PM +0800, Zhang Heng wrote:
> The original sequence kfree(dd); pci_set_drvdata(pdev, NULL); creates a
> theoretical race condition window. Between these two calls, the pci_dev
> structure retains a dangling pointer to the already-freed device private
> data (dd). Any concurrent access to the drvdata (e.g., from an interrupt
> handler or an unexpected call to remove) would lead to a use-after-free
> kernel oops.
> 
> Changes made:
> 1. `pci_set_drvdata(pdev, NULL);` - First, atomically sever the link
> from the pci_dev.
> 2. `kfree(dd);` - Then, safely free the private memory.
> 
> This ensures the kernel state is always consistent before resources
> are released, adhering to defensive programming principles.

...

>  iomap_err:
> -	kfree(dd);
>  	pci_set_drvdata(pdev, NULL);
> -	return rv;
> +	kfree(dd);

These two seems to me unrelated. How do you possible have a race? What's racy
there? (Yes, I have read the commit message, but I fail to see how it may lead
to anything here. My question in one of the previous patches was about needless
pci_set_drvdata() call. Do we even need that one?

>  done:
>  	return rv;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] block: mtip32xx: Prioritize state cleanup over memory freeing in the mtip_pci_probe error path.

[Andy Shevchenko]

On Mon, Aug 25, 2025 at 01:56:55PM +0300, Andy Shevchenko wrote:
> On Sat, Aug 23, 2025 at 04:32:22PM +0800, Zhang Heng wrote:
> > The original sequence kfree(dd); pci_set_drvdata(pdev, NULL); creates a
> > theoretical race condition window. Between these two calls, the pci_dev
> > structure retains a dangling pointer to the already-freed device private
> > data (dd). Any concurrent access to the drvdata (e.g., from an interrupt
> > handler or an unexpected call to remove) would lead to a use-after-free
> > kernel oops.
> > 
> > Changes made:
> > 1. `pci_set_drvdata(pdev, NULL);` - First, atomically sever the link
> > from the pci_dev.
> > 2. `kfree(dd);` - Then, safely free the private memory.
> > 
> > This ensures the kernel state is always consistent before resources
> > are released, adhering to defensive programming principles.

...

> >  iomap_err:
> > -	kfree(dd);
> >  	pci_set_drvdata(pdev, NULL);
> > -	return rv;
> > +	kfree(dd);
> 
> These two seems to me unrelated. How do you possible have a race? What's racy
> there? (Yes, I have read the commit message, but I fail to see how it may lead
> to anything here. My question in one of the previous patches was about needless
> pci_set_drvdata() call. Do we even need that one?
> 
> >  done:
> >  	return rv;

Also note, 99.99% of the drivers do that cleanup in the driver core whenever it
considers the best to do a such. So if you see an issue in this driver, the commit
message should really explain much more.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] block: mtip32xx: Prioritize state cleanup over memory freeing in the mtip_pci_probe error path.

[Philipp Stanner]

On Sat, 2025-08-23 at 16:32 +0800, Zhang Heng wrote:
> The original sequence kfree(dd); pci_set_drvdata(pdev, NULL); creates a
> theoretical race condition window. Between these two calls, the pci_dev
> structure retains a dangling pointer to the already-freed device private
> data (dd). Any concurrent access to the drvdata (e.g., from an interrupt
> handler or an unexpected call to remove) would lead to a use-after-free
> kernel oops.

I tend to disagree I think . This is the driver's probe() function. It
is always invoked serially by the driver core.

remove() cannot be called while probe() is still running.

Thus, the only potential explosion that could happen would be if an
interrupt handler were to be set up in this probe() and then accesses
dd.

However, if that were the case, everything would be exploding already
because there is no place in the code that tears down that interrupt
handler or other potential parallel accessors before kfree(dd) is
called.

In this case, the relevant call is pci_enable_msi(). Sooner errors,
like iomap_err: are jumped to before that's the case. Which is the only
sane design for probe() anyways. Otherwise, pci_disable_msi() switches
that off again; IOW: there are no racers.

So I think that the pci_set_drvdata(… NULL) can be removed
alltogether. 

When working on the probe() / remove() paths last and this year, I came
to believe that calls like that were often used because of a
misunderstanding of how the driver core APIs work.


P.


> 
> Changes made:
> 1. `pci_set_drvdata(pdev, NULL);` - First, atomically sever the link
> from the pci_dev.
> 2. `kfree(dd);` - Then, safely free the private memory.
> 
> This ensures the kernel state is always consistent before resources
> are released, adhering to defensive programming principles.
> 
> Signed-off-by: Zhang Heng <zhangheng@kylinos.cn>
> ---
>  drivers/block/mtip32xx/mtip32xx.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c
> index 8fc7761397bd..f228363e6b1c 100644
> --- a/drivers/block/mtip32xx/mtip32xx.c
> +++ b/drivers/block/mtip32xx/mtip32xx.c
> @@ -3839,9 +3839,8 @@ static int mtip_pci_probe(struct pci_dev *pdev,
>  	}
>  
>  iomap_err:
> -	kfree(dd);
>  	pci_set_drvdata(pdev, NULL);
> -	return rv;
> +	kfree(dd);
>  done:
>  	return rv;
>  }

Re: [PATCH v2] block: mtip32xx: Prioritize state cleanup over memory freeing in the mtip_pci_probe error path.

[Andy Shevchenko]

On Mon, Aug 25, 2025 at 01:33:17PM +0200, Philipp Stanner wrote:
> On Sat, 2025-08-23 at 16:32 +0800, Zhang Heng wrote:

...

> So I think that the pci_set_drvdata(… NULL) can be removed
> alltogether. 
> 
> When working on the probe() / remove() paths last and this year, I came
> to believe that calls like that were often used because of a
> misunderstanding of how the driver core APIs work.

I think there are other aspects that makes this happen (any combination of them
possible):

1) old books for Linux kernel development with outdated examples (these calls
   used to be required ca. 2010);

2) initial driver development based on (quite) old examples;

3) (as you said) misunderstanding of the device enumeration process in Linux
device model;

4) ...anything else I forgot...

-- 
With Best Regards,
Andy Shevchenko