On 10/01/2010 12:36 AM, Serge Hallyn wrote: > Quoting Daniel Lezcano (daniel.lezcano@xxxxxxx): > >> I resurect and refreshed this old patch from >> https://lists.linux-foundation.org/pipermail/containers/2007-February/003726.html >> >> This patch makes automount to work within a container. >> >> Make autofs4 container-friendly by caching struct pid reference rather >> than pid_t and using pid_nr() to retreive a task's pid_t. >> >> ChangeLog: >> - Refreshed against linux-next (added dev-ioctl.c) >> - Fix Eric Biederman's comments - Use find_get_pid() to hold a >> reference to oz_pgrp and release while unmounting; separate out >> changes to autofs and autofs4. >> - Also rollback my earlier change to autofs_wait_queue (pid and tgid >> in the wait queue are just used to write to a userspace daemon's >> pipe). >> - Fix Cedric's comments: retain old prototype of parse_options() >> and move necessary change to its caller. >> >> Signed-off-by: Sukadev Bhattiprolu<sukadev@xxxxxxxxxx> >> Signed-off-by: Daniel Lezcano<daniel.lezcano@xxxxxxx> >> Cc: Ian Kent<raven@xxxxxxxxxx> >> Cc: Cedric Le Goater<clg@xxxxxxxxxx> >> Cc: Dave Hansen<haveblue@xxxxxxxxxx> >> Cc: Serge E. Hallyn<serge.hallyn@xxxxxxxxxxxxx> >> Cc: Eric Biederman<ebiederm@xxxxxxxxxxxx> >> Cc: Helmut Lichtenberg<heli@xxxxxxxxxx> >> --- >> [ cut ] >> @@ -133,7 +133,7 @@ static int autofs4_show_options(struct seq_file *m, struct vfsmount *mnt) >> seq_printf(m, ",uid=%u", root_inode->i_uid); >> if (root_inode->i_gid != 0) >> seq_printf(m, ",gid=%u", root_inode->i_gid); >> - seq_printf(m, ",pgrp=%d", sbi->oz_pgrp); >> + seq_printf(m, ",pgrp=%d", pid_nr(sbi->oz_pgrp)); >> seq_printf(m, ",timeout=%lu", sbi->exp_timeout/HZ); >> seq_printf(m, ",minproto=%d", sbi->min_proto); >> seq_printf(m, ",maxproto=%d", sbi->max_proto); >> @@ -263,6 +263,7 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent) >> int pipefd; >> struct autofs_sb_info *sbi; >> struct autofs_info *ino; >> + pid_t pgid; >> >> sbi = kzalloc(sizeof(*sbi), GFP_KERNEL); >> if (!sbi) >> @@ -275,7 +276,7 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent) >> sbi->pipe = NULL; >> sbi->catatonic = 1; >> sbi->exp_timeout = 0; >> - sbi->oz_pgrp = task_pgrp_nr(current); >> + sbi->oz_pgrp = task_pgrp(current); >> sbi->sb = s; >> sbi->version = 0; >> sbi->sub_version = 0; >> @@ -314,7 +315,7 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent) >> >> /* Can this call block? */ >> if (parse_options(data,&pipefd,&root_inode->i_uid,&root_inode->i_gid, >> - &sbi->oz_pgrp,&sbi->type,&sbi->min_proto, >> + &pgid,&sbi->type,&sbi->min_proto, >> &sbi->max_proto)) { >> printk("autofs: called with bogus options\n"); >> goto fail_dput; >> @@ -342,12 +343,19 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent) >> sbi->version = sbi->max_proto; >> sbi->sub_version = AUTOFS_PROTO_SUBVERSION; >> >> - DPRINTK("pipe fd = %d, pgrp = %u", pipefd, sbi->oz_pgrp); >> + DPRINTK("pipe fd = %d, pgrp = %u", pipefd, pgid); >> + >> + sbi->oz_pgrp = find_get_pid(pgid); >> > This is a little backward. You first get current's pgid pid, but don't > take a reference; then parse_options gets current's pgid pid_nr (and > keeps that if no pgid was specified), passes that back here, and here we > get the pid_nr and take a ref. I was actually first going to say that > I didn't want to block this patch on this, but it should be cleaned up > at some point (i.e. at top of this function get the struct pid and get > a ref, pass that to parse_options, and have parse_options get the > specified pgid instead if a valid one was passed in. > I agree, I will cleanup this part. Also, I noticed the: ... case Opt_pgrp: if (match_int(args, &option)) return 1; *pgrp = option; break; ... ouch ! > But now I'm wondering whether this actually is unsafe, bc I'm not quite > sure how to read the comment above task_pgrp() (in sched.h) says not > to dereference this if it wasn't gotten under task_lock or rcu_read_lock. > Which this isn't. So is this actually unsafe? > Good point. task_pgrp_nr calls __task_pid_nr_ns which does rcu_read_lock. task_pgrp does not take any lock. So you are right, replacing task_pgrp_nr by task_pgrp is unsafe. I suppose get_task_pid(current, PIDTYPE_PGID) is the right call. Thanks for looking at the patch. -- Daniel _______________________________________________ Containers mailing list Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/containers
