linux-kernel - Re: [PATCH] mm: Update do_vmi_align

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHk-=wg=DGSsA+=rr3bMDKrGNgy4C+PGM_w2PtpK4+Sx9qFF8w@mail.gmail.com>
Date:   Fri, 30 Jun 2023 09:19:29 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     "Liam R. Howlett" <Liam.Howlett@...cle.com>
Cc:     Matthew Wilcox <willy@...radead.org>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] mm: Update do_vmi_align_munmap() return semantics

On Fri, 30 Jun 2023 at 09:06, Liam R. Howlett <Liam.Howlett@...cle.com> 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