| Message ID | 20200421071026.18394-1-mhocko@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm, mempolicy: fix up gup usage in lookup_node | expand |
On Tue, Apr 21, 2020 at 09:10:26AM +0200, Michal Hocko wrote: > From: Michal Hocko <mhocko@suse.com> > > ba841078cd05 ("mm/mempolicy: Allow lookup_node() to handle fatal signal") has > added a special casing for 0 return value because that was a possible > gup return value when interrupted by fatal signal. This has been fixed > by ae46d2aa6a7f ("mm/gup: Let __get_user_pages_locked() return -EINTR > for fatal signal") in the mean time so ba841078cd05 can be reverted. > > This patch however doesn't go all the way to revert it because the check > for 0 is wrong and confusing here. Firstly it is inherently unsafe to > access the page when get_user_pages_locked returns 0 (aka no page > returned). > Fortunatelly this will not happen because get_user_pages_locked will not > return 0 when nr_pages > 0 unless FOLL_NOWAIT is specified which is not > the case here. Document this potential error code in gup code while we > are at it. > > Signed-off-by: Michal Hocko <mhocko@suse.com> > --- > mm/gup.c | 5 +++++ > mm/mempolicy.c | 5 +---- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/mm/gup.c b/mm/gup.c > index 50681f0286de..a8575b880baf 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -980,6 +980,7 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags) > * -- If nr_pages is >0, but no pages were pinned, returns -errno. > * -- If nr_pages is >0, and some pages were pinned, returns the number of > * pages pinned. Again, this may be less than nr_pages. > + * -- 0 return value is possible when the fault would need to be retried. > * > * The caller is responsible for releasing returned @pages, via put_page(). > * > @@ -1247,6 +1248,10 @@ int fixup_user_fault(struct task_struct *tsk, struct mm_struct *mm, > } > EXPORT_SYMBOL_GPL(fixup_user_fault); > > +/* > + * Please note that this function, unlike __get_user_pages will not > + * return 0 for nr_pages > 0 without FOLL_NOWAIT It's a bit unclear to me on whether "will not return 0" applies to "this function" or "__get_user_pages"... Might be easier just to avoid mentioning __get_user_pages? > + */ > static __always_inline long __get_user_pages_locked(struct task_struct *tsk, > struct mm_struct *mm, > unsigned long start, > diff --git a/mm/mempolicy.c b/mm/mempolicy.c > index 48ba9729062e..1965e2681877 100644 > --- a/mm/mempolicy.c > +++ b/mm/mempolicy.c > @@ -927,10 +927,7 @@ static int lookup_node(struct mm_struct *mm, unsigned long addr) > > int locked = 1; > err = get_user_pages_locked(addr & PAGE_MASK, 1, 0, &p, &locked); > - if (err == 0) { > - /* E.g. GUP interrupted by fatal signal */ > - err = -EFAULT; > - } else if (err > 0) { > + if (err > 0) { > err = page_to_nid(p); > put_page(p); > } Again, this is my totally humble opinion: I'm fine with removing the comment, however I still don't think it's helpful at all to explicitly remove a check against invalid return value (err==0), especially if that's the only functional change in this patch. Thanks,
On Tue 21-04-20 09:29:16, Peter Xu wrote: > On Tue, Apr 21, 2020 at 09:10:26AM +0200, Michal Hocko wrote: > > From: Michal Hocko <mhocko@suse.com> > > > > ba841078cd05 ("mm/mempolicy: Allow lookup_node() to handle fatal signal") has > > added a special casing for 0 return value because that was a possible > > gup return value when interrupted by fatal signal. This has been fixed > > by ae46d2aa6a7f ("mm/gup: Let __get_user_pages_locked() return -EINTR > > for fatal signal") in the mean time so ba841078cd05 can be reverted. > > > > This patch however doesn't go all the way to revert it because the check > > for 0 is wrong and confusing here. Firstly it is inherently unsafe to > > access the page when get_user_pages_locked returns 0 (aka no page > > returned). > > Fortunatelly this will not happen because get_user_pages_locked will not > > return 0 when nr_pages > 0 unless FOLL_NOWAIT is specified which is not > > the case here. Document this potential error code in gup code while we > > are at it. > > > > Signed-off-by: Michal Hocko <mhocko@suse.com> > > --- > > mm/gup.c | 5 +++++ > > mm/mempolicy.c | 5 +---- > > 2 files changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/mm/gup.c b/mm/gup.c > > index 50681f0286de..a8575b880baf 100644 > > --- a/mm/gup.c > > +++ b/mm/gup.c > > @@ -980,6 +980,7 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags) > > * -- If nr_pages is >0, but no pages were pinned, returns -errno. > > * -- If nr_pages is >0, and some pages were pinned, returns the number of > > * pages pinned. Again, this may be less than nr_pages. > > + * -- 0 return value is possible when the fault would need to be retried. > > * > > * The caller is responsible for releasing returned @pages, via put_page(). > > * > > @@ -1247,6 +1248,10 @@ int fixup_user_fault(struct task_struct *tsk, struct mm_struct *mm, > > } > > EXPORT_SYMBOL_GPL(fixup_user_fault); > > > > +/* > > + * Please note that this function, unlike __get_user_pages will not > > + * return 0 for nr_pages > 0 without FOLL_NOWAIT > > It's a bit unclear to me on whether "will not return 0" applies to "this > function" or "__get_user_pages"... Might be easier just to avoid mentioning > __get_user_pages? I really wanted to call out __get_user_pages because the semantic of 0 return value is different. If you have a suggestion how to reformulate this to be more clear then I will incorporate that. > > + */ > > static __always_inline long __get_user_pages_locked(struct task_struct *tsk, > > struct mm_struct *mm, > > unsigned long start, > > diff --git a/mm/mempolicy.c b/mm/mempolicy.c > > index 48ba9729062e..1965e2681877 100644 > > --- a/mm/mempolicy.c > > +++ b/mm/mempolicy.c > > @@ -927,10 +927,7 @@ static int lookup_node(struct mm_struct *mm, unsigned long addr) > > > > int locked = 1; > > err = get_user_pages_locked(addr & PAGE_MASK, 1, 0, &p, &locked); > > - if (err == 0) { > > - /* E.g. GUP interrupted by fatal signal */ > > - err = -EFAULT; > > - } else if (err > 0) { > > + if (err > 0) { > > err = page_to_nid(p); > > put_page(p); > > } > > Again, this is my totally humble opinion: I'm fine with removing the comment, > however I still don't think it's helpful at all to explicitly remove a check > against invalid return value (err==0), especially if that's the only functional > change in this patch. I thought I have explained that when we have discussed last time and the changelog is explaining that as well. Checking for impossible error code is simply confusing and provokes for copy&pasting this pattern. I wouldn't really bother if I haven't seen this cargo cult pattern in the so many times.
On Tue, Apr 21, 2020 at 04:46:03PM +0200, Michal Hocko wrote: > I thought I have explained that when we have discussed last time and the > changelog is explaining that as well. Checking for impossible error code > is simply confusing and provokes for copy&pasting this pattern. I > wouldn't really bother if I haven't seen this cargo cult pattern in the > so many times. It's just my poor habit to avoid churns like this. Say, if the check is not there, I definitely shouldn't add that check without explicit reason. However if it's there already (and it's not an extremely hot path so no number to show that it will bring any performance impact), then I won't touch it either without a good reasoning. "Somebody could copy & paste the same code" isn't a reason to me - that's something we can observe when reviewing a patch. I've broken some code due to some tiny trivial small changes that I thought won't hurt, and I've also been debugging for hours due to some "should be trivial" patches from others. This is how the habit comes... But it's not a strong opinion either. I'd be fine if the patch is liked by others and Andrew would like to queue it. Thanks,
diff --git a/mm/gup.c b/mm/gup.c index 50681f0286de..a8575b880baf 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -980,6 +980,7 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags) * -- If nr_pages is >0, but no pages were pinned, returns -errno. * -- If nr_pages is >0, and some pages were pinned, returns the number of * pages pinned. Again, this may be less than nr_pages. + * -- 0 return value is possible when the fault would need to be retried. * * The caller is responsible for releasing returned @pages, via put_page(). * @@ -1247,6 +1248,10 @@ int fixup_user_fault(struct task_struct *tsk, struct mm_struct *mm, } EXPORT_SYMBOL_GPL(fixup_user_fault); +/* + * Please note that this function, unlike __get_user_pages will not + * return 0 for nr_pages > 0 without FOLL_NOWAIT + */ static __always_inline long __get_user_pages_locked(struct task_struct *tsk, struct mm_struct *mm, unsigned long start, diff --git a/mm/mempolicy.c b/mm/mempolicy.c index 48ba9729062e..1965e2681877 100644 --- a/mm/mempolicy.c +++ b/mm/mempolicy.c @@ -927,10 +927,7 @@ static int lookup_node(struct mm_struct *mm, unsigned long addr) int locked = 1; err = get_user_pages_locked(addr & PAGE_MASK, 1, 0, &p, &locked); - if (err == 0) { - /* E.g. GUP interrupted by fatal signal */ - err = -EFAULT; - } else if (err > 0) { + if (err > 0) { err = page_to_nid(p); put_page(p); }