Messages in this thread | | | Date | Thu, 18 Jun 2015 18:08:29 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH 0/3] aio: ctx->dead cleanups |
| |
On 06/17, Oleg Nesterov wrote: > > On 06/17, Al Viro wrote: > > > > On Wed, Jun 17, 2015 at 01:39:06AM +0100, Al Viro wrote: > > > > And yes, the leak you've spotted is real, but I would very much prefer > > to avoid that goto - something like this instead: > > > > diff --git a/mm/mremap.c b/mm/mremap.c > > index 034e2d3..b36b530 100644 > > --- a/mm/mremap.c > > +++ b/mm/mremap.c > > @@ -291,7 +291,10 @@ static unsigned long move_vma(struct vm_area_struct *vma, > > if (err < 0) { > > move_page_tables(new_vma, new_addr, vma, old_addr, > > moved_len, true); > > - return err; > > + vma = new_vma; > > + old_len = new_len; > > + old_addr = new_addr; > > + new_addr = err; > > Personally, I'd really prefer to factor out at least this > move_page_tables() with six args. Although I agree, "goto previous_if" > doesn't look nice too, this needs cleanup. > > But this is minor. I am already sleeping, most probably I misread > this code. But it seems that there is another bug with VM_ACCOUNT. > > I'll recheck tomorrow and write another email.
Yes this look wrong. At least we shouldn't set *locked on failure. mm->locked_vm += new_len is probably fine, but doesn't look really nice because ->locked_vm can underflow in do_munmap() before that.
I'll send the fixes, but also I'll try to cleanup this code. Not sure I will succeed ;)
Oleg.
| |