* Re: [PATCH] mtd: fix: avoid race condition when accessing mtd->usecount [not found] ` <20150508001745.GW32500@ld-irv-0074> @ 2015-05-08 0:26 ` Brian Norris 2015-05-11 7:44 ` Cantavenera, Giuseppe (EXT-Other - DE/Ulm) 2015-05-12 16:37 ` Alexander Sverdlin 0 siblings, 2 replies; 6+ messages in thread From: Brian Norris @ 2015-05-08 0:26 UTC (permalink / raw) To: Giuseppe Cantavenera Cc: linux-mtd, lorenzo.restelli.ext, dwmw2, stable, alexander.sverdlin, zhangxingcai, fengfuqiu, tanhaijun, linux-kernel On Thu, May 07, 2015 at 05:17:45PM -0700, Brian Norris wrote: > On Thu, May 07, 2015 at 05:10:12PM -0700, Brian Norris wrote: > > On Tue, Apr 21, 2015 at 12:20:22PM +0200, Giuseppe Cantavenera wrote: > > > @@ -484,7 +486,7 @@ int del_mtd_blktrans_dev(struct mtd_blktrans_dev *old) > > > if (old->open) { > > > if (old->tr->release) > > > old->tr->release(old); > > > - __put_mtd_device(old->mtd); > > > + put_mtd_device(old->mtd); > > > > This looks wrong. See: > [...] > > deregister_mtd_blktrans() > > |_ mutex_lock(&mtd_table_mutex) > > |_ tr->remove_dev() -> inftl_remove_dev() > > |_ del_mtd_blktrans_dev() > > |_ put_mtd_device() > > |_ mutex_lock(&mtd_table_mutex) <--- AA deadlock > > What's more, this code in del_mtd_blktrans_dev() makes it obvious that > this hunk is wrong: > > int del_mtd_blktrans_dev(struct mtd_blktrans_dev *old) > { > unsigned long flags; > > if (mutex_trylock(&mtd_table_mutex)) { > mutex_unlock(&mtd_table_mutex); > BUG(); > } > ... > > So rather than a comment, the code is showing that it's a BUG() to not > be holding mtd_table_mutex already. As an alternative to your patch, how about the following? BTW, this does still leave a usecount race in drivers/mtd/maps/vmu-flash.c. But that driver should really be using mtd->_get_device(), if it actually wants its own refcount. Signed-off-by: Brian Norris <computersforpeace@gmail.com> --- diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c index 2b0c52870999..df7c6c70757a 100644 --- a/drivers/mtd/mtd_blkdevs.c +++ b/drivers/mtd/mtd_blkdevs.c @@ -197,6 +197,7 @@ static int blktrans_open(struct block_device *bdev, fmode_t mode) return -ERESTARTSYS; /* FIXME: busy loop! -arnd*/ mutex_lock(&dev->lock); + mutex_lock(&mtd_table_mutex); if (dev->open) goto unlock; @@ -220,6 +221,7 @@ static int blktrans_open(struct block_device *bdev, fmode_t mode) unlock: dev->open++; + mutex_unlock(&mtd_table_mutex); mutex_unlock(&dev->lock); blktrans_dev_put(dev); return ret; @@ -230,6 +232,7 @@ error_release: error_put: module_put(dev->tr->owner); kref_put(&dev->ref, blktrans_dev_release); + mutex_unlock(&mtd_table_mutex); mutex_unlock(&dev->lock); blktrans_dev_put(dev); return ret; @@ -243,6 +246,7 @@ static void blktrans_release(struct gendisk *disk, fmode_t mode) return; mutex_lock(&dev->lock); + mutex_lock(&mtd_table_mutex); if (--dev->open) goto unlock; @@ -256,6 +260,7 @@ static void blktrans_release(struct gendisk *disk, fmode_t mode) __put_mtd_device(dev->mtd); } unlock: + mutex_unlock(&mtd_table_mutex); mutex_unlock(&dev->lock); blktrans_dev_put(dev); } ^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCH] mtd: fix: avoid race condition when accessing mtd->usecount 2015-05-08 0:26 ` [PATCH] mtd: fix: avoid race condition when accessing mtd->usecount Brian Norris @ 2015-05-11 7:44 ` Cantavenera, Giuseppe (EXT-Other - DE/Ulm) 2015-05-11 22:25 ` Brian Norris 2015-05-12 16:37 ` Alexander Sverdlin 1 sibling, 1 reply; 6+ messages in thread From: Cantavenera, Giuseppe (EXT-Other - DE/Ulm) @ 2015-05-11 7:44 UTC (permalink / raw) To: ext Brian Norris Cc: linux-mtd@lists.infradead.org, Restelli, Lorenzo (EXT-Other - DE/Ulm), dwmw2@infradead.org, stable@vger.kernel.org, Sverdlin, Alexander (Nokia - DE/Ulm), zhangxingcai, fengfuqiu@huawei.com, tanhaijun@huawei.com, linux-kernel@vger.kernel.org > -----Original Message----- > From: ext Brian Norris [mailto:computersforpeace@gmail.com] > Sent: Friday, May 08, 2015 2:27 AM > To: Cantavenera, Giuseppe (EXT-Other - DE/Ulm) > Cc: linux-mtd@lists.infradead.org; Restelli, Lorenzo (EXT-Other - > DE/Ulm); dwmw2@infradead.org; stable@vger.kernel.org; Sverdlin, > Alexander (Nokia - DE/Ulm); zhangxingcai; fengfuqiu@huawei.com; > tanhaijun@huawei.com; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] mtd: fix: avoid race condition when accessing mtd- > >usecount > > On Thu, May 07, 2015 at 05:17:45PM -0700, Brian Norris wrote: > > On Thu, May 07, 2015 at 05:10:12PM -0700, Brian Norris wrote: > > > On Tue, Apr 21, 2015 at 12:20:22PM +0200, Giuseppe Cantavenera > wrote: > > > > @@ -484,7 +486,7 @@ int del_mtd_blktrans_dev(struct > mtd_blktrans_dev *old) > > > > if (old->open) { > > > > if (old->tr->release) > > > > old->tr->release(old); > > > > - __put_mtd_device(old->mtd); > > > > + put_mtd_device(old->mtd); > > > > > > This looks wrong. See: > > [...] > > > deregister_mtd_blktrans() > > > |_ mutex_lock(&mtd_table_mutex) > > > |_ tr->remove_dev() -> inftl_remove_dev() > > > |_ del_mtd_blktrans_dev() > > > |_ put_mtd_device() > > > |_ mutex_lock(&mtd_table_mutex) <--- AA deadlock > > > > What's more, this code in del_mtd_blktrans_dev() makes it obvious > that > > this hunk is wrong: > > > > int del_mtd_blktrans_dev(struct mtd_blktrans_dev *old) > > { > > unsigned long flags; > > > > if (mutex_trylock(&mtd_table_mutex)) { > > mutex_unlock(&mtd_table_mutex); > > BUG(); > > } > > ... > > > > So rather than a comment, the code is showing that it's a BUG() to > not > > be holding mtd_table_mutex already. > Hello, Thanks for your comments and for pointing this out. Definitely yes.. we shouldn't change del_mtd_blktrans_dev(). > As an alternative to your patch, how about the following? I think it's the right way to go now. Thanks! Giuseppe ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd: fix: avoid race condition when accessing mtd->usecount 2015-05-11 7:44 ` Cantavenera, Giuseppe (EXT-Other - DE/Ulm) @ 2015-05-11 22:25 ` Brian Norris 2015-05-12 6:38 ` Cantavenera, Giuseppe (EXT-Other - DE/Ulm) 0 siblings, 1 reply; 6+ messages in thread From: Brian Norris @ 2015-05-11 22:25 UTC (permalink / raw) To: Cantavenera, Giuseppe (EXT-Other - DE/Ulm) Cc: linux-mtd@lists.infradead.org, Restelli, Lorenzo (EXT-Other - DE/Ulm), dwmw2@infradead.org, stable@vger.kernel.org, Sverdlin, Alexander (Nokia - DE/Ulm), zhangxingcai, fengfuqiu@huawei.com, tanhaijun@huawei.com, linux-kernel@vger.kernel.org On Mon, May 11, 2015 at 07:44:26AM +0000, Cantavenera, Giuseppe (EXT-Other - DE/Ulm) wrote: > > -----Original Message----- > > From: ext Brian Norris [mailto:computersforpeace@gmail.com] > > Sent: Friday, May 08, 2015 2:27 AM > > To: Cantavenera, Giuseppe (EXT-Other - DE/Ulm) > > Cc: linux-mtd@lists.infradead.org; Restelli, Lorenzo (EXT-Other - > > DE/Ulm); dwmw2@infradead.org; stable@vger.kernel.org; Sverdlin, > > Alexander (Nokia - DE/Ulm); zhangxingcai; fengfuqiu@huawei.com; > > tanhaijun@huawei.com; linux-kernel@vger.kernel.org > > Subject: Re: [PATCH] mtd: fix: avoid race condition when accessing mtd- > > >usecount > > > > On Thu, May 07, 2015 at 05:17:45PM -0700, Brian Norris wrote: > > > On Thu, May 07, 2015 at 05:10:12PM -0700, Brian Norris wrote: > > > > On Tue, Apr 21, 2015 at 12:20:22PM +0200, Giuseppe Cantavenera > > wrote: > > > > > @@ -484,7 +486,7 @@ int del_mtd_blktrans_dev(struct > > mtd_blktrans_dev *old) > > > > > if (old->open) { > > > > > if (old->tr->release) > > > > > old->tr->release(old); > > > > > - __put_mtd_device(old->mtd); > > > > > + put_mtd_device(old->mtd); > > > > > > > > This looks wrong. See: > > > [...] > > > > deregister_mtd_blktrans() > > > > |_ mutex_lock(&mtd_table_mutex) > > > > |_ tr->remove_dev() -> inftl_remove_dev() > > > > |_ del_mtd_blktrans_dev() > > > > |_ put_mtd_device() > > > > |_ mutex_lock(&mtd_table_mutex) <--- AA deadlock > > > > > > What's more, this code in del_mtd_blktrans_dev() makes it obvious > > that > > > this hunk is wrong: > > > > > > int del_mtd_blktrans_dev(struct mtd_blktrans_dev *old) > > > { > > > unsigned long flags; > > > > > > if (mutex_trylock(&mtd_table_mutex)) { > > > mutex_unlock(&mtd_table_mutex); > > > BUG(); > > > } > > > ... > > > > > > So rather than a comment, the code is showing that it's a BUG() to > > not > > > be holding mtd_table_mutex already. > > > > Hello, > Thanks for your comments and for pointing this out. > Definitely yes.. we shouldn't change del_mtd_blktrans_dev(). > > > As an alternative to your patch, how about the following? > > I think it's the right way to go now. Can I get a 'Tested-by', or at least an 'Acked-by' for the patch? I tested it, but I don't think I can reproduce your original problem very easily. Brian ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] mtd: fix: avoid race condition when accessing mtd->usecount 2015-05-11 22:25 ` Brian Norris @ 2015-05-12 6:38 ` Cantavenera, Giuseppe (EXT-Other - DE/Ulm) 2015-05-12 18:04 ` Brian Norris 0 siblings, 1 reply; 6+ messages in thread From: Cantavenera, Giuseppe (EXT-Other - DE/Ulm) @ 2015-05-12 6:38 UTC (permalink / raw) To: ext Brian Norris Cc: linux-mtd@lists.infradead.org, Restelli, Lorenzo (EXT-Other - DE/Ulm), dwmw2@infradead.org, stable@vger.kernel.org, Sverdlin, Alexander (Nokia - DE/Ulm), zhangxingcai, fengfuqiu@huawei.com, tanhaijun@huawei.com, linux-kernel@vger.kernel.org > -----Original Message----- > From: ext Brian Norris [mailto:computersforpeace@gmail.com] > Sent: Tuesday, May 12, 2015 12:25 AM > To: Cantavenera, Giuseppe (EXT-Other - DE/Ulm) > Cc: linux-mtd@lists.infradead.org; Restelli, Lorenzo (EXT-Other - > DE/Ulm); dwmw2@infradead.org; stable@vger.kernel.org; Sverdlin, > Alexander (Nokia - DE/Ulm); zhangxingcai; fengfuqiu@huawei.com; > tanhaijun@huawei.com; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] mtd: fix: avoid race condition when accessing mtd- > >usecount > > On Mon, May 11, 2015 at 07:44:26AM +0000, Cantavenera, Giuseppe (EXT- > Other - DE/Ulm) wrote: > > > -----Original Message----- > > > From: ext Brian Norris [mailto:computersforpeace@gmail.com] > > > Sent: Friday, May 08, 2015 2:27 AM > > > To: Cantavenera, Giuseppe (EXT-Other - DE/Ulm) > > > Cc: linux-mtd@lists.infradead.org; Restelli, Lorenzo (EXT-Other - > > > DE/Ulm); dwmw2@infradead.org; stable@vger.kernel.org; Sverdlin, > > > Alexander (Nokia - DE/Ulm); zhangxingcai; fengfuqiu@huawei.com; > > > tanhaijun@huawei.com; linux-kernel@vger.kernel.org > > > Subject: Re: [PATCH] mtd: fix: avoid race condition when accessing > mtd- > > > >usecount > > > > > > On Thu, May 07, 2015 at 05:17:45PM -0700, Brian Norris wrote: > > > > On Thu, May 07, 2015 at 05:10:12PM -0700, Brian Norris wrote: > > > > > On Tue, Apr 21, 2015 at 12:20:22PM +0200, Giuseppe Cantavenera > > > wrote: > Can I get a 'Tested-by', or at least an 'Acked-by' for the patch? I > tested it, but I don't think I can reproduce your original problem very > easily. > > Brian Hello Brian, We were able to do some long runs last night and as expected this patch seems to address the issue pretty well. Thanks, Giuseppe Tested-by: Giuseppe Cantavenera <giuseppe.cantavenera.ext@nokia.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd: fix: avoid race condition when accessing mtd->usecount 2015-05-12 6:38 ` Cantavenera, Giuseppe (EXT-Other - DE/Ulm) @ 2015-05-12 18:04 ` Brian Norris 0 siblings, 0 replies; 6+ messages in thread From: Brian Norris @ 2015-05-12 18:04 UTC (permalink / raw) To: Cantavenera, Giuseppe (EXT-Other - DE/Ulm) Cc: linux-mtd@lists.infradead.org, Restelli, Lorenzo (EXT-Other - DE/Ulm), dwmw2@infradead.org, stable@vger.kernel.org, Sverdlin, Alexander (Nokia - DE/Ulm), zhangxingcai, fengfuqiu@huawei.com, tanhaijun@huawei.com, linux-kernel@vger.kernel.org On Tue, May 12, 2015 at 06:38:34AM +0000, Cantavenera, Giuseppe (EXT-Other - DE/Ulm) wrote: > > -----Original Message----- > > From: ext Brian Norris [mailto:computersforpeace@gmail.com] > > Sent: Tuesday, May 12, 2015 12:25 AM > > To: Cantavenera, Giuseppe (EXT-Other - DE/Ulm) > > Cc: linux-mtd@lists.infradead.org; Restelli, Lorenzo (EXT-Other - > > DE/Ulm); dwmw2@infradead.org; stable@vger.kernel.org; Sverdlin, > > Alexander (Nokia - DE/Ulm); zhangxingcai; fengfuqiu@huawei.com; > > tanhaijun@huawei.com; linux-kernel@vger.kernel.org > > Subject: Re: [PATCH] mtd: fix: avoid race condition when accessing mtd- > > >usecount > > > > On Mon, May 11, 2015 at 07:44:26AM +0000, Cantavenera, Giuseppe (EXT- > > Other - DE/Ulm) wrote: > > > > -----Original Message----- > > > > From: ext Brian Norris [mailto:computersforpeace@gmail.com] > > > > Sent: Friday, May 08, 2015 2:27 AM > > > > To: Cantavenera, Giuseppe (EXT-Other - DE/Ulm) > > > > Cc: linux-mtd@lists.infradead.org; Restelli, Lorenzo (EXT-Other - > > > > DE/Ulm); dwmw2@infradead.org; stable@vger.kernel.org; Sverdlin, > > > > Alexander (Nokia - DE/Ulm); zhangxingcai; fengfuqiu@huawei.com; > > > > tanhaijun@huawei.com; linux-kernel@vger.kernel.org > > > > Subject: Re: [PATCH] mtd: fix: avoid race condition when accessing > > mtd- > > > > >usecount > > > > > > > > On Thu, May 07, 2015 at 05:17:45PM -0700, Brian Norris wrote: > > > > > On Thu, May 07, 2015 at 05:10:12PM -0700, Brian Norris wrote: > > > > > > On Tue, Apr 21, 2015 at 12:20:22PM +0200, Giuseppe Cantavenera > > > > wrote: > > > Can I get a 'Tested-by', or at least an 'Acked-by' for the patch? I > > tested it, but I don't think I can reproduce your original problem very > > easily. > > > > Brian > > Hello Brian, > > We were able to do some long runs last night and as expected > this patch seems to address the issue pretty well. > > Thanks, > Giuseppe > > Tested-by: Giuseppe Cantavenera <giuseppe.cantavenera.ext@nokia.com> Great, thanks to all of you. I reworked the patch description and applied this to l2-mtd.git. Brian ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd: fix: avoid race condition when accessing mtd->usecount 2015-05-08 0:26 ` [PATCH] mtd: fix: avoid race condition when accessing mtd->usecount Brian Norris 2015-05-11 7:44 ` Cantavenera, Giuseppe (EXT-Other - DE/Ulm) @ 2015-05-12 16:37 ` Alexander Sverdlin 1 sibling, 0 replies; 6+ messages in thread From: Alexander Sverdlin @ 2015-05-12 16:37 UTC (permalink / raw) To: ext Brian Norris, Giuseppe Cantavenera Cc: linux-mtd, lorenzo.restelli.ext, dwmw2, stable, zhangxingcai, fengfuqiu, tanhaijun, linux-kernel On 08/05/15 02:26, ext Brian Norris wrote: > As an alternative to your patch, how about the following? > > BTW, this does still leave a usecount race in > drivers/mtd/maps/vmu-flash.c. But that driver should really be using > mtd->_get_device(), if it actually wants its own refcount. > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> Acked-by: Alexander Sverdlin <alexander.sverdlin@nokia.com> > --- > > diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c > index 2b0c52870999..df7c6c70757a 100644 > --- a/drivers/mtd/mtd_blkdevs.c > +++ b/drivers/mtd/mtd_blkdevs.c > @@ -197,6 +197,7 @@ static int blktrans_open(struct block_device *bdev, fmode_t mode) > return -ERESTARTSYS; /* FIXME: busy loop! -arnd*/ > > mutex_lock(&dev->lock); > + mutex_lock(&mtd_table_mutex); > > if (dev->open) > goto unlock; > @@ -220,6 +221,7 @@ static int blktrans_open(struct block_device *bdev, fmode_t mode) > > unlock: > dev->open++; > + mutex_unlock(&mtd_table_mutex); > mutex_unlock(&dev->lock); > blktrans_dev_put(dev); > return ret; > @@ -230,6 +232,7 @@ error_release: > error_put: > module_put(dev->tr->owner); > kref_put(&dev->ref, blktrans_dev_release); > + mutex_unlock(&mtd_table_mutex); > mutex_unlock(&dev->lock); > blktrans_dev_put(dev); > return ret; > @@ -243,6 +246,7 @@ static void blktrans_release(struct gendisk *disk, fmode_t mode) > return; > > mutex_lock(&dev->lock); > + mutex_lock(&mtd_table_mutex); > > if (--dev->open) > goto unlock; > @@ -256,6 +260,7 @@ static void blktrans_release(struct gendisk *disk, fmode_t mode) > __put_mtd_device(dev->mtd); > } > unlock: > + mutex_unlock(&mtd_table_mutex); > mutex_unlock(&dev->lock); > blktrans_dev_put(dev); > } ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-05-12 18:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1429611622-2652-1-git-send-email-giuseppe.cantavenera.ext@nokia.com>
[not found] ` <20150508001012.GV32500@ld-irv-0074>
[not found] ` <20150508001745.GW32500@ld-irv-0074>
2015-05-08 0:26 ` [PATCH] mtd: fix: avoid race condition when accessing mtd->usecount Brian Norris
2015-05-11 7:44 ` Cantavenera, Giuseppe (EXT-Other - DE/Ulm)
2015-05-11 22:25 ` Brian Norris
2015-05-12 6:38 ` Cantavenera, Giuseppe (EXT-Other - DE/Ulm)
2015-05-12 18:04 ` Brian Norris
2015-05-12 16:37 ` Alexander Sverdlin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox