[Devel] Re: [PATCH 2/2] Replace pid_t in autofs with struct pid reference
Ian Kent
raven at themaw.net
Wed Mar 21 20:18:14 PDT 2007
On Wed, 2007-03-21 at 21:19 -0500, Serge E. Hallyn wrote:
> Quoting Ian Kent (raven at themaw.net):
> > On Tue, 2007-03-20 at 16:01 -0600, Eric W. Biederman wrote:
> > > "Serge E. Hallyn" <serue at us.ibm.com> writes:
> > >
> > > >> > void autofs4_dentry_release(struct dentry *);
> > > >> > extern void autofs4_kill_sb(struct super_block *);
> > > >> > diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> > > >> > index 9857543..4a9ad9b 100644
> > > >> > --- a/fs/autofs4/waitq.c
> > > >> > +++ b/fs/autofs4/waitq.c
> > > >> > @@ -141,8 +141,8 @@ static void autofs4_notify_daemon(struct
> > > >> > packet->ino = wq->ino;
> > > >> > packet->uid = wq->uid;
> > > >> > packet->gid = wq->gid;
> > > >> > - packet->pid = wq->pid;
> > > >> > - packet->tgid = wq->tgid;
> > > >> > + packet->pid = pid_nr(wq->pid);
> > > >> > + packet->tgid = pid_nr(wq->tgid);
> > > >> > break;
> > > >>
> > > >> I'm assuming we build the packet in the process context of the
> > > >> daemon we are sending it to. If not we have a problem here.
> > > >
> > > > Yes this is data being sent to a userspace daemon (Ian pls correct me if
> > > > I'm wrong) so the pid_nr is the only thing we can send.
> > >
> > > Agreed. The question is are we in the user space daemon's process when
> > > we generate the pid_nr. Or do we stuff this in some kind of socket,
> > > and the socket switch locations of the packet.
> >
> > The context here is the automount daemon only for expire runs.
> >
> > Mount request packets are triggered by user processes walking over an
> > autofs mount point directory. So "current" in this case isn't the autofs
> > daemon.
> >
> > Requests are sent via a pipe to the daemon.
>
> So is the pid used for anything other than debugging?
>
> In any case, here is a replacement patch which sends the pid number
> in the pid_namespace of the process which did the autofs4 mount.
>
> Still not sure whether that is actually what makes sense...
>
> From: "Serge E. Hallyn" <serue at us.ibm.com>
> Subject: [PATCH] autofs: prevent pid wraparound in waitqs
>
> Instead of storing pid numbers for waitqs, store references
> to struct pids. Also store a reference to the mounter's pid
> namespace in the autofs4 sb info so that pid numbers for
> mount miss and expiry msgs can send the pid# in the mounter's
> pidns.
I think this amounts to what I suggested in my previous replies.
Hopefully my comments are enough to clear up any questions on
correctness of this approach.
Sorry to be a pain but I'm having a little trouble reviewing the patch
because I'm not clear on where the code to handle the automount process
group (so called oz_pgrp), from the first patch, fits in with this.
Is this patch in addition to the original?
If so are the references to pid_nr still OK?
Ian
>
> Signed-off-by: Serge E. Hallyn <serue at us.ibm.com>
>
> ---
>
> fs/autofs4/autofs_i.h | 14 ++++++++++++--
> fs/autofs4/inode.c | 5 +++++
> fs/autofs4/waitq.c | 12 ++++++------
> include/linux/pid.h | 1 +
> kernel/pid.c | 13 ++++++++++---
> 5 files changed, 34 insertions(+), 11 deletions(-)
>
> e4184e6923f811f8a025b831ea33541fa820fd62
> diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
> index 3ccec0a..55026dd 100644
> --- a/fs/autofs4/autofs_i.h
> +++ b/fs/autofs4/autofs_i.h
> @@ -79,8 +79,8 @@ struct autofs_wait_queue {
> u64 ino;
> uid_t uid;
> gid_t gid;
> - pid_t pid;
> - pid_t tgid;
> + struct pid *pid;
> + struct pid *tgid;
> /* This is for status reporting upon return */
> int status;
> atomic_t wait_ctr;
> @@ -97,6 +97,7 @@ struct autofs_sb_info {
> int pipefd;
> struct file *pipe;
> struct pid *oz_pgrp;
> + struct pid_namespace *pidns;
> int catatonic;
> int version;
> int sub_version;
> @@ -228,5 +229,14 @@ out:
> return ret;
> }
>
> +static inline void autofs_free_wait_queue(struct autofs_wait_queue *wq)
> +{
> + if (wq->pid)
> + put_pid(wq->pid);
> + if (wq->tgid)
> + put_pid(wq->tgid);
> + kfree(wq);
> +}
> +
> void autofs4_dentry_release(struct dentry *);
> extern void autofs4_kill_sb(struct super_block *);
> diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
> index c34131a..294efd8 100644
> --- a/fs/autofs4/inode.c
> +++ b/fs/autofs4/inode.c
> @@ -20,6 +20,7 @@
> #include <linux/bitops.h>
> #include <linux/smp_lock.h>
> #include <linux/magic.h>
> +#include <linux/pid_namespace.h>
> #include "autofs_i.h"
> #include <linux/module.h>
>
> @@ -164,6 +165,7 @@ void autofs4_kill_sb(struct super_block
> autofs4_catatonic_mode(sbi); /* Free wait queues, close pipe */
>
> put_pid(sbi->oz_pgrp);
> + put_pid_ns(sbi->pidns);
>
> /* Clean up and release dangling references */
> autofs4_force_release(sbi);
> @@ -334,6 +336,8 @@ int autofs4_fill_super(struct super_bloc
> sbi->type = 0;
> sbi->min_proto = 0;
> sbi->max_proto = 0;
> + sbi->pidns = task_pid_ns(current);
> + get_pid_ns(sbi->pidns);
> mutex_init(&sbi->wq_mutex);
> spin_lock_init(&sbi->fs_lock);
> sbi->queues = NULL;
> @@ -435,6 +439,7 @@ fail_iput:
> fail_ino:
> kfree(ino);
> fail_free:
> + put_pid_ns(sbi->pidns);
> kfree(sbi);
> s->s_fs_info = NULL;
> fail_unlock:
> diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> index 9857543..30e2a90 100644
> --- a/fs/autofs4/waitq.c
> +++ b/fs/autofs4/waitq.c
> @@ -141,8 +141,8 @@ static void autofs4_notify_daemon(struct
> packet->ino = wq->ino;
> packet->uid = wq->uid;
> packet->gid = wq->gid;
> - packet->pid = wq->pid;
> - packet->tgid = wq->tgid;
> + packet->pid = __pid_nr(sbi->pidns, wq->pid);
> + packet->tgid = __pid_nr(sbi->pidns, wq->tgid);
> break;
> }
> default:
> @@ -292,8 +292,8 @@ int autofs4_wait(struct autofs_sb_info *
> wq->ino = autofs4_get_ino(sbi);
> wq->uid = current->uid;
> wq->gid = current->gid;
> - wq->pid = pid_nr(task_pid(current));
> - wq->tgid = pid_nr(task_tgid(current));
> + wq->pid = get_pid(task_pid(current));
> + wq->tgid = get_pid(task_tgid(current));
> wq->status = -EINTR; /* Status return if interrupted */
> atomic_set(&wq->wait_ctr, 2);
> mutex_unlock(&sbi->wq_mutex);
> @@ -361,7 +361,7 @@ int autofs4_wait(struct autofs_sb_info *
>
> /* Are we the last process to need status? */
> if (atomic_dec_and_test(&wq->wait_ctr))
> - kfree(wq);
> + autofs_free_wait_queue(wq);
>
> return status;
> }
> @@ -390,7 +390,7 @@ int autofs4_wait_release(struct autofs_s
> wq->status = status;
>
> if (atomic_dec_and_test(&wq->wait_ctr)) /* Is anyone still waiting for this guy? */
> - kfree(wq);
> + autofs_free_wait_queue(wq);
> else
> wake_up_interruptible(&wq->queue);
>
> diff --git a/include/linux/pid.h b/include/linux/pid.h
> index d399679..4f043bf 100644
> --- a/include/linux/pid.h
> +++ b/include/linux/pid.h
> @@ -120,6 +120,7 @@ extern void free_pid_nr(struct pid_nr *p
> extern struct pid_nr *alloc_pid_nr(struct pid_namespace *pid_ns);
> extern struct pid *alloc_pid(int clone_flags);
> extern void FASTCALL(free_pid(struct pid *pid));
> +extern pid_t __pid_nr(struct pid_namespace *ns, struct pid *pid);
> extern pid_t pid_nr(struct pid *pid);
>
>
> diff --git a/kernel/pid.c b/kernel/pid.c
> index b040e19..aa61b7e 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -299,23 +299,30 @@ int attach_pid_nr(struct pid *pid, struc
> return 0;
> }
>
> -pid_t pid_nr(struct pid *pid)
> +pid_t __pid_nr(struct pid_namespace *ns, struct pid *pid)
> {
> struct pid_nr* pid_nr;
> struct hlist_node *pos;
>
> if (!pid)
> return 0;
> -
> rcu_read_lock();
> hlist_for_each_entry_rcu(pid_nr, pos, &pid->pid_nrs, node)
> - if (pid_nr->pid_ns == task_pid_ns(current)) {
> + if (pid_nr->pid_ns == ns) {
> rcu_read_unlock();
> return pid_nr->nr;
> }
>
> rcu_read_unlock();
> return 0;
> +
> +}
> +
> +EXPORT_SYMBOL_GPL(__pid_nr);
> +
> +pid_t pid_nr(struct pid *pid)
> +{
> + return __pid_nr(task_pid_ns(current), pid);
> }
>
> EXPORT_SYMBOL_GPL(pid_nr);
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers
More information about the Devel
mailing list