[PATCH] fs: synchronize_rcu when unregister_filesystem success not failure

[PATCH] fs: synchronize_rcu when unregister_filesystem success not failure

[Milton Miller]

While checking unregister_filesystem for saftey vs extra calls for
"ext4: register ext2 and ext3 alias after ext4" I realized that
the synchronize_rcu() was called on the error path but not on
the success path.

Should we call it in both?
Cc: stable (2.6.38)
Signed-off-by: Milton Miller <miltonm@bga.com>

Index: work.git/fs/filesystems.c
===================================================================
--- work.git.orig/fs/filesystems.c	2011-04-14 10:06:44.360068116 -0500
+++ work.git/fs/filesystems.c	2011-04-14 10:08:41.880061794 -0500
@@ -110,14 +110,13 @@ int unregister_filesystem(struct file_sy
 			*tmp = fs->next;
 			fs->next = NULL;
 			write_unlock(&file_systems_lock);
+			synchronize_rcu();
 			return 0;
 		}
 		tmp = &(*tmp)->next;
 	}
 	write_unlock(&file_systems_lock);
 
-	synchronize_rcu();
-
 	return -EINVAL;
 }

Re: [PATCH] fs: synchronize_rcu when unregister_filesystem success not failure

[Linus Torvalds]

On Thu, Apr 14, 2011 at 8:41 AM, Milton Miller <miltonm@bga.com> wrote:
>
> While checking unregister_filesystem for saftey vs extra calls for
> "ext4: register ext2 and ext3 alias after ext4" I realized that
> the synchronize_rcu() was called on the error path but not on
> the success path.

Good catch.

I think this is the bug that then caused us to do commit d863b50ab013
("vfs: call rcu_barrier after ->kill_sb()")

That said, that commit says that "synchronize_rcu()" isn't enough, and
uses rcu_barrier().

Which _should_ mean that there are no actual users that care about RCU
events by the time you actually hit "unregister_filesystem()".

So I think your patch is correct, but won't actually matter. But maybe
I'm missing something.

> Should we call it in both?

No, I think the success path is the one that would matter.

Comments?

                              Linus

Re: [PATCH] fs: synchronize_rcu when unregister_filesystem success not failure

[Marco Stornelli]

Il 14/04/2011 17:52, Linus Torvalds ha scritto:
> On Thu, Apr 14, 2011 at 8:41 AM, Milton Miller<miltonm@bga.com>  wrote:
>>
>> While checking unregister_filesystem for saftey vs extra calls for
>> "ext4: register ext2 and ext3 alias after ext4" I realized that
>> the synchronize_rcu() was called on the error path but not on
>> the success path.
>
> Good catch.
>
> I think this is the bug that then caused us to do commit d863b50ab013
> ("vfs: call rcu_barrier after ->kill_sb()")
>
> That said, that commit says that "synchronize_rcu()" isn't enough, and
> uses rcu_barrier().
>
> Which _should_ mean that there are no actual users that care about RCU
> events by the time you actually hit "unregister_filesystem()".
>
> So I think your patch is correct, but won't actually matter. But maybe
> I'm missing something.
>
>> Should we call it in both?
>
> No, I think the success path is the one that would matter.
>
> Comments?
>

If I well remember the rcu_barrier was needed for the fs module 
unloading problem. In that case synchronize_rcu() wasn't enough. That 
said, I agree with you, it won't have any impact.

Marco

Re: [PATCH] fs: synchronize_rcu when unregister_filesystem success not failure

[Mark Lord]

On 11-04-14 11:52 AM, Linus Torvalds wrote:
> On Thu, Apr 14, 2011 at 8:41 AM, Milton Miller <miltonm@bga.com> wrote:
>>
>> While checking unregister_filesystem for saftey vs extra calls for
>> "ext4: register ext2 and ext3 alias after ext4" I realized that
>> the synchronize_rcu() was called on the error path but not on
>> the success path.
> 
> Good catch.
> 
> I think this is the bug that then caused us to do commit d863b50ab013
> ("vfs: call rcu_barrier after ->kill_sb()")
> 
> That said, that commit says that "synchronize_rcu()" isn't enough, and
> uses rcu_barrier().
> 
> Which _should_ mean that there are no actual users that care about RCU
> events by the time you actually hit "unregister_filesystem()".

Is that true of older kernels?  (eg. 2.6.38 has the same bug)

IOW, is this a -stable candidate?