* Re: [PATCH] mm: allow unmapped hole at head side of mbind range
2019-10-24 10:48 ` [PATCH] mm: allow unmapped hole at head side of mbind range Vlastimil Babka
@ 2019-10-25 2:32 ` Hugh Dickins
0 siblings, 0 replies; 2+ messages in thread
From: Hugh Dickins @ 2019-10-25 2:32 UTC (permalink / raw)
To: Li Xinhai
Cc: Vlastimil Babka, Michal Hocko, linux-mm@kvack.org,
linux-kernel@vger.kernel.org, Linux API
On Thu, 24 Oct 2019, Vlastimil Babka wrote:
> + linux-api
>
> On 10/24/19 9:35 AM, Li Xinhai wrote:
> > From: Li Xinhai <xinhai.li@outlook.com>
> >
> > mbind_range silently ignore unmapped hole at middle and tail of the
> > specified range, but report EFAULT if hole at head side.
>
>
> Hmm that's unfortunate. mbind() manpage says:
>
> EFAULT Part or all of the memory range specified by nodemask and maxnode
> points outside your accessible address space. Or, there was an unmapped
> hole in the specified memory range specified by addr and len.
>
> That sounds like any hole inside the specified range should return
> EFAULT.
Yes (though an exception is allowed when restoring to default).
> But perhaps it can be also interpreted as you suggest, that the
> whole range is an unmapped hole. There's some risk of breaking existing
> userspace if we change it either way.
>
> > It is more reasonable to support silently ignore holes at any part of
> > the range, only report EFAULT if the whole range is in hole.
> >
> > Signed-off-by: Li Xinhai <xinhai.li@outlook.com>
Xinhai, I'm sceptical about this patch: is it something you found
by code inspection, or something you found when using mbind()?
I've not looked long enough to be certain, nor experimented, but:
mbind_range() is only one stage of the mbind() syscall implementation,
and is preceded by queue_pages_range(): look what queue_pages_test_walk()
does when MPOL_MF_DISCONTIG_OK not set.
My impression is that mbind_range() is merely correcting an omission
from the checks already made my queue_pages_test_walk() (an odd way
to proceed, I admit: would be better to check initially than later).
I do think that you should not make this change without considering
MPOL_MF_DISCONTIG_OK and its intention.
Hugh
> > ---
> >
> > mm/mempolicy.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >
> > diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> > index 4ae967bcf954..ae160d9936d9 100644
> > --- a/mm/mempolicy.c
> > +++ b/mm/mempolicy.c
> > @@ -738,7 +738,7 @@ static int mbind_range(struct mm_struct *mm, unsigned long start,
> > unsigned long vmend;
> >
> > vma = find_vma(mm, start);
> > - if (!vma || vma->vm_start > start)
> > + if (!vma || vma->vm_start >= end)
> > return -EFAULT;
> >
> > prev = vma->vm_prev;
> >
^ permalink raw reply [flat|nested] 2+ messages in thread