[PATCH] deadlock when swapping to FAT

[PATCH] deadlock when swapping to FAT

[Mikulas Patocka]

Hi

swapon will deadlock when an attempt to swap to a file on FAT filesystem 
is made. swapon holds i_mutex and FAT bmap is attempting to take it again.

This bug was introduced somewhere between 2.6.27 and 2.6.28.

No other filesystem is taking i_mutex in bmap, so I removed it as well.

Note that there are many other cases where bmap can race with truncate in 
almost all the filesystems. They don't impose an immediate threat because 
bmap can be only called by root. These problems should be solved in a 
generic way, not in individual filesystems.

Mikulas

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com
Cc: <stable@kernel.org>

---
 fs/fat/inode.c |    2 --
 1 file changed, 2 deletions(-)

Index: linux-2.6.29-rc7-devel/fs/fat/inode.c
===================================================================
--- linux-2.6.29-rc7-devel.orig/fs/fat/inode.c	2009-03-10 21:17:22.000000000 +0100
+++ linux-2.6.29-rc7-devel/fs/fat/inode.c	2009-03-11 22:52:38.000000000 +0100
@@ -202,9 +202,7 @@ static sector_t _fat_bmap(struct address
 	sector_t blocknr;
 
 	/* fat_get_cluster() assumes the requested blocknr isn't truncated. */
-	mutex_lock(&mapping->host->i_mutex);
 	blocknr = generic_block_bmap(mapping, block, fat_get_block);
-	mutex_unlock(&mapping->host->i_mutex);
 
 	return blocknr;
 }

Re: [PATCH] deadlock when swapping to FAT

[OGAWA Hirofumi]

Mikulas Patocka <mpatocka@redhat.com> writes:

> swapon will deadlock when an attempt to swap to a file on FAT filesystem 
> is made. swapon holds i_mutex and FAT bmap is attempting to take it again.
>
> This bug was introduced somewhere between 2.6.27 and 2.6.28.
>
> No other filesystem is taking i_mutex in bmap, so I removed it as well.
>
> Note that there are many other cases where bmap can race with truncate in 
> almost all the filesystems. They don't impose an immediate threat because 
> bmap can be only called by root. These problems should be solved in a 
> generic way, not in individual filesystems.

Thanks. This was fixed recently with a bit different way.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] deadlock when swapping to FAT

[Mikulas Patocka]

On Fri, 13 Mar 2009, OGAWA Hirofumi wrote:

> Mikulas Patocka <mpatocka@redhat.com> writes:
> 
> > swapon will deadlock when an attempt to swap to a file on FAT filesystem 
> > is made. swapon holds i_mutex and FAT bmap is attempting to take it again.
> >
> > This bug was introduced somewhere between 2.6.27 and 2.6.28.
> >
> > No other filesystem is taking i_mutex in bmap, so I removed it as well.
> >
> > Note that there are many other cases where bmap can race with truncate in 
> > almost all the filesystems. They don't impose an immediate threat because 
> > bmap can be only called by root. These problems should be solved in a 
> > generic way, not in individual filesystems.
> 
> Thanks. This was fixed recently with a bit different way.
> -- 
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Note that the same race condition is happening in all the other 
filesystems. Maybe move that i_alloc_sem up to ->bmap method caller?

Mikulas

Re: [PATCH] deadlock when swapping to FAT

[OGAWA Hirofumi]

Mikulas Patocka <mpatocka@redhat.com> writes:

> Note that the same race condition is happening in all the other 
> filesystems. Maybe move that i_alloc_sem up to ->bmap method caller?

It can be. However, I guess locking strategy would be per
filesystems. Because the fs may be using i_alloc_sem in get_block
already.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] deadlock when swapping to FAT

[Mikulas Patocka]

On Sun, 15 Mar 2009, OGAWA Hirofumi wrote:

> Mikulas Patocka <mpatocka@redhat.com> writes:
> 
> > Note that the same race condition is happening in all the other 
> > filesystems. Maybe move that i_alloc_sem up to ->bmap method caller?
> 
> It can be. However, I guess locking strategy would be per
> filesystems. Because the fs may be using i_alloc_sem in get_block
> already.

Which ones take it in get_block? I grepped for i_alloc_sem and don't see 
them. Besides, it is mostly taken only for read and recursive taking of 
read-lock for read is allowed. It is taken for writes only in truncate.

Mikulas

> Thanks.
> -- 
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> 

Re: [PATCH] deadlock when swapping to FAT

[OGAWA Hirofumi]

Mikulas Patocka <mpatocka@redhat.com> writes:

> On Sun, 15 Mar 2009, OGAWA Hirofumi wrote:
>
>> Mikulas Patocka <mpatocka@redhat.com> writes:
>> 
>> > Note that the same race condition is happening in all the other 
>> > filesystems. Maybe move that i_alloc_sem up to ->bmap method caller?
>> 
>> It can be. However, I guess locking strategy would be per
>> filesystems. Because the fs may be using i_alloc_sem in get_block
>> already.
>
> Which ones take it in get_block? I grepped for i_alloc_sem and don't see 
> them. Besides, it is mostly taken only for read and recursive taking of 
> read-lock for read is allowed. It is taken for writes only in truncate.

