Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1503120 > unrolled thread

Re: [PATCH 00/10] mm: adjust get_user_pages* functions to explicitly pass FOLL_* flags

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 00/10] mm: adjust get_user_pages* functions to explicitly  pass FOLL_* flags Michal Hocko <mhocko@kernel.org> - 2016-10-18 17:40 +0200
    Re: [PATCH 00/10] mm: adjust get_user_pages* functions to explicitly  pass FOLL_* flags Michal Hocko <mhocko@kernel.org> - 2016-10-19 16:20 +0200
      Re: [PATCH 00/10] mm: adjust get_user_pages* functions to explicitly  pass FOLL_* flags Dave Hansen <dave.hansen@linux.intel.com> - 2016-10-19 19:00 +0200
        Re: [PATCH 00/10] mm: adjust get_user_pages* functions to explicitly  pass FOLL_* flags Michal Hocko <mhocko@kernel.org> - 2016-10-19 19:10 +0200
          Re: [PATCH 00/10] mm: adjust get_user_pages* functions to explicitly  pass FOLL_* flags Dave Hansen <dave.hansen@linux.intel.com> - 2016-10-19 19:30 +0200
            Re: [PATCH 00/10] mm: adjust get_user_pages* functions to explicitly  pass FOLL_* flags Michal Hocko <mhocko@kernel.org> - 2016-10-20 21:30 +0200
    Re: [PATCH 00/10] mm: adjust get_user_pages* functions to explicitly  pass FOLL_* flags Lorenzo Stoakes <lstoakes@gmail.com> - 2016-10-19 18:20 +0200
    Re: [PATCH 00/10] mm: adjust get_user_pages* functions to explicitly  pass FOLL_* flags Lorenzo Stoakes <lstoakes@gmail.com> - 2016-10-26 11:00 +0200

#1503120 — Re: [PATCH 00/10] mm: adjust get_user_pages* functions to explicitly pass FOLL_* flags

On Thu 13-10-16 01:20:10, Lorenzo Stoakes wrote:
> This patch series adjusts functions in the get_user_pages* family such that
> desired FOLL_* flags are passed as an argument rather than implied by flags.
> 
> The purpose of this change is to make the use of FOLL_FORCE explicit so it is
> easier to grep for and clearer to callers that this flag is being used. The use
> of FOLL_FORCE is an issue as it overrides missing VM_READ/VM_WRITE flags for the
> VMA whose pages we are reading from/writing to, which can result in surprising
> behaviour.
> 
> The patch series came out of the discussion around commit 38e0885, which
> addressed a BUG_ON() being triggered when a page was faulted in with PROT_NONE
> set but having been overridden by FOLL_FORCE. do_numa_page() was run on the
> assumption the page _must_ be one marked for NUMA node migration as an actual
> PROT_NONE page would have been dealt with prior to this code path, however
> FOLL_FORCE introduced a situation where this assumption did not hold.
> 
> See https://marc.info/?l=linux-mm&m=147585445805166 for the patch proposal.

I like this cleanup. Tracking FOLL_FORCE users was always a nightmare
and the flag behavior is really subtle so we should better be explicit
about it. I haven't gone through each patch separately but rather
applied the whole series and checked the resulting diff. This all seems
OK to me and feel free to add
Acked-by: Michal Hocko <mhocko@suse.com>

I am wondering whether we can go further. E.g. it is not really clear to
me whether we need an explicit FOLL_REMOTE when we can in fact check
mm != current->mm and imply that. Maybe there are some contexts which
wouldn't work, I haven't checked.

