Re: [PATCH] mm: Update do_vmi_align_munmap() return semantics

[Linus Torvalds]

On Fri, 30 Jun 2023 at 09:06, Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> wrote:
>
> Update do_vmi_align_munmap() to return 0 for success.  Clean up the
> callers and comments to always expect the lock downgrade to be honored
> on the success path.  The error path will always leave the lock
> untouched.

Thanks for doing this, but with this cleanup, it becomes clear that
some of the callers that asked for a downgrade didn't actually want
that at all...

For example:

> +               if (do_vma_munmap(&vmi, brkvma, newbrk, oldbrk, &uf, true))
> +                       goto out;
> +
> +               mmap_read_unlock(mm);
> +               goto success_unlocked;

this clearly wanted the lock to be dropped entirely.

As did this one:

>         ret = do_vmi_munmap(&vmi, mm, start, len, &uf, downgrade);
>         /*
> -        * Returning 1 indicates mmap_lock is downgraded.
> -        * But 1 is not legal return value of vm_munmap() and munmap(), reset
> -        * it to 0 before return.
> +        * Returning 0 is successful, but the lock status depends what was
> +        * passed in.
>          */
> -       if (ret == 1) {
> +       if (!ret && downgrade)
>                 mmap_read_unlock(mm);
> -               ret = 0;
> -       } else
> +       else
>                 mmap_write_unlock(mm);

And this one:

> +               ret = do_vmi_munmap(&vmi, mm, addr + new_len, old_len - new_len,
> +                                   &uf_unmap, true);
> +               if (ret)
> +                       goto out;
> +
> +               mmap_read_unlock(current->mm);

I didn't look at what all the indirect callers here were doing, but it
really looked to me like *most* callers wanted the lock dropped
entirely at the end.

In fact, looking at that patch, it looks like *all* of the callers
that asked for downgrading actually really wanted the lock dropped
entirely.

But I may well be missing some context.  So take this not as a NAK,
but as a "you looked at all this code, could it perhaps be simplified
a bit more still?"

               Linus