by Tetsuo Handa

[permalink] [raw]

Subject: Re: [PATCH v2 3/7] mm/gup: remove vmas parameter from get_user_pages_remote()

On 2023/04/15 19:14, Lorenzo Stoakes wrote:
> On Sat, Apr 15, 2023 at 06:52:41PM +0900, Tetsuo Handa wrote:
>> On 2023/04/15 18:08, Lorenzo Stoakes wrote:
>>> @@ -475,10 +474,14 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm,
>>> gup_flags |= FOLL_SPLIT_PMD;
>>> /* Read the page with vaddr into memory */
>>> ret = get_user_pages_remote(mm, vaddr, 1, gup_flags,
>>> - &old_page, &vma, NULL);
>>> + &old_page, NULL);
>>> if (ret <= 0)
>>> return ret;
>>>
>>> + vma = vma_lookup(mm, vaddr);
>>> + if (!vma)
>>> + goto put_old;
>>> +
>>> ret = verify_opcode(old_page, vaddr, &opcode);
>>> if (ret <= 0)
>>> goto put_old;
>>
>> This conversion looks wrong.
>> This causes returning a positive number when vma_lookup() returned NULL.
>>
>> * Return 0 (success) or a negative errno.
>>
>
> In reality it shouldn't be possible for vma to return NULL, I'm adding the
> checks to be extra careful.
>
> In any case you're right, attaching a -fix patch to avoid spam:-

If you want to return -EINVAL when vma_lookup() returned NULL for whatever
unexpected reason, returning -EOPNOTSUPP in below path looks strange.

