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
