[PATCH] Missing BKL in sys_chroot() for 2.6

[PATCH] Missing BKL in sys_chroot() for 2.6

[BlaisorBlade]

(PLEASE cc me on replies as I'm not subscribed).

Set_fs_root *claims* it wants the BKL held:

/*
 * Replace the fs->{rootmnt,root} with {mnt,dentry}. Put the old values.
 * It can block. Requires the big lock held.
 */
void set_fs_root(struct fs_struct *fs, struct vfsmount *mnt,
                 struct dentry *dentry)

But sys_chroot ignores this. So I attach this patch (apply with patch -p1 -l). 
Maybe the solution is to kill that comment, maybe not (sys_pivot_root() calls 
chroot_fs_refs() -> set_fs_root() with the BKL held, but that has a lot of 
reasons other than this). If that comment is correct, however, it probably 
holds even for set_fs_altroot(), since it's similar. So this patch adds it.


TESTING: none, I'm sure about the inconsistency, a lot less about the solution 
(no kernel manual explains the BKL, since it is a relict; except the ones who 
still says it blocks all the rest of the kernel, while I think it is just a 
recursive and "sleepable" spinlock - am I correct?).

--- vanilla-linux-2.6.6/fs/open.c.saved 2004-05-10 21:37:11.000000000 +0200
+++ vanilla-linux-2.6.6/fs/open.c       2004-06-06 19:40:20.000000000 +0200
@@ -581,8 +581,13 @@
        if (!capable(CAP_SYS_CHROOT))
                goto dput_and_out;

+       lock_kernel();
+
        set_fs_root(current->fs, nd.mnt, nd.dentry);
        set_fs_altroot();
+
+       unlock_kernel();
+
        error = 0;
 dput_and_out:
        path_release(&nd);
--- vanilla-linux-2.6.6/fs/namei.c.saved        2004-05-10 21:37:09.000000000 
+0200
+++ vanilla-linux-2.6.6/fs/namei.c      2004-06-06 19:49:25.000000000 +0200
@@ -814,6 +814,9 @@
        return 1;
 }

+/*
+ * Requires the big lock held.
+ */
 void set_fs_altroot(void)
 {
        char *emul = __emul_prefix();

-- 
Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729

Re: [PATCH] Missing BKL in sys_chroot() for 2.6

[Linus Torvalds]

On Sun, 6 Jun 2004, BlaisorBlade wrote:
>
> (PLEASE cc me on replies as I'm not subscribed).
> 
> Set_fs_root *claims* it wants the BKL held:

I think the set_fs_root() comment is just wrong.

We properly lock the accesses to root/rootmnt with "fs->lock", and in fact 
no other users will have the BKL when accessing them anyway, so I don't 
see what the BKL would help in this case.

However, from a quick grep of users, it does look like some other users 
aren't real careful with "fs->lock" (ie chroot_fs_refs() looks like it 
could have problems - probably purely theoretical).

Al, do your eagle-eyes see something I missed?

		Linus

Re: [PATCH] Missing BKL in sys_chroot() for 2.6

[Phy Prabab]

 
> see what the BKL 

What does BKL stand for?

Thanks,
Phy


	
		
__________________________________
Do you Yahoo!?
Friends.  Fun.  Try the all-new Yahoo! Messenger.
http://messenger.yahoo.com/ 

Re: [PATCH] Missing BKL in sys_chroot() for 2.6

[Linus Torvalds]

On Mon, 7 Jun 2004, Phy Prabab wrote:
>  
> > see what the BKL 
> 
> What does BKL stand for?

"big kernel lock" aka the traditional global kernel lock that these days 
is not actually used that much any more. When you see "lock_kernel()", 
"unlock_kernel()", that's the BKL.

		Linus

Re: [PATCH] Missing BKL in sys_chroot() for 2.6

[Phy Prabab]

Thanks for the clarification.

Phy


--- Linus Torvalds <torvalds@osdl.org> wrote:
> 
> 
> On Mon, 7 Jun 2004, Phy Prabab wrote:
> >  
> > > see what the BKL 
> > 
> > What does BKL stand for?
> 
> "big kernel lock" aka the traditional global kernel
> lock that these days 
> is not actually used that much any more. When you
> see "lock_kernel()", 
> "unlock_kernel()", that's the BKL.
> 
> 		Linus
> -
> To unsubscribe from this list: send the line
> "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at 
> http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: [PATCH] Missing BKL in sys_chroot() for 2.6

[viro]

On Mon, Jun 07, 2004 at 11:56:37AM -0700, Linus Torvalds wrote:
> 
> 
> On Sun, 6 Jun 2004, BlaisorBlade wrote:
> >
> > (PLEASE cc me on replies as I'm not subscribed).
> > 
> > Set_fs_root *claims* it wants the BKL held:
> 
> I think the set_fs_root() comment is just wrong.
> 
> We properly lock the accesses to root/rootmnt with "fs->lock", and in fact 
> no other users will have the BKL when accessing them anyway, so I don't 
> see what the BKL would help in this case.
> 
> However, from a quick grep of users, it does look like some other users 
> aren't real careful with "fs->lock" (ie chroot_fs_refs() looks like it 
> could have problems - probably purely theoretical).
> 
> Al, do your eagle-eyes see something I missed?

chroot_fs_refs() is OK - it's a part of pivot_root(2) and it's just as
"if process looks like the have root and/or cwd in old root, we assume
they want to have those flipped to new one; if they are not, assume
that they know what they are doing and wouldn't like us to pull anything
on them".  IOW, here we don't really care.

selinux open_devnull(), OTOH, is bogus - they already have an fs of their
own that is not going away; so why not put the damn device node on it and
be done with that?

In any case, BKL is irrelevant - that comment should've been dropped a long
time ago.