* mprotect and hugetlb mappings
@ 2023-07-05 23:08 Mike Kravetz
2023-07-05 23:22 ` Matthew Wilcox
0 siblings, 1 reply; 6+ messages in thread
From: Mike Kravetz @ 2023-07-05 23:08 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: David Hildenbrand, Rik van Riel, Michal Hocko, Peter Xu, Dave Hansen
I was recently asked about the behavior of mprotect on a hugetlb
mapping where addr or addr+len is not hugetlb page size aligned. As
one might expect, EINVAL is returned in such cases. However, the man
page makes no mention of alignment requirements for hugetlb mappings.
I am happy to submit man page updates if people agree this is the correct
behavior. We might even want to check alignment earlier in the code
path as we fail when trying to split the vma today.
An alternative behavior would be to operate on whole hugetlb pages within
the range addr - addr+len.
--
Mike Kravetz
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: mprotect and hugetlb mappings
2023-07-05 23:08 mprotect and hugetlb mappings Mike Kravetz
@ 2023-07-05 23:22 ` Matthew Wilcox
2023-07-05 23:53 ` Mike Kravetz
0 siblings, 1 reply; 6+ messages in thread
From: Matthew Wilcox @ 2023-07-05 23:22 UTC (permalink / raw)
To: Mike Kravetz
Cc: linux-mm, linux-kernel, David Hildenbrand, Rik van Riel,
Michal Hocko, Peter Xu, Dave Hansen
On Wed, Jul 05, 2023 at 04:08:08PM -0700, Mike Kravetz wrote:
> I was recently asked about the behavior of mprotect on a hugetlb
> mapping where addr or addr+len is not hugetlb page size aligned. As
> one might expect, EINVAL is returned in such cases. However, the man
> page makes no mention of alignment requirements for hugetlb mappings.
>
> I am happy to submit man page updates if people agree this is the correct
> behavior. We might even want to check alignment earlier in the code
> path as we fail when trying to split the vma today.
>
> An alternative behavior would be to operate on whole hugetlb pages within
> the range addr - addr+len.
After a careful re-reading of the mprotect() man page, I suggest the
following behaviour ...
addr must be a multiple of the hpage size. Otherwise -EINVAL.
len should be rounded up to hpage size.
I wonder how likely this change would be to break userspace code.
Maybe some test cases.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: mprotect and hugetlb mappings
2023-07-05 23:22 ` Matthew Wilcox
@ 2023-07-05 23:53 ` Mike Kravetz
2023-07-17 16:19 ` Vlastimil Babka
0 siblings, 1 reply; 6+ messages in thread
From: Mike Kravetz @ 2023-07-05 23:53 UTC (permalink / raw)
To: Matthew Wilcox
Cc: linux-mm, linux-kernel, David Hildenbrand, Rik van Riel,
Michal Hocko, Peter Xu, Dave Hansen
On 07/06/23 00:22, Matthew Wilcox wrote:
> On Wed, Jul 05, 2023 at 04:08:08PM -0700, Mike Kravetz wrote:
> > I was recently asked about the behavior of mprotect on a hugetlb
> > mapping where addr or addr+len is not hugetlb page size aligned. As
> > one might expect, EINVAL is returned in such cases. However, the man
> > page makes no mention of alignment requirements for hugetlb mappings.
> >
> > I am happy to submit man page updates if people agree this is the correct
> > behavior. We might even want to check alignment earlier in the code
> > path as we fail when trying to split the vma today.
> >
> > An alternative behavior would be to operate on whole hugetlb pages within
> > the range addr - addr+len.
>
> After a careful re-reading of the mprotect() man page, I suggest the
> following behaviour ...
>
> addr must be a multiple of the hpage size. Otherwise -EINVAL.
> len should be rounded up to hpage size.
>
> I wonder how likely this change would be to break userspace code.
> Maybe some test cases.
My concern is that this is the approach I took with huegtlb MADV_DONTNEED,
and this caused problems discussed and eventually modified here:
https://lore.kernel.org/linux-mm/20221021154546.57df96db@imladris.surriel.com/
In the MADV_DONTNEED case we were throwing away data. With mprotect we are
only modifying access to data.
--
Mike Kravetz
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: mprotect and hugetlb mappings
2023-07-05 23:53 ` Mike Kravetz
@ 2023-07-17 16:19 ` Vlastimil Babka
2023-07-17 18:52 ` Mike Kravetz
0 siblings, 1 reply; 6+ messages in thread
From: Vlastimil Babka @ 2023-07-17 16:19 UTC (permalink / raw)
To: Mike Kravetz, Matthew Wilcox
Cc: linux-mm, linux-kernel, David Hildenbrand, Rik van Riel,
Michal Hocko, Peter Xu, Dave Hansen
On 7/6/23 01:53, Mike Kravetz wrote:
> On 07/06/23 00:22, Matthew Wilcox wrote:
>> On Wed, Jul 05, 2023 at 04:08:08PM -0700, Mike Kravetz wrote:
>> > I was recently asked about the behavior of mprotect on a hugetlb
>> > mapping where addr or addr+len is not hugetlb page size aligned. As
>> > one might expect, EINVAL is returned in such cases. However, the man
>> > page makes no mention of alignment requirements for hugetlb mappings.
>> >
>> > I am happy to submit man page updates if people agree this is the correct
>> > behavior. We might even want to check alignment earlier in the code
>> > path as we fail when trying to split the vma today.
>> >
>> > An alternative behavior would be to operate on whole hugetlb pages within
>> > the range addr - addr+len.
>>
>> After a careful re-reading of the mprotect() man page, I suggest the
>> following behaviour ...
>>
>> addr must be a multiple of the hpage size. Otherwise -EINVAL.
>> len should be rounded up to hpage size.
>>
>> I wonder how likely this change would be to break userspace code.
>> Maybe some test cases.
>
> My concern is that this is the approach I took with huegtlb MADV_DONTNEED,
> and this caused problems discussed and eventually modified here:
> https://lore.kernel.org/linux-mm/20221021154546.57df96db@imladris.surriel.com/
>
> In the MADV_DONTNEED case we were throwing away data. With mprotect we are
> only modifying access to data.
That can still confuse some userspace, no? I think realistically we can only
document the current implementation better, maybe improve it without
changing observed behavior as you suggested wrt the split vma fail. But
changing it would be dangerous.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: mprotect and hugetlb mappings
2023-07-17 16:19 ` Vlastimil Babka
@ 2023-07-17 18:52 ` Mike Kravetz
2023-07-17 19:00 ` David Hildenbrand
0 siblings, 1 reply; 6+ messages in thread
From: Mike Kravetz @ 2023-07-17 18:52 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Matthew Wilcox, linux-mm, linux-kernel, David Hildenbrand,
Rik van Riel, Michal Hocko, Peter Xu, Dave Hansen
On 07/17/23 18:19, Vlastimil Babka wrote:
> On 7/6/23 01:53, Mike Kravetz wrote:
> > On 07/06/23 00:22, Matthew Wilcox wrote:
> >> On Wed, Jul 05, 2023 at 04:08:08PM -0700, Mike Kravetz wrote:
> >> > I was recently asked about the behavior of mprotect on a hugetlb
> >> > mapping where addr or addr+len is not hugetlb page size aligned. As
> >> > one might expect, EINVAL is returned in such cases. However, the man
> >> > page makes no mention of alignment requirements for hugetlb mappings.
> >> >
> >> > I am happy to submit man page updates if people agree this is the correct
> >> > behavior. We might even want to check alignment earlier in the code
> >> > path as we fail when trying to split the vma today.
> >> >
> >> > An alternative behavior would be to operate on whole hugetlb pages within
> >> > the range addr - addr+len.
> >>
> >> After a careful re-reading of the mprotect() man page, I suggest the
> >> following behaviour ...
> >>
> >> addr must be a multiple of the hpage size. Otherwise -EINVAL.
> >> len should be rounded up to hpage size.
> >>
> >> I wonder how likely this change would be to break userspace code.
> >> Maybe some test cases.
> >
> > My concern is that this is the approach I took with huegtlb MADV_DONTNEED,
> > and this caused problems discussed and eventually modified here:
> > https://lore.kernel.org/linux-mm/20221021154546.57df96db@imladris.surriel.com/
> >
> > In the MADV_DONTNEED case we were throwing away data. With mprotect we are
> > only modifying access to data.
>
> That can still confuse some userspace, no? I think realistically we can only
> document the current implementation better, maybe improve it without
> changing observed behavior as you suggested wrt the split vma fail. But
> changing it would be dangerous.
Thanks for the comments Vlastimil.
That would be my thought/preferred path forward. Simply document the
current behavior, and MAYBE update code to be more explicit.
Any other thoughts?
--
Mike Kravetz
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: mprotect and hugetlb mappings
2023-07-17 18:52 ` Mike Kravetz
@ 2023-07-17 19:00 ` David Hildenbrand
0 siblings, 0 replies; 6+ messages in thread
From: David Hildenbrand @ 2023-07-17 19:00 UTC (permalink / raw)
To: Mike Kravetz, Vlastimil Babka
Cc: Matthew Wilcox, linux-mm, linux-kernel, Rik van Riel,
Michal Hocko, Peter Xu, Dave Hansen
On 17.07.23 20:52, Mike Kravetz wrote:
> On 07/17/23 18:19, Vlastimil Babka wrote:
>> On 7/6/23 01:53, Mike Kravetz wrote:
>>> On 07/06/23 00:22, Matthew Wilcox wrote:
>>>> On Wed, Jul 05, 2023 at 04:08:08PM -0700, Mike Kravetz wrote:
>>>>> I was recently asked about the behavior of mprotect on a hugetlb
>>>>> mapping where addr or addr+len is not hugetlb page size aligned. As
>>>>> one might expect, EINVAL is returned in such cases. However, the man
>>>>> page makes no mention of alignment requirements for hugetlb mappings.
>>>>>
>>>>> I am happy to submit man page updates if people agree this is the correct
>>>>> behavior. We might even want to check alignment earlier in the code
>>>>> path as we fail when trying to split the vma today.
>>>>>
>>>>> An alternative behavior would be to operate on whole hugetlb pages within
>>>>> the range addr - addr+len.
>>>>
>>>> After a careful re-reading of the mprotect() man page, I suggest the
>>>> following behaviour ...
>>>>
>>>> addr must be a multiple of the hpage size. Otherwise -EINVAL.
>>>> len should be rounded up to hpage size.
>>>>
>>>> I wonder how likely this change would be to break userspace code.
>>>> Maybe some test cases.
>>>
>>> My concern is that this is the approach I took with huegtlb MADV_DONTNEED,
>>> and this caused problems discussed and eventually modified here:
>>> https://lore.kernel.org/linux-mm/20221021154546.57df96db@imladris.surriel.com/
>>>
>>> In the MADV_DONTNEED case we were throwing away data. With mprotect we are
>>> only modifying access to data.
>>
>> That can still confuse some userspace, no? I think realistically we can only
>> document the current implementation better, maybe improve it without
>> changing observed behavior as you suggested wrt the split vma fail. But
>> changing it would be dangerous.
>
> Thanks for the comments Vlastimil.
>
> That would be my thought/preferred path forward. Simply document the
> current behavior, and MAYBE update code to be more explicit.
>
> Any other thoughts?
I agree.
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-07-17 19:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-05 23:08 mprotect and hugetlb mappings Mike Kravetz
2023-07-05 23:22 ` Matthew Wilcox
2023-07-05 23:53 ` Mike Kravetz
2023-07-17 16:19 ` Vlastimil Babka
2023-07-17 18:52 ` Mike Kravetz
2023-07-17 19:00 ` David Hildenbrand
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox