| Message ID | 1617262486-4223-1-git-send-email-yangyicong@hisilicon.com |
|---|---|
| State | Accepted |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | [RESEND] mtd:spi-nor: Fix an issue of releasing resources during read/write | expand |
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
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 > > .
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 >
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 >> >
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 >>> >> >
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 >>>> >>> >> >
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,
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;