lkml.org 
[lkml]   [2011]   [Jun]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] futex: Fix regression with read only mappings
On Mon, Jun 27, 2011 at 02:39:31PM -0700, Darren Hart wrote:
>
>
> On 06/27/2011 02:08 PM, Shawn Bohrer wrote:
> > 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.
>
> I see. That would suggest to me then that the get_user_pages doesn't
> force the COW when for read-only access when write access is requested.
> This makes sense from a get_user_pages perspective.
>
> Kosaki pointed out that the mapping information is contained in the VMA.
> We could test for this only if the RW get_user_pages fails, that would
> leave the common case fast, but would hurt the valid RO SHARED case. The
> alternative it seems is to just let RW private users hang themselves.

RW private users? I believe that RW private users always have been
able and still can use a futex within their mapping between threads.
The RW get_user_pages() does force a COW which means that another
process updating the underlying file won't wake up the process, but
once again this seems to have been the behavior on 2.6.18-128.7.1.el5,
2.6.32.41 and probably all other versions.

> I'm tempted to accept the latter and document it in the futex.c file and
> then in the man page.
>
> Thoughts?

So yes I vote for the patch I've been sending with perhaps a futex man
page cleanup. It does open some corner cases for RO private mappings.
You can hang on the zero page, or hang if you change the permissions
on the mapping after the fact with mprotect, but both of those things
seem very obscure to me thus I vote they should simply be documented
(I did add them to the patch description).

--
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.


\
 
 \ /
  Last update: 2011-06-28 00:21    [from the cache]
©2003-2011 Jasper Spaans