From: Kirill Korotaev <dev@sw.ru>
This patch fixes race in writeback_inodes() described below:
writeback_inodes()
{
....
sb->s_count++;
spin_unlock(&sb_lock);
....
spin_lock(&sb_lock);
if (__put_super(sb)) <<< X
goto restart;
}
}
deactivate_super()
{
fs->kill_sb(s);
kill_block_super(sb)
generic_shutdown_super(sb)
spin_lock(&sb_lock);
list_del(&sb->s_list); <<< Y
spin_unlock(&sb_lock);
....
put_super(s);
spin_lock(&sb_lock);
__put_super(sb); <<< Z
spin_unlock(&sb_lock);
}
The problem with it is that writeback_inodes() supposes that if
__put_super() returns 0 then no super block was deleted from the list and
we can safely traverse sb list further.
But as it is obvious from the deactivate_super() it's not actually true.
because at point Y we delete super block from the list and drop the lock.
We do __put_super() very much later... So we can find sb with poisoned
sb->s_list at point X and we won't be the last sb reference holders. The
last reference will be dropped in point Z.
So in case of the following sequence of execution Y -> X -> Z we'll get an
oops after point X in writeback_inodes().
This patch introduces __put_super_and_need_restart() function which allows
safe traversing of sb list. I'll send a couple of patches later which
remove O(n^2) algos and using this function.
Signed-Off-By: Kirill Korotaev <dev@sw.ru>
Signed-off-by: Andrew Morton <akpm@osdl.org>
---
25-akpm/fs/fs-writeback.c | 2 +-
25-akpm/fs/super.c | 26 ++++++++++++++++++++++++--
25-akpm/include/linux/fs.h | 1 +
3 files changed, 26 insertions(+), 3 deletions(-)
diff -puN fs/fs-writeback.c~fix-of-race-in-writeback_inodes fs/fs-writeback.c
--- 25/fs/fs-writeback.c~fix-of-race-in-writeback_inodes 2004-09-21 01:57:32.195543352 -0700
+++ 25-akpm/fs/fs-writeback.c 2004-09-21 01:57:32.201542440 -0700
@@ -441,7 +441,7 @@ restart:
up_read(&sb->s_umount);
}
spin_lock(&sb_lock);
- if (__put_super(sb))
+ if (__put_super_and_need_restart(sb))
goto restart;
}
if (wbc->nr_to_write <= 0)
diff -puN fs/super.c~fix-of-race-in-writeback_inodes fs/super.c
--- 25/fs/super.c~fix-of-race-in-writeback_inodes 2004-09-21 01:57:32.196543200 -0700
+++ 25-akpm/fs/super.c 2004-09-21 01:57:32.202542288 -0700
@@ -319,6 +319,27 @@ int __put_super(struct super_block *sb)
return ret;
}
+/*
+ * Drop a superblock's refcount.
+ * Returns non-zero if the superblock is about to be destroyed and
+ * at least is already removed from super_blocks list, so if we are
+ * making a loop through super blocks then we need to restart.
+ * The caller must hold sb_lock.
+ */
+int __put_super_and_need_restart(struct super_block *sb)
+{
+ /* check for race with generic_shutdown_super() */
+ if (list_empty(&sb->s_list)) {
+ /* super block is removed, need to restart... */
+ __put_super(sb);
+ return 1;
+ }
+ /* can't be the last, since s_list is still in use */
+ sb->s_count--;
+ BUG_ON(sb->s_count == 0);
+ return 0;
+}
+
/**
* put_super - drop a temporary reference to superblock
* @s: superblock in question
@@ -433,7 +454,8 @@ void generic_shutdown_super(struct super
unlock_super(sb);
}
spin_lock(&sb_lock);
- list_del(&sb->s_list);
+ /* should be initialized for __put_super_and_need_restart() */
+ list_del_init(&sb->s_list);
list_del(&sb->s_instances);
spin_unlock(&sb_lock);
up_write(&sb->s_umount);
@@ -485,7 +507,7 @@ retry:
}
s->s_type = type;
strlcpy(s->s_id, type->name, sizeof(s->s_id));
- list_add(&s->s_list, super_blocks.prev);
+ list_add_tail(&s->s_list, &super_blocks);
list_add(&s->s_instances, &type->fs_supers);
spin_unlock(&sb_lock);
get_filesystem(type);
diff -puN include/linux/fs.h~fix-of-race-in-writeback_inodes include/linux/fs.h
--- 25/include/linux/fs.h~fix-of-race-in-writeback_inodes 2004-09-21 01:57:32.198542896 -0700
+++ 25-akpm/include/linux/fs.h 2004-09-21 01:57:32.204541984 -0700
@@ -1225,6 +1225,7 @@ struct super_block *sget(struct file_sys
struct super_block *get_sb_pseudo(struct file_system_type *, char *,
struct super_operations *ops, unsigned long);
int __put_super(struct super_block *sb);
+int __put_super_and_need_restart(struct super_block *sb);
void unnamed_dev_init(void);
/* Alas, no aliases. Too much hassle with bringing module.h everywhere */
_