[PATCH RESEND] mtd:spi-nor: Fix an issue of releasing resources during read/write

[PATCH RESEND] mtd:spi-nor: Fix an issue of releasing resources during read/write

[Yicong Yang]

From: Xiang Chen <chenxiang66@hisilicon.com>

If rmmod the driver during read or write, the driver will release the
resources which are used during read or write, so it is possible to
refer to NULL pointer.

Use the testcase "mtd_debug read /dev/mtd0 0xc00000 0x400000 dest_file &
sleep 0.5;rmmod spi_hisi_sfc_v3xx.ko", the issue can be reproduced in
hisi_sfc_v3xx driver.

To avoid the issue, fill the interface _get_device and _put_device of
mtd_info to grab the reference to the spi controller driver module, so
the request of rmmod the driver is rejected before read/write is finished.

Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/mtd/spi-nor/core.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 0522304..72bc134 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3301,6 +3301,37 @@ static void spi_nor_resume(struct mtd_info *mtd)
 		dev_err(dev, "resume() failed\n");
 }
 
+static int spi_nor_get_device(struct mtd_info *mtd)
+{
+	struct mtd_info *master = mtd_get_master(mtd);
+	struct spi_nor *nor = mtd_to_spi_nor(master);
+	struct device *dev;
+
+	if (nor->spimem)
+		dev = nor->spimem->spi->controller->dev.parent;
+	else
+		dev = nor->dev;
+
+	if (!try_module_get(dev->driver->owner))
+		return -ENODEV;
+
+	return 0;
+}
+
+static void spi_nor_put_device(struct mtd_info *mtd)
+{
+	struct mtd_info *master = mtd_get_master(mtd);
+	struct spi_nor *nor = mtd_to_spi_nor(master);
+	struct device *dev;
+
+	if (nor->spimem)
+		dev = nor->spimem->spi->controller->dev.parent;
+	else
+		dev = nor->dev;
+
+	module_put(dev->driver->owner);
+}
+
 void spi_nor_restore(struct spi_nor *nor)
 {
 	/* restore the addressing mode */
@@ -3495,6 +3526,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 	mtd->_read = spi_nor_read;
 	mtd->_suspend = spi_nor_suspend;
 	mtd->_resume = spi_nor_resume;
+	mtd->_get_device = spi_nor_get_device;
+	mtd->_put_device = spi_nor_put_device;
 
 	if (nor->params->locking_ops) {
 		mtd->_lock = spi_nor_lock;
-- 
2.8.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RESEND] mtd:spi-nor: Fix an issue of releasing resources during read/write

[Michael Walle]

Hi,

nit: subject should be "mtd: spi-nor: Fix.."

Am 2021-04-01 09:34, schrieb Yicong Yang:
> From: Xiang Chen <chenxiang66@hisilicon.com>
> 
> If rmmod the driver during read or write, the driver will release the
> resources which are used during read or write, so it is possible to
> refer to NULL pointer.
> 
> Use the testcase "mtd_debug read /dev/mtd0 0xc00000 0x400000 dest_file 
> &
> sleep 0.5;rmmod spi_hisi_sfc_v3xx.ko", the issue can be reproduced in
> hisi_sfc_v3xx driver.
> 
> To avoid the issue, fill the interface _get_device and _put_device of
> mtd_info to grab the reference to the spi controller driver module, so
> the request of rmmod the driver is rejected before read/write is 
> finished.
> 
> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>

Reviewed-by: Michael Walle <michael@walle.cc>
Tested-by: Michael Walle <michael@walle.cc> [with spi-nxp-fspi and 
spi-fsl-dspi]

btw. you can also do:
lsmod
exec 3<> /dev/mtd0
lsmod

and have a look at the ref count.

-michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RESEND] mtd:spi-nor: Fix an issue of releasing resources during read/write

[Yicong Yang]

On 2021/4/7 22:50, Michael Walle wrote:
> Hi,
> 
> nit: subject should be "mtd: spi-nor: Fix.."
> 

thanks. maybe Tudor can get this fixed, or i'll
send a v2 one.

