LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Serge E. Hallyn" <serue@us.ibm.com>
To: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: "Serge E. Hallyn" <serue@us.ibm.com>, Ian Kent <raven@themaw.net>,
	Cedric Le Goater <clg@fr.ibm.com>,
	sukadev@us.ibm.com, Andrew Morton <akpm@osdl.org>,
	Dave Hansen <haveblue@us.ibm.com>,
	Herbert Poetzl <herbert@13thfloor.at>,
	containers@lists.osdl.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] Replace pid_t in autofs with struct pid reference
Date: Tue, 20 Mar 2007 15:15:48 -0500	[thread overview]
Message-ID: <20070320201548.GA21312@sergelap.austin.ibm.com> (raw)
In-Reply-To: <m1odmpvuft.fsf@ebiederm.dsl.xmission.com>

Quoting Eric W. Biederman (ebiederm@xmission.com):
> "Serge E. Hallyn" <serue@us.ibm.com> writes:
> 
> >> 
> >> >> Index: 2.6.20/fs/autofs4/waitq.c
> >> >> ===================================================================
> >> >> --- 2.6.20.orig/fs/autofs4/waitq.c
> >> >> +++ 2.6.20/fs/autofs4/waitq.c
> >> >> @@ -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 = current->pid;
> >> >> -		wq->tgid = current->tgid;
> >> >> +		wq->pid = pid_nr(task_pid(current));
> >> >> +		wq->tgid = pid_nr(task_tgid(current));
> >> >>  		wq->status = -EINTR; /* Status return if interrupted */
> >> >>  		atomic_set(&wq->wait_ctr, 2);
> >> >>  		mutex_unlock(&sbi->wq_mutex);
> >> 
> >> I have a concern with this bit as I my quick review said the wait queue
> >> persists, and if so we should be cache the struct pid pointer, not the
> >> pid_t value.  Heck the whol pid_nr(task_xxx(current)) idiom I find very
> >> suspicious.
> >
> > Based just on what I see right here I agree it seems like we would want
> > to store a ref to the pid, not store the pid_nr(pid) output, so in this
> > context it is suspicious.
> 
> So that far we are in agreement.

So how about the following on top of the patch Cedric sent out?

Subject: [PATCH] autofs4: store struct pids in autofs_waitqs
From: Serge Hallyn <serue@us.ibm.com>
Date: 1174412305 -0500

Store struct pids in autofs_waitqs in place of pidnrs to prevent
pid overflow problems.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>

---

 fs/autofs4/autofs_i.h |   13 +++++++++++--
 fs/autofs4/waitq.c    |   15 ++++++++-------
 2 files changed, 19 insertions(+), 9 deletions(-)

86a5866c7672b88d48380504977496d5637642ab
diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
index 3ccec0a..5d119e3 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;
@@ -228,5 +228,14 @@ out:
 	return ret;
 }
 
+static 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/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;
 	}
 	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);
@@ -360,8 +360,9 @@ int autofs4_wait(struct autofs_sb_info *
 	status = wq->status;
 
 	/* Are we the last process to need status? */
-	if (atomic_dec_and_test(&wq->wait_ctr))
-		kfree(wq);
+	if (atomic_dec_and_test(&wq->wait_ctr)) {
+		autofs_free_wait_queue(wq);
+	}
 
 	return status;
 }
@@ -390,7 +391,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);
 
-- 
1.1.6

  parent reply	other threads:[~2007-03-20 20:15 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-03-13  4:51 sukadev
2007-03-16  4:04 ` Ian Kent
2007-03-16 11:32   ` Eric W. Biederman
2007-03-16 14:31     ` Ian Kent
2007-03-16 14:44       ` Cedric Le Goater
2007-03-16 16:46         ` Ian Kent
2007-03-16 19:21           ` Eric W. Biederman
2007-03-19 20:08             ` Serge E. Hallyn
2007-03-19 20:40               ` Eric W. Biederman
2007-03-19 21:19                 ` Serge E. Hallyn
2007-03-19 21:43                   ` Eric W. Biederman
2007-03-20 20:15                 ` Serge E. Hallyn [this message]
2007-03-20 20:45                   ` Eric W. Biederman
2007-03-20 21:41                     ` Serge E. Hallyn
2007-03-20 22:01                       ` Eric W. Biederman
2007-03-21 20:58                         ` Serge E. Hallyn
2007-03-22  2:28                           ` Ian Kent
2007-03-22 14:33                             ` Herbert Poetzl
2007-03-22 15:03                               ` Ian Kent
2007-03-22 15:29                                 ` Eric W. Biederman
2007-03-22 15:22                             ` Eric W. Biederman
2007-03-22 18:07                             ` Serge E. Hallyn
2007-03-22  2:00                         ` Ian Kent
2007-03-22  2:19                           ` Serge E. Hallyn
2007-03-22  3:18                             ` Ian Kent
2007-03-22 13:31                               ` Serge E. Hallyn
2007-03-22 14:48                                 ` Ian Kent
2007-03-22 15:06                                 ` Ian Kent
2007-03-22  6:43                             ` Eric W. Biederman
2007-03-22 13:28                               ` Serge E. Hallyn

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20070320201548.GA21312@sergelap.austin.ibm.com \
    --to=serue@us.ibm.com \
    --cc=akpm@osdl.org \
    --cc=clg@fr.ibm.com \
    --cc=containers@lists.osdl.org \
    --cc=ebiederm@xmission.com \
    --cc=haveblue@us.ibm.com \
    --cc=herbert@13thfloor.at \
    --cc=linux-kernel@vger.kernel.org \
    --cc=raven@themaw.net \
    --cc=sukadev@us.ibm.com \
    --subject='Re: [PATCH 2/2] Replace pid_t in autofs with struct pid reference' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).