Then I am also wondering about FOLL_TOUCH behavior.
__get_user_pages_unlocked has only few callers which used to be
get_user_pages_unlocked before 1e9877902dc7e ("mm/gup: Introduce
get_user_pages_remote()"). To me a dropped FOLL_TOUCH seems
unintentional. Now that get_user_pages_unlocked has gup_flags argument I
guess we might want to get rid of the __g-u-p-u version altogether, no?

__get_user_pages is quite low level and imho shouldn't be exported. It's
only user - kvm - should rather pull those two functions to gup instead
and export them. There is nothing really KVM specific in them.

I also cannot say I would be entirely thrilled about get_user_pages_locked,
we only have one user which can simply do lock g-u-p unlock AFAICS.

I guess there is more work in that area and I do not want to impose all
that work on you, but I couldn't resist once I saw you playing in that
area ;) Definitely a good start!
-- 
Michal Hocko
SUSE Labs

[toc] | [next] | [standalone]

#1503623

On Wed 19-10-16 09:58:15, Lorenzo Stoakes wrote:
> On Tue, Oct 18, 2016 at 05:30:50PM +0200, Michal Hocko wrote:
> > I am wondering whether we can go further. E.g. it is not really clear to
> > me whether we need an explicit FOLL_REMOTE when we can in fact check
> > mm != current->mm and imply that. Maybe there are some contexts which
> > wouldn't work, I haven't checked.
> 
> This flag is set even when /proc/self/mem is used. I've not looked deeply into
> this flag but perhaps accessing your own memory this way can be considered
> 'remote' since you're not accessing it directly. On the other hand, perhaps this
> is just mistaken in this case?

My understanding of the flag is quite limited as well. All I know it is
related to protection keys and it is needed to bypass protection check.
See arch_vma_access_permitted. See also 1b2ee1266ea6 ("mm/core: Do not
enforce PKEY permissions on remote mm access").

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1504137

On 10/19/2016 02:07 AM, Michal Hocko wrote:
> On Wed 19-10-16 09:58:15, Lorenzo Stoakes wrote:
>> On Tue, Oct 18, 2016 at 05:30:50PM +0200, Michal Hocko wrote:
>>> I am wondering whether we can go further. E.g. it is not really clear to
>>> me whether we need an explicit FOLL_REMOTE when we can in fact check
>>> mm != current->mm and imply that. Maybe there are some contexts which
>>> wouldn't work, I haven't checked.
>>
>> This flag is set even when /proc/self/mem is used. I've not looked deeply into
>> this flag but perhaps accessing your own memory this way can be considered
>> 'remote' since you're not accessing it directly. On the other hand, perhaps this
>> is just mistaken in this case?
> 
> My understanding of the flag is quite limited as well. All I know it is
> related to protection keys and it is needed to bypass protection check.
> See arch_vma_access_permitted. See also 1b2ee1266ea6 ("mm/core: Do not
> enforce PKEY permissions on remote mm access").

Yeah, we need the flag to tell us when PKEYs should be applied or not.
The current task's PKRU (pkey rights register) should really only be
used to impact access to the task's memory, but has no bearing on how a
given task should access remote memory.

[toc] | [prev] | [next] | [standalone]

#1504144

On Wed 19-10-16 09:49:43, Dave Hansen wrote:
> On 10/19/2016 02:07 AM, Michal Hocko wrote:
> > On Wed 19-10-16 09:58:15, Lorenzo Stoakes wrote:
> >> On Tue, Oct 18, 2016 at 05:30:50PM +0200, Michal Hocko wrote:
> >>> I am wondering whether we can go further. E.g. it is not really clear to
> >>> me whether we need an explicit FOLL_REMOTE when we can in fact check
> >>> mm != current->mm and imply that. Maybe there are some contexts which
> >>> wouldn't work, I haven't checked.
> >>
> >> This flag is set even when /proc/self/mem is used. I've not looked deeply into
> >> this flag but perhaps accessing your own memory this way can be considered
> >> 'remote' since you're not accessing it directly. On the other hand, perhaps this
> >> is just mistaken in this case?
> > 
> > My understanding of the flag is quite limited as well. All I know it is
> > related to protection keys and it is needed to bypass protection check.
> > See arch_vma_access_permitted. See also 1b2ee1266ea6 ("mm/core: Do not
> > enforce PKEY permissions on remote mm access").
> 
> Yeah, we need the flag to tell us when PKEYs should be applied or not.
> The current task's PKRU (pkey rights register) should really only be
> used to impact access to the task's memory, but has no bearing on how a
> given task should access remote memory.

The question I had earlier was whether this has to be an explicit FOLL
flag used by g-u-p users or we can just use it internally when mm !=
current->mm

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1504157

On 10/19/2016 10:01 AM, Michal Hocko wrote:
> The question I had earlier was whether this has to be an explicit FOLL
> flag used by g-u-p users or we can just use it internally when mm !=
> current->mm

The reason I chose not to do that was that deferred work gets run under
a basically random 'current'.  If we just use 'mm != current->mm', then
the deferred work will sometimes have pkeys enforced and sometimes not,
basically randomly.

We want to be consistent with whether they are enforced or not, so we
explicitly indicate that by calling the remote variant vs. plain.

[toc] | [prev] | [next] | [standalone]

#1505202

On Wed 19-10-16 10:23:55, Dave Hansen wrote:
> On 10/19/2016 10:01 AM, Michal Hocko wrote:
> > The question I had earlier was whether this has to be an explicit FOLL
> > flag used by g-u-p users or we can just use it internally when mm !=
> > current->mm
> 
> The reason I chose not to do that was that deferred work gets run under
> a basically random 'current'.  If we just use 'mm != current->mm', then
> the deferred work will sometimes have pkeys enforced and sometimes not,
> basically randomly.

OK, I see (async_pf_execute and ksm ). It makes more sense to me. Thanks
for the clarification.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1504052

On Tue, Oct 18, 2016 at 05:30:50PM +0200, Michal Hocko wrote:
> I am wondering whether we can go further. E.g. it is not really clear to
> me whether we need an explicit FOLL_REMOTE when we can in fact check
> mm != current->mm and imply that. Maybe there are some contexts which
> wouldn't work, I haven't checked.

This flag is set even when /proc/self/mem is used. I've not looked deeply into
this flag but perhaps accessing your own memory this way can be considered
'remote' since you're not accessing it directly. On the other hand, perhaps this
is just mistaken in this case?

> I guess there is more work in that area and I do not want to impose all
> that work on you, but I couldn't resist once I saw you playing in that
> area ;) Definitely a good start!

Thanks, I am more than happy to go as far down this rabbit hole as is helpful,
no imposition at all :)

[toc] | [prev] | [next] | [standalone]

#1508961

On Tue, Oct 18, 2016 at 05:30:50PM +0200, Michal Hocko wrote:
>I am wondering whether we can go further. E.g. it is not really clear to
>me whether we need an explicit FOLL_REMOTE when we can in fact check
>mm != current->mm and imply that. Maybe there are some contexts which
>wouldn't work, I haven't checked.
>
>Then I am also wondering about FOLL_TOUCH behavior.
>__get_user_pages_unlocked has only few callers which used to be
>get_user_pages_unlocked before 1e9877902dc7e ("mm/gup: Introduce
>get_user_pages_remote()"). To me a dropped FOLL_TOUCH seems
>unintentional. Now that get_user_pages_unlocked has gup_flags argument I
>guess we might want to get rid of the __g-u-p-u version altogether, no?
>
>__get_user_pages is quite low level and imho shouldn't be exported. It's
>only user - kvm - should rather pull those two functions to gup instead
>and export them. There is nothing really KVM specific in them.

I believe I've attacked each of these, other than the use of explicit
FOLL_REMOTE which was explained by Dave.

> I also cannot say I would be entirely thrilled about get_user_pages_locked,
> we only have one user which can simply do lock g-u-p unlock AFAICS.

The principle difference here seems to be the availability of VM_FAULT_RETRY
behaviour (by passing a non-NULL locked argument), and indeed the comments in
gup.c recommends using get_user_pages_locked() if possible (though it seems not
to have been heeded too much :), so I'm not sure if this would be a fruitful
refactor, let me know!

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web