I don't know which fs take it, and whether i_alloc_sem is enough for
which fs. It was just guess. And important one is locking strategy of
that would be per filesystems. E.g. it seems XFS is taking own lock.

Well, personally, I don't have objection to add i_alloc_sem, however I'm
not sure, what does i_alloc_sem guarantee for other fs.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] deadlock when swapping to FAT

[Mikulas Patocka]

On Wed, 18 Mar 2009, OGAWA Hirofumi wrote:

> Mikulas Patocka <mpatocka@redhat.com> writes:
> 
> > On Sun, 15 Mar 2009, OGAWA Hirofumi wrote:
> >
> >> Mikulas Patocka <mpatocka@redhat.com> writes:
> >> 
> >> > Note that the same race condition is happening in all the other 
> >> > filesystems. Maybe move that i_alloc_sem up to ->bmap method caller?
> >> 
> >> It can be. However, I guess locking strategy would be per
> >> filesystems. Because the fs may be using i_alloc_sem in get_block
> >> already.
> >
> > Which ones take it in get_block? I grepped for i_alloc_sem and don't see 
> > them. Besides, it is mostly taken only for read and recursive taking of 
> > read-lock for read is allowed. It is taken for writes only in truncate.
> 
> I don't know which fs take it, and whether i_alloc_sem is enough for
> which fs. It was just guess. And important one is locking strategy of
> that would be per filesystems. E.g. it seems XFS is taking own lock.
> 
> Well, personally, I don't have objection to add i_alloc_sem, however I'm
> not sure, what does i_alloc_sem guarantee for other fs.

It should prevent truncation under bmap. It is used by direct-io code to 
protect the file from being truncated while there's direct-io being 
processed on it.

But some filesystems do their own direct-io locking (for example XFS). So 
I think it would be best to place the lock to generic_block_bmap, so that 
filesystem that doesn't want the lock can easily avoid it.

You can submit this patch after 2.6.29 is released.

Mikulas

> -- 
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>


FAT filesystem used down_read(&mapping->host->i_alloc_sem) to prevent 
a race between bmap and truncate. However, such race is present in all the
other filesystems --- it is generally assumed that blocks queried with
get_block won't disappear while get_block is in progress.

The race can be only triggered by root, non-privileged users can't use
bmap, so it is not a security issue (unless there is some program run
by root that bmaps users' files).

This patch fixes the race in a generic way, in all the filesystems. If some
filesystem employs its own locking and doesn't want to take i_alloc_sem
(I don't know about any, where taking i_alloc_sem could be problem),
let it use its own function and not generic_block_bmap.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 fs/buffer.c    |    8 ++++++++
 fs/fat/inode.c |    2 --
 2 files changed, 8 insertions(+), 2 deletions(-)

Index: linux-2.6.29-rc8-devel/fs/buffer.c
===================================================================
--- linux-2.6.29-rc8-devel.orig/fs/buffer.c	2009-03-19 15:57:03.000000000 +0100
+++ linux-2.6.29-rc8-devel/fs/buffer.c	2009-03-19 15:58:00.000000000 +0100
@@ -2964,7 +2964,15 @@ sector_t generic_block_bmap(struct addre
 	tmp.b_state = 0;
 	tmp.b_blocknr = 0;
 	tmp.b_size = 1 << inode->i_blkbits;
+
+	/*
+	 * Protect the inode from being truncated while get_block is
+	 * in progress.
+	 */
+	down_read(&mapping->host->i_alloc_sem);
 	get_block(inode, block, &tmp, 0);
+	up_read(&mapping->host->i_alloc_sem);
+
 	return tmp.b_blocknr;
 }
 
Index: linux-2.6.29-rc8-devel/fs/fat/inode.c
===================================================================
--- linux-2.6.29-rc8-devel.orig/fs/fat/inode.c	2009-03-19 15:56:50.000000000 +0100
+++ linux-2.6.29-rc8-devel/fs/fat/inode.c	2009-03-19 15:56:58.000000000 +0100
@@ -202,9 +202,7 @@ static sector_t _fat_bmap(struct address
 	sector_t blocknr;
 
 	/* fat_get_cluster() assumes the requested blocknr isn't truncated. */
-	down_read(&mapping->host->i_alloc_sem);
 	blocknr = generic_block_bmap(mapping, block, fat_get_block);
-	up_read(&mapping->host->i_alloc_sem);
 
 	return blocknr;
 }

Re: [PATCH] deadlock when swapping to FAT

[OGAWA Hirofumi]

Mikulas Patocka <mpatocka@redhat.com> writes:

> It should prevent truncation under bmap. It is used by direct-io code to 
> protect the file from being truncated while there's direct-io being 
> processed on it.

I know, FAT is ok, of course, and the simple fs would also be ok.
However, it's not enough for all filesystems, in theory.

> But some filesystems do their own direct-io locking (for example XFS). So 
> I think it would be best to place the lock to generic_block_bmap, so that 
> filesystem that doesn't want the lock can easily avoid it.
>
> You can submit this patch after 2.6.29 is released.

I don't have objection to it. Please submit it yourself.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>