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

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

[akumar, 2012-01-23 19:43:26]

Hello nix-dev@googlegroups.com (cc: nix-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/nix-os/

[nemo_lsub.org, 2012-01-23 19:47:13]

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,  <seed@mail.nanosouffle.net> wrote:
> Reviewers: nix-dev_googlegroups.com,
>
> Message:
> Hello nix-dev@googlegroups.com (cc: nix-dev@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=7afbe2c3d65f4aa3dea6aad9f...
>
> Please review this at http://codereview.appspot.com/5574049/
>
> Affected files:
>  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 @@
>                        unlock(&pga);
>
>                        psrelease(up);
> +
> +                       /* proc is free now, make sure unlock() won't touch
> it */
> +                       up = procalloc.p = nil;
>                        unlock(&procalloc);
> -                       break;
> +                       sched();
>                }
>                up->mach = nil;
>                updatecpu(up);
>
>

[akumar, 2012-04-24 19:52:09]

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:seed@mail.nanosouffle.net> wrote:
> > Reviewers: http://nix-dev_googlegroups.com,
> >
> > Message:
> > Hello mailto:nix-dev@googlegroups.com (cc: mailto:nix-dev@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=7afbe2c3d65f4aa3dea6aad9f...
> >
> > 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);
> >
> >