Re: [PATCH] mtd: fix: avoid race condition when accessing mtd->usecount

Re: [PATCH] mtd: fix: avoid race condition when accessing mtd->usecount

[Brian Norris]

On Tue, Apr 21, 2015 at 12:20:22PM +0200, Giuseppe Cantavenera wrote:
> On A MIPS 32-cores machine a BUG_ON was triggered because some acesses to
> mtd->usecount were done without taking mtd_table_mutex.
> kernel: Call Trace:
> kernel: [<ffffffff80401818>] __put_mtd_device+0x20/0x50
> kernel: [<ffffffff804086f4>] blktrans_release+0x8c/0xd8
> kernel: [<ffffffff802577e0>] __blkdev_put+0x1a8/0x200
> kernel: [<ffffffff802579a4>] blkdev_close+0x1c/0x30
> kernel: [<ffffffff8022006c>] __fput+0xac/0x250
> kernel: [<ffffffff80171208>] task_work_run+0xd8/0x120
> kernel: [<ffffffff8012c23c>] work_notifysig+0x10/0x18
> kernel:
> kernel:
>         Code: 2442ffff  ac8202d8  000217fe <00020336> dc820128  10400003
>                00000000  0040f809  00000000
> kernel: ---[ end trace 080fbb4579b47a73 ]---
> 
> Fixed by taking the mutex in blktrans_open and by using the protected
> version of put_mtd_device in blktrans_release and del_mtd_blktrans_dev

FYI, I think there's a similar report here:

http://patchwork.ozlabs.org/patch/415125/

But the patch had other flaws. I think you're more along the right track
here.

> Cc: stable@vger.kernel.org
> Cc: alexander.sverdlin@nokia.com
> Signed-off-by: Giuseppe Cantavenera <giuseppe.cantavenera.ext@nokia.com>
> Acked-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> ---
>  drivers/mtd/mtd_blkdevs.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
> index 2b0c5287..3a9f138 100644
> --- a/drivers/mtd/mtd_blkdevs.c
> +++ b/drivers/mtd/mtd_blkdevs.c
> @@ -213,7 +213,9 @@ static int blktrans_open(struct block_device *bdev, fmode_t mode)
>  			goto error_put;
>  	}
>  
> +	mutex_lock(&mtd_table_mutex);
>  	ret = __get_mtd_device(dev->mtd);
> +	mutex_unlock(&mtd_table_mutex);

I think this is a start, but it's not enough. See below [*].

>  	if (ret)
>  		goto error_release;
>  	dev->file_mode = mode;
> @@ -253,7 +255,7 @@ static void blktrans_release(struct gendisk *disk, fmode_t mode)
>  	if (dev->mtd) {
>  		if (dev->tr->release)
>  			dev->tr->release(dev);
> -		__put_mtd_device(dev->mtd);
> +		put_mtd_device(dev->mtd);

Same, see [*].

>  	}
>  unlock:
>  	mutex_unlock(&dev->lock);
> @@ -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:

int deregister_mtd_blktrans(struct mtd_blktrans_ops *tr)                                       
{                                                                                              
        struct mtd_blktrans_dev *dev, *next;                                                   
                                                                                               
        mutex_lock(&mtd_table_mutex);                                                          
                                                                                               
        /* Remove it from the list of active majors */                                         
        list_del(&tr->list);                                                                   
                                                                                               
        list_for_each_entry_safe(dev, next, &tr->devs, list)                                   
                tr->remove_dev(dev);                                                           
                                                                                               
        unregister_blkdev(tr->major, tr->name);                                                
        mutex_unlock(&mtd_table_mutex);                                                        
                                                                                               
        BUG_ON(!list_empty(&tr->devs));                                                        
        return 0;                       
}

Now, many mtd_blkdev implementations call del_mtd_blktrans_dev() in
their tr->remove_dev() functions (e.g., see inftl_remove_dev()). This
means you will have:

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

>  	}
>  
>  	old->mtd = NULL;

Note that I haven't actually tested this, so you may have introduced
other bad locking issues too. I'd expect a little more rigor out of a
patch marked for -stable, especially.

To that end, it looks like maybe we could use some better comments to
describe the expected locking. Patches are welcome, if you learn some
things in trying to verify your patch!

Speaking of that...

[*] I poked through the locking a bit more, and I see that such a
comment exists in mtd/blktrans.h:

struct mtd_blktrans_ops {
...
        /* Called with mtd_table_mutex held; no race with add/remove */
        int (*open)(struct mtd_blktrans_dev *dev);
        void (*release)(struct mtd_blktrans_dev *dev);
...
};

However, that is currently *not* the case. See blktrans_open() and
blktrans_release(), which you are modifying. They are calling tr->open()
and tr->release() *without* holding mtd_table_mutex. So the correct fix
might be to hold the table mutex for those entire functions.

Brian

Re: [PATCH] mtd: fix: avoid race condition when accessing mtd->usecount

[Brian Norris]

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.

Brian

Re: [PATCH] mtd: fix: avoid race condition when accessing mtd->usecount

[Brian Norris]

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);
 }

RE: [PATCH] mtd: fix: avoid race condition when accessing mtd->usecount

[Cantavenera, Giuseppe (EXT-Other - DE/Ulm)]

> -----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

Re: [PATCH] mtd: fix: avoid race condition when accessing mtd->usecount

[Brian Norris]

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

RE: [PATCH] mtd: fix: avoid race condition when accessing mtd->usecount

[Cantavenera, Giuseppe (EXT-Other - DE/Ulm)]

> -----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>

Re: [PATCH] mtd: fix: avoid race condition when accessing mtd->usecount

[Alexander Sverdlin]

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);
>  }

Re: [PATCH] mtd: fix: avoid race condition when accessing mtd->usecount

[Brian Norris]

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