Skip to contentSkip to conversation

cgroups: fix possible use after free

PATCH In review Li Zefan 4 participants 10 messages Feb 10, 2009 – Feb 11, 2009 2 messages not in the archive

lore
Share & tools
lore

Li Zefan

author
#0 lore raw

PATCH 1/1 cgroups: fix possible use after free

In cgroup_kill_sb(), root is freed before sb is detached from the list, so another sget() may find this sb and call cgroup_test_super(), which will access the root that has been freed.

Reported-by Al Viro <[email protected]>
Signed-off-by Li Zefan <[email protected]>
---
 kernel/cgroup.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/cgroup.c +1 -1
index 5a54ff4..42fd3f2 100644
@@ -1122,8 +1122,8 @@ static void cgroup_kill_sb(struct super_block *sb) {
 
 	mutex_unlock(&cgroup_mutex);
 
-	kfree(root);
 	kill_litter_super(sb);
+	kfree(root);
 }
 
 static struct file_system_type cgroup_fs_type = {
Signature · 1 line
1.5.4.rc3

A message that is not in the archive (sent off-list or not yet received): [email protected]

#2 lore raw
Quoted 3 lines · Li Zefan

In cgroup_kill_sb(), root is freed before sb is detached from the list, so another sget() may find this sb and call cgroup_test_super(), which will access the root that has been freed.

I think that I'd assumed that by the time we get to cgroup_kill_sb() there's no chance of the sb being resurrected by sget().

If anyone can call cgroup_test_super() on the sb that we're trying to free, isn't that bad? Since even with this patch we free root just after calling kill_litter_sb(), if anyone tries to reuse that super block then they're toast.

Would it be better to set sb->s_fs_info to NULL, before calling kill_litter_sb(), and have cgroup_test_super() just return 0 for any sb with a NULL sb->s_fs_info.

Paul

Quoted 24 lines
Reported-by Al Viro <[email protected]>
Signed-off-by Li Zefan <[email protected]>
---
 kernel/cgroup.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/cgroup.c +1 -2
index 5a54ff4..42fd3f2 100644
@@ -1122,8 +1122,8 @@ static void cgroup_kill_sb(struct super_block *sb) {
 
       mutex_unlock(&cgroup_mutex);
 
-       kfree(root);
       kill_litter_super(sb);
+       kfree(root);
 }
 
 static struct file_system_type cgroup_fs_type = {
--

1.5.4.rc3

#4 lore raw
In reply to Li Zefan
Quoted 3 lines · Li Zefan

In cgroup_kill_sb(), root is freed before sb is detached from the list, so another sget() may find this sb and call cgroup_test_super(), which will access the root that has been freed.

I think that I'd assumed that by the time we get to cgroup_kill_sb() there's no chance of the sb being resurrected by sget().

If anyone can call cgroup_test_super() on the sb that we're trying to free, isn't that bad? Since even with this patch we free root just after calling kill_litter_sb(), if anyone tries to reuse that super block then they're toast.

Would it be better to set sb->s_fs_info to NULL, before calling kill_litter_sb(), and have cgroup_test_super() just return 0 for any sb with a NULL sb->s_fs_info.

Paul

Quoted 24 lines
Reported-by Al Viro <[email protected]>
Signed-off-by Li Zefan <[email protected]>
---
 kernel/cgroup.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/cgroup.c +1 -2
index 5a54ff4..42fd3f2 100644
@@ -1122,8 +1122,8 @@ static void cgroup_kill_sb(struct super_block *sb) {
 
       mutex_unlock(&cgroup_mutex);
 
-       kfree(root);
       kill_litter_super(sb);
+       kfree(root);
 }
 
 static struct file_system_type cgroup_fs_type = {
--

1.5.4.rc3

#5 lore raw
Quoted 7 lines · Paul Menage
Quoted 3 lines · Li Zefan

In cgroup_kill_sb(), root is freed before sb is detached from the list, so another sget() may find this sb and call cgroup_test_super(), which will access the root that has been freed.

I think that I'd assumed that by the time we get to cgroup_kill_sb() there's no chance of the sb being resurrected by sget().

There is none. grab_super() will fail to get it, so sget() will go through retry logics. Which doesn't mean that test won't be called on it in the meanwhile.

#6 lore raw
Quoted 6 lines · Al Viro
Quoted 5 lines · Paul Menage
Quoted 3 lines · Li Zefan

In cgroup_kill_sb(), root is freed before sb is detached from the list, so another sget() may find this sb and call cgroup_test_super(), which will access the root that has been freed.

Quoted 6 lines
Quoted 2 lines

I think that I'd assumed that by the time we get to cgroup_kill_sb() there's no chance of the sb being resurrected by sget().

There is none. grab_super() will fail to get it, so sget() will go through retry logics. Which doesn't mean that test won't be called on it in the meanwhile.

OK, so Zefan's patch looks like the safest way to fix this particular issue. I think I see some other potential races with cgroup_test_super() though - we probably need to synchronize against the changing of a root's subsys_bits in rebind_subsystems(). Taking cgroup_mutex around the call to sget() would certainly provide that, but I'd have to check whether it causes locking cycles.

Paul

#7 lore raw
Quoted 14 lines · Paul Menage
Quoted 6 lines · Al Viro
Quoted 5 lines · Paul Menage
Quoted 3 lines · Li Zefan

In cgroup_kill_sb(), root is freed before sb is detached from the list, so another sget() may find this sb and call cgroup_test_super(), which will access the root that has been freed.

Quoted 6 lines
Quoted 2 lines

I think that I'd assumed that by the time we get to cgroup_kill_sb() there's no chance of the sb being resurrected by sget().

There is none. grab_super() will fail to get it, so sget() will go through retry logics. Which doesn't mean that test won't be called on it in the meanwhile.

Quoted 6 lines

OK, so Zefan's patch looks like the safest way to fix this particular issue. I think I see some other potential races with cgroup_test_super() though - we probably need to synchronize against the changing of a root's subsys_bits in rebind_subsystems(). Taking cgroup_mutex around the call to sget() would certainly provide that, but I'd have to check whether it causes locking cycles.

Deadlock. The whole point of what sget() is doing with retries is this:
if you have a matching fs in the middle of shutdown, you obviously can't pick its sb (it's halfway shut down) and you can't start bringing your instance up until the halfdead one is gone.

So you can't wrap sget() into a mutex that might be grabbed at some point of ->kill_sb() or it'll wait forever in that scenario. Note that test is essentially atomic; we are doing it fast for all superblocks of this type.

If we find a match, we go ahead and pick that superblock. So there's no problem just assuming we'd won the race, got existing superblock and change (if any) of ->subsys_bits simply happened just after that.

AFAICS, that's perfectly sane - you are talking about the race between mount picking fs instance with the same options and remount changing said instance. Other callers of rebind_subsystems() do not apply - one in your ->get_sb() won't change the field anyway and one in ->kill_sb() is irrelevant since the superblock will be rejected by grab_super().

So no exclusion is needed there at all. If you don't want later remount of the first mount to affect these flags of second one you shouldn't share the superblock at all, obviously...

A message that is not in the archive (sent off-list or not yet received): 20090211011933.GB28946-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org

#9 lore raw
Quoted 3 lines · Al Viro

So you can't wrap sget() into a mutex that might be grabbed at some point of ->kill_sb() or it'll wait forever in that scenario.

Good point.

Quoted 4 lines

So no exclusion is needed there at all. If you don't want later remount of the first mount to affect these flags of second one you shouldn't share the superblock at all, obviously...

I have plans for some slightly more complex state in that structure - I'll probably just add a spinlock around accesses to those fields so that we can avoid exposing any inconsistent state.

Paul

#10 lore raw
In reply to Al Viro
Quoted 3 lines · Al Viro

So you can't wrap sget() into a mutex that might be grabbed at some point of ->kill_sb() or it'll wait forever in that scenario.

Good point.

Quoted 4 lines

So no exclusion is needed there at all. If you don't want later remount of the first mount to affect these flags of second one you shouldn't share the superblock at all, obviously...

I have plans for some slightly more complex state in that structure - I'll probably just add a spinlock around accesses to those fields so that we can avoid exposing any inconsistent state.

Paul