> @@ -448,7 +448,8 @@ static int __access_remote_tags(struct mm_struct *mm, unsigned long addr,
> * would cause the existing tags to be cleared if the page
> * was never mapped with PROT_MTE.
> */
> - if (!(vma->vm_flags & VM_MTE)) {
> + vma = vma_lookup(mm, addr);
> + if (!vma || !(vma->vm_flags & VM_MTE)) {
> ret = -EOPNOTSUPP;
> put_page(page);
> break;

Also,

> @@ -5591,7 +5591,9 @@ int __access_remote_vm(struct mm_struct *mm, unsigned long addr, void *buf,
> struct page *page = NULL;
>
> ret = get_user_pages_remote(mm, addr, 1,
> - gup_flags, &page, &vma, NULL);
> + gup_flags, &page, NULL);
> + vma = vma_lookup(mm, addr);
> +
> if (ret <= 0) {
> #ifndef CONFIG_HAVE_IOREMAP_PROT
> break;
> @@ -5600,7 +5602,6 @@ int __access_remote_vm(struct mm_struct *mm, unsigned long addr, void *buf,
> * Check if this is a VM_IO | VM_PFNMAP VMA, which
> * we can access using slightly different code.
> */
> - vma = vma_lookup(mm, addr);
> if (!vma)
> break;
> if (vma->vm_ops && vma->vm_ops->access)
> @@ -5617,11 +5618,11 @@ int __access_remote_vm(struct mm_struct *mm, unsigned long addr, void *buf,
> bytes = PAGE_SIZE-offset;
>
> maddr = kmap(page);
> - if (write) {
> + if (write && vma) {
> copy_to_user_page(vma, page, addr,
> maddr + offset, buf, bytes);
> set_page_dirty_lock(page);
> - } else {
> + } else if (vma) {
> copy_from_user_page(vma, page, addr,
> buf, maddr + offset, bytes);
> }

not calling copy_{from,to}_user_page() if vma == NULL is not sufficient for
propagating an error to caller.

2023-04-15 11:40:11

by Lorenzo Stoakes

[permalink] [raw]

Subject: Re: [PATCH v2 3/7] mm/gup: remove vmas parameter from get_user_pages_remote()

On Sat, Apr 15, 2023 at 07:36:06PM +0900, Tetsuo Handa wrote:
> On 2023/04/15 19:14, Lorenzo Stoakes wrote:
> > On Sat, Apr 15, 2023 at 06:52:41PM +0900, Tetsuo Handa wrote:
> >> On 2023/04/15 18:08, Lorenzo Stoakes wrote:
> >>> @@ -475,10 +474,14 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm,
> >>> gup_flags |= FOLL_SPLIT_PMD;
> >>> /* Read the page with vaddr into memory */
> >>> ret = get_user_pages_remote(mm, vaddr, 1, gup_flags,
> >>> - &old_page, &vma, NULL);
> >>> + &old_page, NULL);
> >>> if (ret <= 0)
> >>> return ret;
> >>>
> >>> + vma = vma_lookup(mm, vaddr);
> >>> + if (!vma)
> >>> + goto put_old;
> >>> +
> >>> ret = verify_opcode(old_page, vaddr, &opcode);
> >>> if (ret <= 0)
> >>> goto put_old;
> >>
> >> This conversion looks wrong.
> >> This causes returning a positive number when vma_lookup() returned NULL.
> >>
> >> * Return 0 (success) or a negative errno.
> >>
> >
> > In reality it shouldn't be possible for vma to return NULL, I'm adding the
> > checks to be extra careful.
> >
> > In any case you're right, attaching a -fix patch to avoid spam:-
>
> If you want to return -EINVAL when vma_lookup() returned NULL for whatever
> unexpected reason, returning -EOPNOTSUPP in below path looks strange.
>

This feels a little pedantic, this is not a condition that is expected to
occur in practice, I'm not sure users will be writing code to differentiate
between the two, and certainly vma being NULL implies MTE is not supported.

To differentiate with minimal churn, I'll add a WARN_ON_ONCE() here and in
each other case where an impossible condition arises as it would be
indicative of a kernel bug.

> > @@ -448,7 +448,8 @@ static int __access_remote_tags(struct mm_struct *mm, unsigned long addr,
> > * would cause the existing tags to be cleared if the page
> > * was never mapped with PROT_MTE.
> > */
> > - if (!(vma->vm_flags & VM_MTE)) {
> > + vma = vma_lookup(mm, addr);
> > + if (!vma || !(vma->vm_flags & VM_MTE)) {
> > ret = -EOPNOTSUPP;
> > put_page(page);
> > break;
>
> Also,
>
> > @@ -5591,7 +5591,9 @@ int __access_remote_vm(struct mm_struct *mm, unsigned long addr, void *buf,
> > struct page *page = NULL;
> >
> > ret = get_user_pages_remote(mm, addr, 1,
> > - gup_flags, &page, &vma, NULL);
> > + gup_flags, &page, NULL);
> > + vma = vma_lookup(mm, addr);
> > +
> > if (ret <= 0) {
> > #ifndef CONFIG_HAVE_IOREMAP_PROT
> > break;
> > @@ -5600,7 +5602,6 @@ int __access_remote_vm(struct mm_struct *mm, unsigned long addr, void *buf,
> > * Check if this is a VM_IO | VM_PFNMAP VMA, which
> > * we can access using slightly different code.
> > */
> > - vma = vma_lookup(mm, addr);
> > if (!vma)
> > break;
> > if (vma->vm_ops && vma->vm_ops->access)
> > @@ -5617,11 +5618,11 @@ int __access_remote_vm(struct mm_struct *mm, unsigned long addr, void *buf,
> > bytes = PAGE_SIZE-offset;
> >
> > maddr = kmap(page);
> > - if (write) {
> > + if (write && vma) {
> > copy_to_user_page(vma, page, addr,
> > maddr + offset, buf, bytes);
> > set_page_dirty_lock(page);
> > - } else {
> > + } else if (vma) {
> > copy_from_user_page(vma, page, addr,
> > buf, maddr + offset, bytes);
> > }
>
> not calling copy_{from,to}_user_page() if vma == NULL is not sufficient for
> propagating an error to caller.
>

This is a product of wanting to avoid churn, again this condition is simply
impossible. Also as a pedantic side note - the loop explicitly indicates no
errors are propagated, so there is no need to do so.

However, I want to be consistent with how I handle this and also I think
it's sensible to add warnings for violation of this 'impossible' condition
so I'll add a branch for it.

Since I'd end up confusingly fixing up a fix-patch (and I want to change
another patch in series to be consistent), I'll do a respin, apologies for
spam in advance...