| Message ID | 20250407030306.411977-3-bhe@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/gup: Minor fix, cleanup and improvements | expand |
On Mon, Apr 07, 2025 at 11:03:05AM +0800, Baoquan He wrote: > In __get_user_pages(), it will traverse page table and take a reference > to the page the given user address corresponds to if GUP_GET or GUP_PIN > is set. However, it's not supported both GUP_GET and GUP_PIN are set. > Even though this check need be done, it should be done earlier, but not > doing it till entering into follow_page_pte() and failed. > > Furthermore, this checking has been done in is_valid_gup_args() and all > external users of __get_user_pages() will call is_valid_gup_args() to > catch the illegal setting. We don't need to worry about internal users > of __get_user_pages() because the gup_flags are set by MM code correctly. > > Here remove the checking in follow_page_pte(), and add VM_WARN_ON_ONCE() > to catch the possible exceptional setting just in case. > > And also change the VM_BUG_ON to VM_WARN_ON_ONCE() for checking > (!!pages != !!(gup_flags & (FOLL_GET | FOLL_PIN))) because the checking > has been done in is_valid_gup_args() for external users of __get_user_pages(). I will also note that you changed the flags check to VM_WARN_ON_ONCE. I guess this is fine as we have the WARN_ON_ONCE version in is_valid_gup_args(). > Signed-off-by: Baoquan He <bhe@redhat.com> LGTM, Reviewed-by: Oscar Salvador <osalvador@suse.de>
On 08.04.25 11:04, Oscar Salvador wrote: > On Mon, Apr 07, 2025 at 11:03:05AM +0800, Baoquan He wrote: >> In __get_user_pages(), it will traverse page table and take a reference >> to the page the given user address corresponds to if GUP_GET or GUP_PIN >> is set. However, it's not supported both GUP_GET and GUP_PIN are set. >> Even though this check need be done, it should be done earlier, but not >> doing it till entering into follow_page_pte() and failed. >> >> Furthermore, this checking has been done in is_valid_gup_args() and all >> external users of __get_user_pages() will call is_valid_gup_args() to >> catch the illegal setting. We don't need to worry about internal users >> of __get_user_pages() because the gup_flags are set by MM code correctly. >> >> Here remove the checking in follow_page_pte(), and add VM_WARN_ON_ONCE() >> to catch the possible exceptional setting just in case. >> >> And also change the VM_BUG_ON to VM_WARN_ON_ONCE() for checking >> (!!pages != !!(gup_flags & (FOLL_GET | FOLL_PIN))) because the checking >> has been done in is_valid_gup_args() for external users of __get_user_pages(). > > I will also note that you changed the flags check to VM_WARN_ON_ONCE. > I guess this is fine as we have the WARN_ON_ONCE version in > is_valid_gup_args(). > Yes, that was my reasoning when I proposed it. >> Signed-off-by: Baoquan He <bhe@redhat.com> > > LGTM, > > Reviewed-by: Oscar Salvador <osalvador@suse.de> > > Acked-by: David Hildenbrand <david@redhat.com>
diff --git a/mm/gup.c b/mm/gup.c index 67a7de9e4f80..5b3ac5a867a3 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -844,11 +844,6 @@ static struct page *follow_page_pte(struct vm_area_struct *vma, pte_t *ptep, pte; int ret; - /* FOLL_GET and FOLL_PIN are mutually exclusive. */ - if (WARN_ON_ONCE((flags & (FOLL_PIN | FOLL_GET)) == - (FOLL_PIN | FOLL_GET))) - return ERR_PTR(-EINVAL); - ptep = pte_offset_map_lock(mm, pmd, address, &ptl); if (!ptep) return no_page_table(vma, flags, address); @@ -1432,7 +1427,11 @@ static long __get_user_pages(struct mm_struct *mm, start = untagged_addr_remote(mm, start); - VM_BUG_ON(!!pages != !!(gup_flags & (FOLL_GET | FOLL_PIN))); + VM_WARN_ON_ONCE(!!pages != !!(gup_flags & (FOLL_GET | FOLL_PIN))); + + /* FOLL_GET and FOLL_PIN are mutually exclusive. */ + VM_WARN_ON_ONCE((gup_flags & (FOLL_PIN | FOLL_GET)) == + (FOLL_PIN | FOLL_GET)); do { struct page *page;
In __get_user_pages(), it will traverse page table and take a reference to the page the given user address corresponds to if GUP_GET or GUP_PIN is set. However, it's not supported both GUP_GET and GUP_PIN are set. Even though this check need be done, it should be done earlier, but not doing it till entering into follow_page_pte() and failed. Furthermore, this checking has been done in is_valid_gup_args() and all external users of __get_user_pages() will call is_valid_gup_args() to catch the illegal setting. We don't need to worry about internal users of __get_user_pages() because the gup_flags are set by MM code correctly. Here remove the checking in follow_page_pte(), and add VM_WARN_ON_ONCE() to catch the possible exceptional setting just in case. And also change the VM_BUG_ON to VM_WARN_ON_ONCE() for checking (!!pages != !!(gup_flags & (FOLL_GET | FOLL_PIN))) because the checking has been done in is_valid_gup_args() for external users of __get_user_pages(). Signed-off-by: Baoquan He <bhe@redhat.com> --- mm/gup.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)