Prev: [PATCH] Prevent immediate process rescheduling
Next: block: Add blk_queue_copy_limits()
From: Eric W. Biederman on 18 Sep 2009 17:10

As a special case you can implement this much
more simply in devtmpfs_mount just do:

int devtmpfs_mount(const char *mountpoint)
{
sys_mount("none", "dev", "devtmpfs", MS_SILENT, NULL);
sys_chmod("dev/console", 0666);
sys_chmod("dev/tty", 0666);
sys_chmod("dev/null", 0666);
sys_chmod("dev/zero", 0666);
}

Not using sys_mount is the problem Christoph was complaining about.

Grafting dev_mount into the global namespace (instead of making
a copy and grafting that is pretty hideous). It means that
vfs_path_lookup will follow mounts, and it is a reference counting
problem. You can probably oops the kernel by going into single
user mode and unmounting devtmpfs as the code stands right now.

Eric


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Linus Torvalds on 18 Sep 2009 17:20


On Fri, 18 Sep 2009, Eric W. Biederman wrote:
>
> As a special case you can implement this much
> more simply in devtmpfs_mount just do:
>
> int devtmpfs_mount(const char *mountpoint)
> {
> sys_mount("none", "dev", "devtmpfs", MS_SILENT, NULL);
> sys_chmod("dev/console", 0666);
> sys_chmod("dev/tty", 0666);
> sys_chmod("dev/null", 0666);
> sys_chmod("dev/zero", 0666);
> }

Yeah, I think that's the way to go.

But add some error checking for sys_mount(), so that the code in question
would _never_ try to do a chmod() on some unrelated /dev/ entries.

(Ok, so the mount isn't supposed to fail, but still - conceptually that's
a really important error to check for)

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Kay Sievers on 18 Sep 2009 17:20
On Fri, Sep 18, 2009 at 23:09, Linus Torvalds
<torvalds(a)linux-foundation.org> wrote:
> On Fri, 18 Sep 2009, Eric W. Biederman wrote:
>>
>> As a special case you can implement this much
>> more simply in devtmpfs_mount just do:
>>
>> int devtmpfs_mount(const char *mountpoint)
>> {
>>         sys_mount("none", "dev", "devtmpfs", MS_SILENT, NULL);
>>       sys_chmod("dev/console", 0666);
>>       sys_chmod("dev/tty", 0666);
>>       sys_chmod("dev/null", 0666);
>>       sys_chmod("dev/zero", 0666);
>> }
>
> Yeah, I think that's the way to go.

Are we sure about console? It's not what udev does today, it stays as 0600.

Kay
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Kay Sievers on 18 Sep 2009 17:40
On Fri, 2009-09-18 at 13:58 -0700, Eric W. Biederman wrote:
> As a special case you can implement this much
> more simply in devtmpfs_mount just do:

I sent the patch to Greg already. I like to keep the things in the
subsystem where the non-default names are set. The issue will come back
from the embedded guys, who want to do be able to do that for a few more
nodes and run their stuff completely without userspace /dev. We are also
sure never to touch anything we did not create.

> int devtmpfs_mount(const char *mountpoint)
> {
> sys_mount("none", "dev", "devtmpfs", MS_SILENT, NULL);
> sys_chmod("dev/console", 0666);
> sys_chmod("dev/tty", 0666);
> sys_chmod("dev/null", 0666);
> sys_chmod("dev/zero", 0666);
> }
>
> Not using sys_mount is the problem Christoph was complaining about.
>
> Grafting dev_mount into the global namespace (instead of making
> a copy and grafting that is pretty hideous). It means that
> vfs_path_lookup will follow mounts, and it is a reference counting
> problem.

Ah, thanks.

> You can probably oops the kernel by going into single
> user mode and unmounting devtmpfs as the code stands right now.

It's busy here, and looks fine.



This change works for me here. Do you want to send or sign that off? :)

Thanks a lot,
Kay

---
devtmpfs.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
index fd488ad..685fc05 100644
--- a/drivers/base/devtmpfs.c
+++ b/drivers/base/devtmpfs.c
@@ -316,7 +316,6 @@ out:
*/
int devtmpfs_mount(const char *mountpoint)
{
- struct path path;
int err;

if (!dev_mount)
@@ -325,15 +324,11 @@ int devtmpfs_mount(const char *mountpoint)
if (!dev_mnt)
return 0;

- err = kern_path(mountpoint, LOOKUP_FOLLOW, &path);
- if (err)
- return err;
- err = do_add_mount(dev_mnt, &path, 0, NULL);
+ err = sys_mount("none", mountpoint, "devtmpfs", MS_SILENT, NULL);
if (err)
printk(KERN_INFO "devtmpfs: error mounting %i\n", err);
else
printk(KERN_INFO "devtmpfs: mounted\n");
- path_put(&path);
return err;
}



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Eric W. Biederman on 18 Sep 2009 18:10
Kay Sievers <kay.sievers(a)vrfy.org> writes:

> On Fri, 2009-09-18 at 13:58 -0700, Eric W. Biederman wrote:
>> As a special case you can implement this much
>> more simply in devtmpfs_mount just do:
>
> I sent the patch to Greg already. I like to keep the things in the
> subsystem where the non-default names are set. The issue will come back
> from the embedded guys, who want to do be able to do that for a few more
> nodes and run their stuff completely without userspace /dev. We are also
> sure never to touch anything we did not create.

There are a lot of stupid expedient things that people making embedded
products like to do. Tell them to write a 5 line /sbin/hotplug script.

udev isn't too expensive to run on the embedded platforms, and if they
have enough horse power in the system to run linux udev is almost certainly
a possibility.

> This change works for me here. Do you want to send or sign that off? :)

Nope. I don't want any piece of ownership of devtmpfs. I have sent
the only patch I am comfortable with.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: [PATCH] Prevent immediate process rescheduling
Next: block: Add blk_queue_copy_limits()