| Message ID | 7ed66bd5243f7535030e0fa6a8a94b76dc5033f1.1681508038.git.lstoakes@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | remove the vmas parameter from GUP APIs | expand |
On Sat, Apr 15, 2023 at 12:27:40AM +0100, Lorenzo Stoakes wrote: > This flag causes GUP to assert that all VMAs within the input range possess > the same vma->vm_file. If not, the operation fails. > > This is part of a patch series which eliminates the vmas parameter from the > GUP API, implementing the one remaining assertion within the entire kernel > that requires access to the VMAs associated with a GUP range. > > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com> > --- > include/linux/mm_types.h | 2 ++ > mm/gup.c | 16 ++++++++++++---- > 2 files changed, 14 insertions(+), 4 deletions(-) > > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h > index 3fc9e680f174..84d1aec9dbab 100644 > --- a/include/linux/mm_types.h > +++ b/include/linux/mm_types.h > @@ -1185,6 +1185,8 @@ enum { > FOLL_PCI_P2PDMA = 1 << 10, > /* allow interrupts from generic signals */ > FOLL_INTERRUPTIBLE = 1 << 11, > + /* assert that the range spans VMAs with the same vma->vm_file */ > + FOLL_SAME_FILE = 1 << 12, I hope we don't add this flag, but it needs to be rejected in internal_get_user_pages_fast() Jason
On Mon, Apr 17, 2023 at 10:14:02AM -0300, Jason Gunthorpe wrote: > On Sat, Apr 15, 2023 at 12:27:40AM +0100, Lorenzo Stoakes wrote: > > This flag causes GUP to assert that all VMAs within the input range possess > > the same vma->vm_file. If not, the operation fails. > > > > This is part of a patch series which eliminates the vmas parameter from the > > GUP API, implementing the one remaining assertion within the entire kernel > > that requires access to the VMAs associated with a GUP range. > > > > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com> > > --- > > include/linux/mm_types.h | 2 ++ > > mm/gup.c | 16 ++++++++++++---- > > 2 files changed, 14 insertions(+), 4 deletions(-) > > > > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h > > index 3fc9e680f174..84d1aec9dbab 100644 > > --- a/include/linux/mm_types.h > > +++ b/include/linux/mm_types.h > > @@ -1185,6 +1185,8 @@ enum { > > FOLL_PCI_P2PDMA = 1 << 10, > > /* allow interrupts from generic signals */ > > FOLL_INTERRUPTIBLE = 1 << 11, > > + /* assert that the range spans VMAs with the same vma->vm_file */ > > + FOLL_SAME_FILE = 1 << 12, > > I hope we don't add this flag, but it needs to be rejected in > internal_get_user_pages_fast() > intenal_get_user_pages_fast() checks against the complement of accepted masks, therefore it will reject this as-is unless I'm missing something. As for not adding the flag (an entirely understandable sentiment), it would be good to get an insight into the semantics of what you feel would be more suitable! > Jason
On Mon, Apr 17, 2023 at 02:25:54PM +0100, Lorenzo Stoakes wrote: > On Mon, Apr 17, 2023 at 10:14:02AM -0300, Jason Gunthorpe wrote: > > On Sat, Apr 15, 2023 at 12:27:40AM +0100, Lorenzo Stoakes wrote: > > > This flag causes GUP to assert that all VMAs within the input range possess > > > the same vma->vm_file. If not, the operation fails. > > > > > > This is part of a patch series which eliminates the vmas parameter from the > > > GUP API, implementing the one remaining assertion within the entire kernel > > > that requires access to the VMAs associated with a GUP range. > > > > > > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com> > > > --- > > > include/linux/mm_types.h | 2 ++ > > > mm/gup.c | 16 ++++++++++++---- > > > 2 files changed, 14 insertions(+), 4 deletions(-) > > > > > > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h > > > index 3fc9e680f174..84d1aec9dbab 100644 > > > --- a/include/linux/mm_types.h > > > +++ b/include/linux/mm_types.h > > > @@ -1185,6 +1185,8 @@ enum { > > > FOLL_PCI_P2PDMA = 1 << 10, > > > /* allow interrupts from generic signals */ > > > FOLL_INTERRUPTIBLE = 1 << 11, > > > + /* assert that the range spans VMAs with the same vma->vm_file */ > > > + FOLL_SAME_FILE = 1 << 12, > > > > I hope we don't add this flag, but it needs to be rejected in > > internal_get_user_pages_fast() > > > > intenal_get_user_pages_fast() checks against the complement of accepted > masks, therefore it will reject this as-is unless I'm missing > something. Hmm, yes, that looks OK > As for not adding the flag (an entirely understandable sentiment), it would > be good to get an insight into the semantics of what you feel would be more > suitable! I'm hoping there is not a solid justification for why io_uring is doing this check so we just delete it. Jason
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h index 3fc9e680f174..84d1aec9dbab 100644 --- a/include/linux/mm_types.h +++ b/include/linux/mm_types.h @@ -1185,6 +1185,8 @@ enum { FOLL_PCI_P2PDMA = 1 << 10, /* allow interrupts from generic signals */ FOLL_INTERRUPTIBLE = 1 << 11, + /* assert that the range spans VMAs with the same vma->vm_file */ + FOLL_SAME_FILE = 1 << 12, /* See also internal only FOLL flags in mm/internal.h */ }; diff --git a/mm/gup.c b/mm/gup.c index 9440aa54c741..3954ce499a4a 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -959,7 +959,8 @@ static int faultin_page(struct vm_area_struct *vma, return 0; } -static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags) +static int check_vma_flags(struct vm_area_struct *vma, struct file *file, + unsigned long gup_flags) { vm_flags_t vm_flags = vma->vm_flags; int write = (gup_flags & FOLL_WRITE); @@ -968,7 +969,7 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags) if (vm_flags & (VM_IO | VM_PFNMAP)) return -EFAULT; - if (gup_flags & FOLL_ANON && !vma_is_anonymous(vma)) + if ((gup_flags & FOLL_ANON) && !vma_is_anonymous(vma)) return -EFAULT; if ((gup_flags & FOLL_LONGTERM) && vma_is_fsdax(vma)) @@ -977,6 +978,9 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags) if (vma_is_secretmem(vma)) return -EFAULT; + if ((gup_flags & FOLL_SAME_FILE) && vma->vm_file != file) + return -EFAULT; + if (write) { if (!(vm_flags & VM_WRITE)) { if (!(gup_flags & FOLL_FORCE)) @@ -1081,6 +1085,7 @@ static long __get_user_pages(struct mm_struct *mm, long ret = 0, i = 0; struct vm_area_struct *vma = NULL; struct follow_page_context ctx = { NULL }; + struct file *file = NULL; if (!nr_pages) return 0; @@ -1111,10 +1116,13 @@ static long __get_user_pages(struct mm_struct *mm, ret = -EFAULT; goto out; } - ret = check_vma_flags(vma, gup_flags); + ret = check_vma_flags(vma, i == 0 ? vma->vm_file : file, + gup_flags); if (ret) goto out; + file = vma->vm_file; + if (is_vm_hugetlb_page(vma)) { i = follow_hugetlb_page(mm, vma, pages, vmas, &start, &nr_pages, i, @@ -1595,7 +1603,7 @@ long faultin_vma_page_range(struct vm_area_struct *vma, unsigned long start, * We want to report -EINVAL instead of -EFAULT for any permission * problems or incompatible mappings. */ - if (check_vma_flags(vma, gup_flags)) + if (check_vma_flags(vma, vma->vm_file, gup_flags)) return -EINVAL; ret = __get_user_pages(mm, start, nr_pages, gup_flags,
This flag causes GUP to assert that all VMAs within the input range possess the same vma->vm_file. If not, the operation fails. This is part of a patch series which eliminates the vmas parameter from the GUP API, implementing the one remaining assertion within the entire kernel that requires access to the VMAs associated with a GUP range. Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com> --- include/linux/mm_types.h | 2 ++ mm/gup.c | 16 ++++++++++++---- 2 files changed, 14 insertions(+), 4 deletions(-)