> Am 2021-04-01 09:34, schrieb Yicong Yang:
>> From: Xiang Chen <chenxiang66@hisilicon.com>
>>
>> If rmmod the driver during read or write, the driver will release the
>> resources which are used during read or write, so it is possible to
>> refer to NULL pointer.
>>
>> Use the testcase "mtd_debug read /dev/mtd0 0xc00000 0x400000 dest_file &
>> sleep 0.5;rmmod spi_hisi_sfc_v3xx.ko", the issue can be reproduced in
>> hisi_sfc_v3xx driver.
>>
>> To avoid the issue, fill the interface _get_device and _put_device of
>> mtd_info to grab the reference to the spi controller driver module, so
>> the request of rmmod the driver is rejected before read/write is finished.
>>
>> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> 
> Reviewed-by: Michael Walle <michael@walle.cc>
> Tested-by: Michael Walle <michael@walle.cc> [with spi-nxp-fspi and spi-fsl-dspi]
> 
> btw. you can also do:
> lsmod
> exec 3<> /dev/mtd0
> lsmod
> 
> and have a look at the ref count.
> 

yes, it works. the ref count are get by the open() callback. thanks for the hint.

thanks
Yicong

> -michael
> 
> .


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RESEND] mtd:spi-nor: Fix an issue of releasing resources during read/write

[Tudor.Ambarus]

On 4/1/21 10:34 AM, Yicong Yang wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> From: Xiang Chen <chenxiang66@hisilicon.com>
> 
> If rmmod the driver during read or write, the driver will release the
> resources which are used during read or write, so it is possible to
> refer to NULL pointer.
> 
> Use the testcase "mtd_debug read /dev/mtd0 0xc00000 0x400000 dest_file &
> sleep 0.5;rmmod spi_hisi_sfc_v3xx.ko", the issue can be reproduced in
> hisi_sfc_v3xx driver.
> 
> To avoid the issue, fill the interface _get_device and _put_device of
> mtd_info to grab the reference to the spi controller driver module, so
> the request of rmmod the driver is rejected before read/write is finished.
> 

Do we care for a Fixes tag and a Cc to stable? I guess this bug is present
since the introduction of the driver.

> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>

Tested-by: Tudor Ambarus <tudor.ambarus@microchip.com>

Will fix the subject of the commit when applying.

