On Tue, Mar 07, 2023 at 04:01:56PM +0300, Alexey Izbyshev wrote:
> On 2023-01-19 19:03, Joey Gouly wrote:
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 87d929316d57..99a4d9e2b0d8 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -2665,6 +2665,16 @@ unsigned long mmap_region(struct file *file,
> > unsigned long addr,
> > vma_set_anonymous(vma);
> > }
> >
> > + if (map_deny_write_exec(vma, vma->vm_flags)) {
> > + error = -EACCES;
> > + if (file)
> > + goto close_and_free_vma;
> > + else if (vma->vm_file)
> > + goto unmap_and_free_vma;
> > + else
> > + goto free_vma;
> > + }
> > +
>
> Why is the cleanup dispatch logic duplicated here, instead of simply doing
> "goto close_and_free_vma" (where basically the same dispatch is done)?
Yes, though that's only possible after commit cc8d1b097de7 ("mmap: clean
up mmap_region() unrolling") in 6.3-rc1. It's worth adding a separate
patch to simplify this before final 6.3.
> > diff --git a/mm/mprotect.c b/mm/mprotect.c
> > index 908df12caa26..bc0587df042f 100644
> > --- a/mm/mprotect.c
> > +++ b/mm/mprotect.c
> > @@ -762,6 +762,11 @@ static int do_mprotect_pkey(unsigned long start,
> > size_t len,
> > break;
> > }
> >
> > + if (map_deny_write_exec(vma, newflags)) {
> > + error = -EACCES;
> > + goto out;
> > + }
> > +
>
> Why does this check use "goto out", thereby skipping post-loop cleanup,
> instead of "break" like all other checks? This looks like a bug to me.
Ah, good point, thanks. I think that's a left-over from my early attempt
at this series. The loop was changed in 5.19 with commit 4a18419f71cd
("mm/mprotect: use mmu_gather") but the patch not updated.
So yeah, it needs fixing. Joey, could you please send fixes for both
issues above?
Thanks.
--
Catalin
