* [PATCH] Fix a race condition between ->i_mapping and iput()
@ 2006-03-09 16:23 OGAWA Hirofumi
2006-03-10 4:27 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: OGAWA Hirofumi @ 2006-03-09 16:23 UTC (permalink / raw)
To: linux-kernel
Hi,
This race became a cause of oops, and can reproduce by the following.
while true; do
dd if=/dev/zero of=/dev/.static/dev/hdg1 bs=512 count=1000 & sync
done
This race condition was between __sync_single_inode() and iput().
cpu0 (fs's inode) cpu1 (bdev's inode)
------------------------------------------------------------------------
close("/dev/hda2")
[...]
__sync_single_inode()
/* copy the bdev's ->i_mapping */
mapping = inode->i_mapping;
generic_forget_inode()
bdev_clear_inode()
/* restre the fs's ->i_mapping */
inode->i_mapping = &inode->i_data;
/* bdev's inode was freed */
destroy_inode(inode);
if (wait) {
/* dereference a freed bdev's mapping->host */
filemap_fdatawait(mapping); /* Oops */
Since __sync_signle_inode() is only taking a ref-count of fs's inode,
the another process can be close() and freeing the bdev's inode while
writing fs's inode. So, __sync_signle_inode() accesses the freed
->i_mapping, oops.
This patch takes ref-count of bdev's inode for fs's inode before
setting a ->i_mapping, and the clear_inode() of fs's inode does iput().
So, if fs's inode is still living, bdev's inode shouldn't be freed.
This lifetime rule may be a poor, but very simple.
Umm... should we use an another rule to free it more early?
(e.g. if bdev's inode become I_FREEING, it should call bd_forget()
before releasing the inode_lock. And some place should call
igrab(->i_mapping->host->i_count) and iput())
What do you think, comment?
Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---
fs/block_dev.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff -puN fs/block_dev.c~i_mapping-race-fix-2 fs/block_dev.c
--- linux-2.6/fs/block_dev.c~i_mapping-race-fix-2 2006-03-09 03:08:54.000000000 +0900
+++ linux-2.6-hirofumi/fs/block_dev.c 2006-03-09 03:08:54.000000000 +0900
@@ -441,13 +441,22 @@ static struct block_device *bd_acquire(s
spin_unlock(&bdev_lock);
bdev = bdget(inode->i_rdev);
if (bdev) {
+ struct block_device *old = NULL;
+
+ BUG_ON(inode->i_sb == blockdev_superblock);
spin_lock(&bdev_lock);
- if (inode->i_bdev)
+ if (inode->i_bdev) {
+ old = inode->i_bdev;
__bd_forget(inode);
+ }
+ atomic_inc(&bdev->bd_inode->i_count);
inode->i_bdev = bdev;
inode->i_mapping = bdev->bd_inode->i_mapping;
list_add(&inode->i_devices, &bdev->bd_inodes);
spin_unlock(&bdev_lock);
+
+ if (old)
+ iput(old->bd_inode);
}
return bdev;
}
@@ -456,10 +465,18 @@ static struct block_device *bd_acquire(s
void bd_forget(struct inode *inode)
{
+ struct block_device *old = NULL;
+
spin_lock(&bdev_lock);
- if (inode->i_bdev)
+ if (inode->i_bdev) {
+ if (inode->i_sb != blockdev_superblock)
+ old = inode->i_bdev;
__bd_forget(inode);
+ }
spin_unlock(&bdev_lock);
+
+ if (old)
+ iput(old->bd_inode);
}
int bd_claim(struct block_device *bdev, void *holder)
_
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Fix a race condition between ->i_mapping and iput()
2006-03-09 16:23 [PATCH] Fix a race condition between ->i_mapping and iput() OGAWA Hirofumi
@ 2006-03-10 4:27 ` Andrew Morton
2006-03-10 15:30 ` OGAWA Hirofumi
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2006-03-10 4:27 UTC (permalink / raw)
To: OGAWA Hirofumi; +Cc: linux-kernel
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> wrote:
>
> Hi,
>
> This race became a cause of oops, and can reproduce by the following.
>
> while true; do
> dd if=/dev/zero of=/dev/.static/dev/hdg1 bs=512 count=1000 & sync
> done
>
>
> This race condition was between __sync_single_inode() and iput().
>
> cpu0 (fs's inode) cpu1 (bdev's inode)
> ------------------------------------------------------------------------
> close("/dev/hda2")
> [...]
> __sync_single_inode()
> /* copy the bdev's ->i_mapping */
> mapping = inode->i_mapping;
>
> generic_forget_inode()
> bdev_clear_inode()
> /* restre the fs's ->i_mapping */
> inode->i_mapping = &inode->i_data;
> /* bdev's inode was freed */
> destroy_inode(inode);
>
> if (wait) {
> /* dereference a freed bdev's mapping->host */
> filemap_fdatawait(mapping); /* Oops */
>
> Since __sync_signle_inode() is only taking a ref-count of fs's inode,
> the another process can be close() and freeing the bdev's inode while
> writing fs's inode. So, __sync_signle_inode() accesses the freed
> ->i_mapping, oops.
>
> This patch takes ref-count of bdev's inode for fs's inode before
> setting a ->i_mapping, and the clear_inode() of fs's inode does iput().
> So, if fs's inode is still living, bdev's inode shouldn't be freed.
>
> This lifetime rule may be a poor, but very simple.
>
> Umm... should we use an another rule to free it more early?
> (e.g. if bdev's inode become I_FREEING, it should call bd_forget()
> before releasing the inode_lock. And some place should call
> igrab(->i_mapping->host->i_count) and iput())
>
>
> What do you think, comment?
Maybe. This code seems relatively straightforward though.
It would be preferable to have a couple of comments in there explaining
what the new refcounting is there for.
>
>...
>
> void bd_forget(struct inode *inode)
> {
> + struct block_device *old = NULL;
> +
> spin_lock(&bdev_lock);
> - if (inode->i_bdev)
> + if (inode->i_bdev) {
> + if (inode->i_sb != blockdev_superblock)
> + old = inode->i_bdev;
> __bd_forget(inode);
> + }
> spin_unlock(&bdev_lock);
> +
> + if (old)
> + iput(old->bd_inode);
> }
We're missing an atomic_inc(i_count) here?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Fix a race condition between ->i_mapping and iput()
2006-03-10 4:27 ` Andrew Morton
@ 2006-03-10 15:30 ` OGAWA Hirofumi
0 siblings, 0 replies; 6+ messages in thread
From: OGAWA Hirofumi @ 2006-03-10 15:30 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel
Andrew Morton <akpm@osdl.org> writes:
>> void bd_forget(struct inode *inode)
>> {
>> + struct block_device *old = NULL;
>> +
>> spin_lock(&bdev_lock);
>> - if (inode->i_bdev)
>> + if (inode->i_bdev) {
>> + if (inode->i_sb != blockdev_superblock)
>> + old = inode->i_bdev;
>> __bd_forget(inode);
>> + }
>> spin_unlock(&bdev_lock);
>> +
>> + if (old)
>> + iput(old->bd_inode);
>> }
>
> We're missing an atomic_inc(i_count) here?
I think we don't need an atomic_inc(i_count) here. The inode of
argument has already I_FREEING, so we just call iput(bdev's inode).
But, sorry. My patch seems broken.
With that simple rule, the code became more simple. And comment was
added.
I'll test a following patch today.
--
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
diff -puN fs/block_dev.c~i_mapping-race-fix-2 fs/block_dev.c
--- linux-2.6/fs/block_dev.c~i_mapping-race-fix-2 2006-03-10 22:54:09.000000000 +0900
+++ linux-2.6-hirofumi/fs/block_dev.c 2006-03-10 23:05:03.000000000 +0900
@@ -432,21 +432,32 @@ EXPORT_SYMBOL(bdput);
static struct block_device *bd_acquire(struct inode *inode)
{
struct block_device *bdev;
+
+ BUG_ON(inode->i_sb == blockdev_superblock);
spin_lock(&bdev_lock);
bdev = inode->i_bdev;
- if (bdev && igrab(bdev->bd_inode)) {
+ if (bdev) {
+ atomic_inc(&bdev->bd_inode->i_count);
spin_unlock(&bdev_lock);
return bdev;
}
spin_unlock(&bdev_lock);
+
bdev = bdget(inode->i_rdev);
if (bdev) {
spin_lock(&bdev_lock);
- if (inode->i_bdev)
- __bd_forget(inode);
- inode->i_bdev = bdev;
- inode->i_mapping = bdev->bd_inode->i_mapping;
- list_add(&inode->i_devices, &bdev->bd_inodes);
+ if (!inode->i_bdev) {
+ /*
+ * We take an additional bd_inode->i_count for inode,
+ * and it's released in clear_inode() of inode.
+ * So, we can access it via ->i_mapping always
+ * without igrab().
+ */
+ atomic_inc(&bdev->bd_inode->i_count);
+ inode->i_bdev = bdev;
+ inode->i_mapping = bdev->bd_inode->i_mapping;
+ list_add(&inode->i_devices, &bdev->bd_inodes);
+ }
spin_unlock(&bdev_lock);
}
return bdev;
@@ -456,10 +467,18 @@ static struct block_device *bd_acquire(s
void bd_forget(struct inode *inode)
{
+ struct block_device *old = NULL;
+
spin_lock(&bdev_lock);
- if (inode->i_bdev)
+ if (inode->i_bdev) {
+ if (inode->i_sb != blockdev_superblock)
+ old = inode->i_bdev;
__bd_forget(inode);
+ }
spin_unlock(&bdev_lock);
+
+ if (old)
+ iput(old->bd_inode);
}
int bd_claim(struct block_device *bdev, void *holder)
_
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] Fix a race condition between ->i_mapping and iput()
@ 2006-03-11 16:00 OGAWA Hirofumi
2006-03-16 22:49 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: OGAWA Hirofumi @ 2006-03-11 16:00 UTC (permalink / raw)
To: linux-kernel; +Cc: Andrew Morton
This race became a cause of oops, and can reproduce by the following.
while true; do
dd if=/dev/zero of=/dev/.static/dev/hdg1 bs=512 count=1000 & sync
done
This race condition was between __sync_single_inode() and iput().
cpu0 (fs's inode) cpu1 (bdev's inode)
------------------------------------------------------------------------
close("/dev/hda2")
[...]
__sync_single_inode()
/* copy the bdev's ->i_mapping */
mapping = inode->i_mapping;
generic_forget_inode()
bdev_clear_inode()
/* restre the fs's ->i_mapping */
inode->i_mapping = &inode->i_data;
/* bdev's inode was freed */
destroy_inode(inode);
if (wait) {
/* dereference a freed bdev's mapping->host */
filemap_fdatawait(mapping); /* Oops */
Since __sync_signle_inode() is only taking a ref-count of fs's inode,
the another process can be close() and freeing the bdev's inode while
writing fs's inode. So, __sync_signle_inode() accesses the freed
->i_mapping, oops.
This patch takes ref-count of bdev's inode for fs's inode before
setting a ->i_mapping, and the clear_inode() of fs's inode does iput().
So, if fs's inode is still living, bdev's inode shouldn't be freed.
This lifetime rule may be a poor, but very simple.
Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---
fs/block_dev.c | 32 +++++++++++++++++++++++++-------
1 file changed, 25 insertions(+), 7 deletions(-)
diff -puN fs/block_dev.c~i_mapping-race-fix-2 fs/block_dev.c
--- linux-2.6/fs/block_dev.c~i_mapping-race-fix-2 2006-03-11 16:19:07.000000000 +0900
+++ linux-2.6-hirofumi/fs/block_dev.c 2006-03-11 17:52:11.000000000 +0900
@@ -418,21 +418,31 @@ EXPORT_SYMBOL(bdput);
static struct block_device *bd_acquire(struct inode *inode)
{
struct block_device *bdev;
+
spin_lock(&bdev_lock);
bdev = inode->i_bdev;
- if (bdev && igrab(bdev->bd_inode)) {
+ if (bdev) {
+ atomic_inc(&bdev->bd_inode->i_count);
spin_unlock(&bdev_lock);
return bdev;
}
spin_unlock(&bdev_lock);
+
bdev = bdget(inode->i_rdev);
if (bdev) {
spin_lock(&bdev_lock);
- if (inode->i_bdev)
- __bd_forget(inode);
- inode->i_bdev = bdev;
- inode->i_mapping = bdev->bd_inode->i_mapping;
- list_add(&inode->i_devices, &bdev->bd_inodes);
+ if (!inode->i_bdev) {
+ /*
+ * We take an additional bd_inode->i_count for inode,
+ * and it's released in clear_inode() of inode.
+ * So, we can access it via ->i_mapping always
+ * without igrab().
+ */
+ atomic_inc(&bdev->bd_inode->i_count);
+ inode->i_bdev = bdev;
+ inode->i_mapping = bdev->bd_inode->i_mapping;
+ list_add(&inode->i_devices, &bdev->bd_inodes);
+ }
spin_unlock(&bdev_lock);
}
return bdev;
@@ -442,10 +452,18 @@ static struct block_device *bd_acquire(s
void bd_forget(struct inode *inode)
{
+ struct block_device *bdev = NULL;
+
spin_lock(&bdev_lock);
- if (inode->i_bdev)
+ if (inode->i_bdev) {
+ if (inode->i_sb != blockdev_superblock)
+ bdev = inode->i_bdev;
__bd_forget(inode);
+ }
spin_unlock(&bdev_lock);
+
+ if (bdev)
+ iput(bdev->bd_inode);
}
int bd_claim(struct block_device *bdev, void *holder)
_
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Fix a race condition between ->i_mapping and iput()
2006-03-11 16:00 OGAWA Hirofumi
@ 2006-03-16 22:49 ` Andrew Morton
2006-03-17 14:13 ` OGAWA Hirofumi
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2006-03-16 22:49 UTC (permalink / raw)
To: OGAWA Hirofumi; +Cc: linux-kernel
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> wrote:
>
> This race became a cause of oops, and can reproduce by the following.
>
> while true; do
> dd if=/dev/zero of=/dev/.static/dev/hdg1 bs=512 count=1000 & sync
> done
>
>
> This race condition was between __sync_single_inode() and iput().
>
> cpu0 (fs's inode) cpu1 (bdev's inode)
> ------------------------------------------------------------------------
> close("/dev/hda2")
> [...]
> __sync_single_inode()
> /* copy the bdev's ->i_mapping */
> mapping = inode->i_mapping;
>
> generic_forget_inode()
> bdev_clear_inode()
> /* restre the fs's ->i_mapping */
> inode->i_mapping = &inode->i_data;
> /* bdev's inode was freed */
> destroy_inode(inode);
>
> if (wait) {
> /* dereference a freed bdev's mapping->host */
> filemap_fdatawait(mapping); /* Oops */
>
> Since __sync_signle_inode() is only taking a ref-count of fs's inode,
> the another process can be close() and freeing the bdev's inode while
> writing fs's inode. So, __sync_signle_inode() accesses the freed
> ->i_mapping, oops.
>
> This patch takes ref-count of bdev's inode for fs's inode before
> setting a ->i_mapping, and the clear_inode() of fs's inode does iput().
> So, if fs's inode is still living, bdev's inode shouldn't be freed.
>
OK, so to rephrase:
Whenever /dev/sda's inode->i_mapping points at a kernel-internal blockdev
inode's i_data, we will hold a ref on that blockdev inode, to pin its
i_data. That sounds sane.
>
> diff -puN fs/block_dev.c~i_mapping-race-fix-2 fs/block_dev.c
> --- linux-2.6/fs/block_dev.c~i_mapping-race-fix-2 2006-03-11 16:19:07.000000000 +0900
> +++ linux-2.6-hirofumi/fs/block_dev.c 2006-03-11 17:52:11.000000000 +0900
> @@ -418,21 +418,31 @@ EXPORT_SYMBOL(bdput);
> static struct block_device *bd_acquire(struct inode *inode)
> {
> struct block_device *bdev;
> +
> spin_lock(&bdev_lock);
> bdev = inode->i_bdev;
> - if (bdev && igrab(bdev->bd_inode)) {
> + if (bdev) {
> + atomic_inc(&bdev->bd_inode->i_count);
No longer checking (I_FREEING|I_WILL_FREE). Why was this change included?
> spin_unlock(&bdev_lock);
> return bdev;
> }
> spin_unlock(&bdev_lock);
> +
> bdev = bdget(inode->i_rdev);
> if (bdev) {
> spin_lock(&bdev_lock);
> - if (inode->i_bdev)
> - __bd_forget(inode);
> - inode->i_bdev = bdev;
> - inode->i_mapping = bdev->bd_inode->i_mapping;
> - list_add(&inode->i_devices, &bdev->bd_inodes);
> + if (!inode->i_bdev) {
> + /*
> + * We take an additional bd_inode->i_count for inode,
> + * and it's released in clear_inode() of inode.
> + * So, we can access it via ->i_mapping always
> + * without igrab().
> + */
> + atomic_inc(&bdev->bd_inode->i_count);
> + inode->i_bdev = bdev;
> + inode->i_mapping = bdev->bd_inode->i_mapping;
> + list_add(&inode->i_devices, &bdev->bd_inodes);
> + }
And why this change? The removal of the __bd_forget() and the changed
behaviour if (inode->i_bdev != NULL)?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Fix a race condition between ->i_mapping and iput()
2006-03-16 22:49 ` Andrew Morton
@ 2006-03-17 14:13 ` OGAWA Hirofumi
0 siblings, 0 replies; 6+ messages in thread
From: OGAWA Hirofumi @ 2006-03-17 14:13 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel
Andrew Morton <akpm@osdl.org> writes:
>> This patch takes ref-count of bdev's inode for fs's inode before
>> setting a ->i_mapping, and the clear_inode() of fs's inode does iput().
>> So, if fs's inode is still living, bdev's inode shouldn't be freed.
>>
>
> OK, so to rephrase:
>
> Whenever /dev/sda's inode->i_mapping points at a kernel-internal blockdev
> inode's i_data, we will hold a ref on that blockdev inode, to pin its
> i_data. That sounds sane.
Yes, thanks.
This approach may have one issue. If user is using a on-memory fs as
/dev (i.e. udev), the inode is never freed, so blockdev's inode is
also never freed.
And if user is creating the many block special file, blockdev's inode
is also pinned additionally.
Probably we can call a igrab() for ->i_mapping instead of this approach,
however it's "run-time overhead vs memory space" trade-off.
>> diff -puN fs/block_dev.c~i_mapping-race-fix-2 fs/block_dev.c
>> --- linux-2.6/fs/block_dev.c~i_mapping-race-fix-2 2006-03-11 16:19:07.000000000 +0900
>> +++ linux-2.6-hirofumi/fs/block_dev.c 2006-03-11 17:52:11.000000000 +0900
>> @@ -418,21 +418,31 @@ EXPORT_SYMBOL(bdput);
>> static struct block_device *bd_acquire(struct inode *inode)
>> {
>> struct block_device *bdev;
>> +
>> spin_lock(&bdev_lock);
>> bdev = inode->i_bdev;
>> - if (bdev && igrab(bdev->bd_inode)) {
>> + if (bdev) {
>> + atomic_inc(&bdev->bd_inode->i_count);
>
> No longer checking (I_FREEING|I_WILL_FREE). Why was this change included?
With this patch, inode->i_bdev is freed in clear_inode() after the
inode became the I_FREEING.
The caller is holding a inode's ref, and if the inode->i_bdev != NULL,
->i_bdev->bd_inode never have the I_FREEING/I_WILL_FREE. So we don't
need to check I_FREEING/I_WILL_FREE anymore.
>> - if (inode->i_bdev)
>> - __bd_forget(inode);
>> - inode->i_bdev = bdev;
>> - inode->i_mapping = bdev->bd_inode->i_mapping;
>> - list_add(&inode->i_devices, &bdev->bd_inodes);
>> + if (!inode->i_bdev) {
>> + /*
>> + * We take an additional bd_inode->i_count for inode,
>> + * and it's released in clear_inode() of inode.
>> + * So, we can access it via ->i_mapping always
>> + * without igrab().
>> + */
>> + atomic_inc(&bdev->bd_inode->i_count);
>> + inode->i_bdev = bdev;
>> + inode->i_mapping = bdev->bd_inode->i_mapping;
>> + list_add(&inode->i_devices, &bdev->bd_inodes);
>> + }
>
> And why this change? The removal of the __bd_forget() and the changed
> behaviour if (inode->i_bdev != NULL)?
With the above change, we don't need to care the case of igrab()
returns NULL anymore. Since inode->i_bdev->bd_inode never has the
I_FREEING, we don't need to call __bd_forget() for that case.
I think we need to address an only following simple race here.
cpu0 cpu1
-----------------------------------+-----------------------------------------
bdev = bdget(inode->i_rdev);
if (bdev) {
bdev = bdget(inode->i_rdev);
if (bdev) {
spin_lock(&bdev_lock);
if (!inode->i_bdev) {
[...]
inode->i_bdev = bdev;
spin_lock(&bdev_lock);
Thanks.
--
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2006-03-17 14:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2006-03-09 16:23 [PATCH] Fix a race condition between ->i_mapping and iput() OGAWA Hirofumi
2006-03-10 4:27 ` Andrew Morton
2006-03-10 15:30 ` OGAWA Hirofumi
2006-03-11 16:00 OGAWA Hirofumi
2006-03-16 22:49 ` Andrew Morton
2006-03-17 14:13 ` OGAWA Hirofumi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®