Thanks,
ta
> ---
>  drivers/mtd/spi-nor/core.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 0522304..72bc134 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3301,6 +3301,37 @@ static void spi_nor_resume(struct mtd_info *mtd)
>                 dev_err(dev, "resume() failed\n");
>  }
> 
> +static int spi_nor_get_device(struct mtd_info *mtd)
> +{
> +       struct mtd_info *master = mtd_get_master(mtd);
> +       struct spi_nor *nor = mtd_to_spi_nor(master);
> +       struct device *dev;
> +
> +       if (nor->spimem)
> +               dev = nor->spimem->spi->controller->dev.parent;
> +       else
> +               dev = nor->dev;
> +
> +       if (!try_module_get(dev->driver->owner))
> +               return -ENODEV;
> +
> +       return 0;
> +}
> +
> +static void spi_nor_put_device(struct mtd_info *mtd)
> +{
> +       struct mtd_info *master = mtd_get_master(mtd);
> +       struct spi_nor *nor = mtd_to_spi_nor(master);
> +       struct device *dev;
> +
> +       if (nor->spimem)
> +               dev = nor->spimem->spi->controller->dev.parent;
> +       else
> +               dev = nor->dev;
> +
> +       module_put(dev->driver->owner);
> +}
> +
>  void spi_nor_restore(struct spi_nor *nor)
>  {
>         /* restore the addressing mode */
> @@ -3495,6 +3526,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>         mtd->_read = spi_nor_read;
>         mtd->_suspend = spi_nor_suspend;
>         mtd->_resume = spi_nor_resume;
> +       mtd->_get_device = spi_nor_get_device;
> +       mtd->_put_device = spi_nor_put_device;
> 
>         if (nor->params->locking_ops) {
>                 mtd->_lock = spi_nor_lock;
> --
> 2.8.1
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RESEND] mtd:spi-nor: Fix an issue of releasing resources during read/write

[Yicong Yang]

On 2021/4/9 11:21, Tudor.Ambarus@microchip.com wrote:
> On 4/1/21 10:34 AM, Yicong Yang wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> From: Xiang Chen <chenxiang66@hisilicon.com>
>>
>> If rmmod the driver during read or write, the driver will release the
>> resources which are used during read or write, so it is possible to
>> refer to NULL pointer.
>>
>> Use the testcase "mtd_debug read /dev/mtd0 0xc00000 0x400000 dest_file &
>> sleep 0.5;rmmod spi_hisi_sfc_v3xx.ko", the issue can be reproduced in
>> hisi_sfc_v3xx driver.
>>
>> To avoid the issue, fill the interface _get_device and _put_device of
>> mtd_info to grab the reference to the spi controller driver module, so
>> the request of rmmod the driver is rejected before read/write is finished.
>>
> 
> Do we care for a Fixes tag and a Cc to stable? I guess this bug is present
> since the introduction of the driver.
> 

yes. I cannot find a proper fix tag. so perhaps:

  Cc: stable@vger.kernel.org

>> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> 
> Tested-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> Will fix the subject of the commit when applying.>

thanks!

> Thanks,
> ta
>> ---
>>  drivers/mtd/spi-nor/core.c | 33 +++++++++++++++++++++++++++++++++
>>  1 file changed, 33 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 0522304..72bc134 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -3301,6 +3301,37 @@ static void spi_nor_resume(struct mtd_info *mtd)
>>                 dev_err(dev, "resume() failed\n");
>>  }
>>
>> +static int spi_nor_get_device(struct mtd_info *mtd)
>> +{
>> +       struct mtd_info *master = mtd_get_master(mtd);
>> +       struct spi_nor *nor = mtd_to_spi_nor(master);
>> +       struct device *dev;
>> +
>> +       if (nor->spimem)
>> +               dev = nor->spimem->spi->controller->dev.parent;
>> +       else
>> +               dev = nor->dev;
>> +
>> +       if (!try_module_get(dev->driver->owner))
>> +               return -ENODEV;
>> +
>> +       return 0;
>> +}
>> +
>> +static void spi_nor_put_device(struct mtd_info *mtd)
>> +{
>> +       struct mtd_info *master = mtd_get_master(mtd);
>> +       struct spi_nor *nor = mtd_to_spi_nor(master);
>> +       struct device *dev;
>> +
>> +       if (nor->spimem)
>> +               dev = nor->spimem->spi->controller->dev.parent;
>> +       else
>> +               dev = nor->dev;
>> +
>> +       module_put(dev->driver->owner);
>> +}
>> +
>>  void spi_nor_restore(struct spi_nor *nor)
>>  {
>>         /* restore the addressing mode */
>> @@ -3495,6 +3526,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>>         mtd->_read = spi_nor_read;
>>         mtd->_suspend = spi_nor_suspend;
>>         mtd->_resume = spi_nor_resume;
>> +       mtd->_get_device = spi_nor_get_device;
>> +       mtd->_put_device = spi_nor_put_device;
>>
>>         if (nor->params->locking_ops) {
>>                 mtd->_lock = spi_nor_lock;
>> --
>> 2.8.1
>>
> 


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RESEND] mtd:spi-nor: Fix an issue of releasing resources during read/write

[Tudor.Ambarus]

On 4/9/21 7:03 AM, Yicong Yang wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 2021/4/9 11:21, Tudor.Ambarus@microchip.com wrote:
>> On 4/1/21 10:34 AM, Yicong Yang wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> From: Xiang Chen <chenxiang66@hisilicon.com>
>>>
>>> If rmmod the driver during read or write, the driver will release the
>>> resources which are used during read or write, so it is possible to
>>> refer to NULL pointer.
>>>
>>> Use the testcase "mtd_debug read /dev/mtd0 0xc00000 0x400000 dest_file &
>>> sleep 0.5;rmmod spi_hisi_sfc_v3xx.ko", the issue can be reproduced in
>>> hisi_sfc_v3xx driver.
>>>
>>> To avoid the issue, fill the interface _get_device and _put_device of
>>> mtd_info to grab the reference to the spi controller driver module, so
>>> the request of rmmod the driver is rejected before read/write is finished.
>>>
>>
>> Do we care for a Fixes tag and a Cc to stable? I guess this bug is present
>> since the introduction of the driver.
>>
> 
> yes. I cannot find a proper fix tag. so perhaps:

Fixes: b199489d37b2 ("mtd: spi-nor: add the framework for SPI NOR")
should be fine.

> 
>   Cc: stable@vger.kernel.org
> 
>>> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
>>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>>
>> Tested-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>>
>> Will fix the subject of the commit when applying.>
> 
> thanks!
> 
>> Thanks,
>> ta
>>> ---
>>>  drivers/mtd/spi-nor/core.c | 33 +++++++++++++++++++++++++++++++++
>>>  1 file changed, 33 insertions(+)
>>>
>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>> index 0522304..72bc134 100644
>>> --- a/drivers/mtd/spi-nor/core.c
>>> +++ b/drivers/mtd/spi-nor/core.c
>>> @@ -3301,6 +3301,37 @@ static void spi_nor_resume(struct mtd_info *mtd)
>>>                 dev_err(dev, "resume() failed\n");
>>>  }
>>>
>>> +static int spi_nor_get_device(struct mtd_info *mtd)
>>> +{
>>> +       struct mtd_info *master = mtd_get_master(mtd);
>>> +       struct spi_nor *nor = mtd_to_spi_nor(master);
>>> +       struct device *dev;
>>> +
>>> +       if (nor->spimem)
>>> +               dev = nor->spimem->spi->controller->dev.parent;
>>> +       else
>>> +               dev = nor->dev;
>>> +
>>> +       if (!try_module_get(dev->driver->owner))
>>> +               return -ENODEV;
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static void spi_nor_put_device(struct mtd_info *mtd)
>>> +{
>>> +       struct mtd_info *master = mtd_get_master(mtd);
>>> +       struct spi_nor *nor = mtd_to_spi_nor(master);
>>> +       struct device *dev;
>>> +
>>> +       if (nor->spimem)
>>> +               dev = nor->spimem->spi->controller->dev.parent;
>>> +       else
>>> +               dev = nor->dev;
>>> +
>>> +       module_put(dev->driver->owner);
>>> +}
>>> +
>>>  void spi_nor_restore(struct spi_nor *nor)
>>>  {
>>>         /* restore the addressing mode */
>>> @@ -3495,6 +3526,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>>>         mtd->_read = spi_nor_read;
>>>         mtd->_suspend = spi_nor_suspend;
>>>         mtd->_resume = spi_nor_resume;
>>> +       mtd->_get_device = spi_nor_get_device;
>>> +       mtd->_put_device = spi_nor_put_device;
>>>
>>>         if (nor->params->locking_ops) {
>>>                 mtd->_lock = spi_nor_lock;
>>> --
>>> 2.8.1
>>>
>>
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RESEND] mtd:spi-nor: Fix an issue of releasing resources during read/write

[Yicong Yang]

On 2021/4/9 12:13, Tudor.Ambarus@microchip.com wrote:
> On 4/9/21 7:03 AM, Yicong Yang wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 2021/4/9 11:21, Tudor.Ambarus@microchip.com wrote:
>>> On 4/1/21 10:34 AM, Yicong Yang wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> From: Xiang Chen <chenxiang66@hisilicon.com>
>>>>
>>>> If rmmod the driver during read or write, the driver will release the
>>>> resources which are used during read or write, so it is possible to
>>>> refer to NULL pointer.
>>>>
>>>> Use the testcase "mtd_debug read /dev/mtd0 0xc00000 0x400000 dest_file &
>>>> sleep 0.5;rmmod spi_hisi_sfc_v3xx.ko", the issue can be reproduced in
>>>> hisi_sfc_v3xx driver.
>>>>
>>>> To avoid the issue, fill the interface _get_device and _put_device of
>>>> mtd_info to grab the reference to the spi controller driver module, so
>>>> the request of rmmod the driver is rejected before read/write is finished.
>>>>
>>>
>>> Do we care for a Fixes tag and a Cc to stable? I guess this bug is present
>>> since the introduction of the driver.
>>>
>>
>> yes. I cannot find a proper fix tag. so perhaps:
> 
> Fixes: b199489d37b2 ("mtd: spi-nor: add the framework for SPI NOR")
> should be fine.
> 

it does make sense. please add that and the Cc stable when applying. thanks.

>>
>>   Cc: stable@vger.kernel.org
>>
>>>> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
>>>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>>>
>>> Tested-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>>>
>>> Will fix the subject of the commit when applying.>
>>
>> thanks!
>>
>>> Thanks,
>>> ta
>>>> ---
>>>>  drivers/mtd/spi-nor/core.c | 33 +++++++++++++++++++++++++++++++++
>>>>  1 file changed, 33 insertions(+)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>> index 0522304..72bc134 100644
>>>> --- a/drivers/mtd/spi-nor/core.c
>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>> @@ -3301,6 +3301,37 @@ static void spi_nor_resume(struct mtd_info *mtd)
>>>>                 dev_err(dev, "resume() failed\n");
>>>>  }
>>>>
>>>> +static int spi_nor_get_device(struct mtd_info *mtd)
>>>> +{
>>>> +       struct mtd_info *master = mtd_get_master(mtd);
>>>> +       struct spi_nor *nor = mtd_to_spi_nor(master);
>>>> +       struct device *dev;
>>>> +
>>>> +       if (nor->spimem)
>>>> +               dev = nor->spimem->spi->controller->dev.parent;
>>>> +       else
>>>> +               dev = nor->dev;
>>>> +
>>>> +       if (!try_module_get(dev->driver->owner))
>>>> +               return -ENODEV;
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static void spi_nor_put_device(struct mtd_info *mtd)
>>>> +{
>>>> +       struct mtd_info *master = mtd_get_master(mtd);
>>>> +       struct spi_nor *nor = mtd_to_spi_nor(master);
>>>> +       struct device *dev;
>>>> +
>>>> +       if (nor->spimem)
>>>> +               dev = nor->spimem->spi->controller->dev.parent;
>>>> +       else
>>>> +               dev = nor->dev;
>>>> +
>>>> +       module_put(dev->driver->owner);
>>>> +}
>>>> +
>>>>  void spi_nor_restore(struct spi_nor *nor)
>>>>  {
>>>>         /* restore the addressing mode */
>>>> @@ -3495,6 +3526,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>>>>         mtd->_read = spi_nor_read;
>>>>         mtd->_suspend = spi_nor_suspend;
>>>>         mtd->_resume = spi_nor_resume;
>>>> +       mtd->_get_device = spi_nor_get_device;
>>>> +       mtd->_put_device = spi_nor_put_device;
>>>>
>>>>         if (nor->params->locking_ops) {
>>>>                 mtd->_lock = spi_nor_lock;
>>>> --
>>>> 2.8.1
>>>>
>>>
>>
> 


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH RESEND] mtd:spi-nor: Fix an issue of releasing resources during read/write

[Tudor Ambarus]

On Thu, 1 Apr 2021 15:34:46 +0800, Yicong Yang wrote:
> If rmmod the driver during read or write, the driver will release the
> resources which are used during read or write, so it is possible to
> refer to NULL pointer.
> 
> Use the testcase "mtd_debug read /dev/mtd0 0xc00000 0x400000 dest_file &
> sleep 0.5;rmmod spi_hisi_sfc_v3xx.ko", the issue can be reproduced in
> hisi_sfc_v3xx driver.
> 
> [...]

Updated subject, added fixes tag and cc to stable.
Applied to spi-nor/next, thanks!

[1/1] mtd:spi-nor: Fix an issue of releasing resources during read/write
      https://git.kernel.org/mtd/c/be94215be1ab

Best regards,
-- 
Tudor Ambarus <tudor.ambarus@microchip.com>

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/