Re: [PATCH v2] futex: Fix regression with read only mappings

[Shawn Bohrer <sbohrer@rgmadvisors.com>]

On Mon, Jun 27, 2011 at 01:41:12PM -0700, Darren Hart wrote:
> 
> 
> On 06/27/2011 11:15 AM, Peter Zijlstra wrote:
> > On Mon, 2011-06-27 at 11:40 -0500, Shawn Bohrer wrote:
> >>>>     if (PageAnon(page_head)) {
> >>>
> >>> This bit needs a comment too (unless I am the only one to whom this
> >> was
> >>> non-obvious), maybe:
> >>>
> >>>               
> >>>               /*
> >>>                * A read-only anonymous page implies a COW on a
> >>>                * MAP_PRIVATE mapping. There is no sane use-case
> >>>                * for this scenario, return -EFAULT to userspace.
> >>>                */
> >>
> >> Your comment is wrong.  Unfortunately the code is completly
> >> non-obvious to me as well, and I have no idea why it is there.  This
> >> little snippet came from Peter's suggested fix in:
> >>
> >> https://lkml.org/lkml/2011/6/6/368
> >>
> >> Sadly Peter's gone silent and I'm left wondering if he knew some
> >> corner case that should return -EFAULT with a RO anonymous page or if
> >> he _thought_ this was preventing RO MAP_PRIVATE mappings.  If it is
> >> the latter then this block can be removed because it does NOT do that.
> >>>> +           if (ro) {
> >>>> +                   err = -EFAULT;
> >>>> +                   goto out;
> >>>> +           }
> >>>> + 
> > 
> > Peter simply gets too much email.. anyway, the reason I put that there
> > is that a RO Anon page will never change and is thus a little pointless
> > to use for futex ops.
> >
> 
> Right, and that was the logic I was trying to document. Shawn, how is my
> comment above wrong? A read-only anonymous page but itself doesn't imply
> a COW, but it does it does in the context of this code from my reading.

All I can tell you is from my testing is a PROT_READ, MAP_PRIVATE
page isn't an anonymous page. In other words.

futex = (int *)mmap(0, sizeof(int), PROT_READ, MAP_PRIVATE, fd, 0);
rc = syscall(SYS_futex, futex, FUTEX_WAIT, val, 0, 0, 0);

Works just fine with my patch and does NOT return EFAULT.  Your
comment indicates the opposite.

--
Shawn


---------------------------------------------------------------
This email, along with any attachments, is confidential. If you 
believe you received this message in error, please contact the 
sender immediately and delete all copies of the message.  
Thank you.