Re: code review 5574049: proc: fix unlock(&procalloc) race (from 9front) (issue 5574049)

[se...@mail.nanosouffle.net]

Aborted.

On 2012/01/23 19:47:13, nemo_lsub.org wrote:
> I'm only looking at the diff in the mail, so perhaps I'm mistaken, but
> changing break with sched does not look good to me.


> On Mon, Jan 23, 2012 at 8:43 PM, <mailto:se...@mail.nanosouffle.net>
wrote:
> > Reviewers: http://nix-dev_googlegroups.com,
> >
> > Message:
> > Hello mailto:nix...@googlegroups.com (cc:
mailto:nix...@googlegroups.com),
> >
> > I'd like you to review this change to
> > https://code.google.com/p/nix-os/
> >
> >
> > Description:
> > proc: fix unlock(&procalloc) race (from 9front)
> >
> > This one comes from Cinap, for 9front¹ and
> > seems to affect us as well. His comment:
> > "problem is, unlock(&procalloc); decrements
> > up->nlock *after* unlock! proc is on freelist
> > at this point so another cpu can come in and
> > use the proc before, zeroing up->nlock.ref!"
> >
> > ¹
> >

http://code.google.com/p/plan9front/source/detail?r=7afbe2c3d65f4aa3dea6aad9f60e479491c4c200
> >
> > Please review this at http://codereview.appspot.com/5574049/
> >
> > Affected files:
> > &nbsp;M sys/src/nix/port/proc.c
> >
> >
> > Index: sys/src/nix/port/proc.c
> > ===================================================================
> > --- a/sys/src/nix/port/proc.c
> > +++ b/sys/src/nix/port/proc.c
> > @@ -144,8 +144,11 @@
> > &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp;unlock(&pga);
> >
> > &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp;psrelease(up);
> > +
> > + &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; /* proc is free now, make sure unlock() won't touch
> > it */
> > + &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; up = procalloc.p = nil;
> > &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp;unlock(&procalloc);
> > - &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; break;
> > + &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; sched();
> > &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;}
> > &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;up->mach =
nil;
> > &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp;updatecpu(up);
> >
> >



https://codereview.appspot.com/5574049/