Re: [BProc] ps oops on 3.2.0

[<er...@he...>]

On Fri, Oct 18, 2002 at 11:18:13AM -0400, er...@he... wrote:
> On Fri, Oct 18, 2002 at 10:46:57AM -0400, Nicholas Henke wrote:
> > On Fri, 18 Oct 2002 10:25:27 -0400
> > er...@he... wrote:
> > 
> > > On Wed, Oct 16, 2002 at 04:06:26PM -0400, Nicholas Henke wrote:
> > > > 
> > > > I can cause a node to oops everytime with the attached script. I am
> > > > running bproc-3.2.0 on 2.4.18, with the procfs locking patch. 
> > > > Here is the oops fed through ksymoops after rebooting.
> > > 
> > > In general, I'd say I don't worry about co-existing nicely other
> > > patches.  It's just too much trouble for me.  If you point me at the
> > > other patch you're using I might be able to see what the problem is
> > > pretty quickly though.  More likely, the locking is all moved around
> > > in procfs and the BProc hooks need to be modified accordingly.  The
> > > BProc hooks into procfs are pretty messy since things need to be done
> > > atomically and there's a bunch of lock grabbing/releasing code already
> > > in there.
> > > 
> > Sorry for the confustion. -- it is the patch that you sent me for the
> > last oops I sent, where we moved the bproc_hook_imv call inside the lock
> > in fs/proc/array.c ( I think that is the file.)
> 
> Oh, I see.  I should extend that policy to my own patches :)
> 
> Seriously though, I took your script and reproduced it so I'm looking
> at it now.

Ok, I found a race in the procfs code with PID mapping.  Here's a
patch to actually fix the problem.  Apply this to bproc/kernel/hooks.c

It looked like your crash dump may have come from elsewhere so I hope
the problem I'm seeing is the same one you're seeing.  In anycase your
little test script stopped killing the slave node after applying this
one.

- Erik



diff -u -r1.48 -r1.49
--- hooks.c	27 Sep 2002 19:22:32 -0000	1.48
+++ hooks.c	18 Oct 2002 16:43:29 -0000	1.49
@@ -17,7 +17,7 @@
  *  along with this program; if not, write to the Free Software
  *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  *
- * $Id: hooks.c,v 1.48 2002/09/27 19:22:32 hendriks Exp $
+ * $Id: hooks.c,v 1.49 2002/10/18 16:43:29 hendriks Exp $
  *-----------------------------------------------------------------------*/
 #define __NO_VERSION__
 
@@ -681,12 +681,21 @@
 
 static
 int bproc_hook_proc_pid(struct task_struct *p) {
-    return do_pid_mapping() ? p->bproc.masq->pid : p->pid;
+    if (!do_pid_mapping()) return p->pid;
+    /* Due to the fact that readdir and opening/reading the process
+       files is not an atomic operation, possibility that p will no
+       longer be a masqueraded process by the time we get here.  If
+       that's the case, just return 0 for the pid. */
+    if (!BPROC_ISMASQ(p)) return 0;
+    return p->bproc.masq->pid;
 }
 
 static
 int bproc_hook_proc_ppid(struct task_struct *p) {
-    return do_pid_mapping() ? p->bproc.masq->ppid : p->p_opptr->pid;
+    if (!do_pid_mapping()) return p->pid ? p->p_opptr->pid : 0;
+    /* See the note in bproc_hook_proc_pid... */
+    if (!BPROC_ISMASQ(p))  return 0;
+    return p->bproc.masq->ppid;
 }
 
 static