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

[Darren Hart <dvhart@linux.intel.com>]

On 06/27/2011 03:14 PM, Shawn Bohrer wrote:
> 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?  

Sorry, I meant "RO 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).


I'm concerned about the zero page. I need to go re-read all of Kosaki's
last round of patches related to the zero page. I'll look at this
tonight or tomorrow, but if I remember correctly, the zero page left a
DOS opportunity - we can't re-introduce something like that.

Thanks,

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel