On Fri, Jun 03, 2016 at 10:46:31PM +0100, Al Viro wrote:
> On Fri, Jun 03, 2016 at 05:17:06PM -0400, Oleg Drokin wrote:
>
> > > Can the same thing be reproduced (with NFS fix) on v4.6, ede4090, 7f427d3,
> > > 4e8440b?
> >
> > Well, that was faster than I expected. 4e8440b triggers right away, so I
> > guess
> > there's no point in trying the later ones?
> > BTW, just to confirm you are noticing - this is a DEBUG_PAGEALLOC build,
> > so all freed memory is unmapped which is likely causing this oops - as a
> > sign
> > of use after free.
>
> > [ 54.990119] BUG: unable to handle kernel paging request at
> > ffff8800d2b7f000
>
> Again a page-aligned nd->last.name and even smaller nd->last.len. It smells
> like a page that used to contain a symlink body, but got freed under us.
OK, I think I understand what's going on there. We have a pathname that ends
with a trailing symlink. Traverse that symlink up to the last component. And
get EOPENSTALE on attempt to open that. At that point we proceed to
retry_lookup: and call lookup_open(). But we'd *already* done put_link()
on the first pass, so now nd->last.name points into freed page.
Damn... I'm very tempted to rip the retry_lookup logics out of there and
just let the damn thing repeat the whole pathname resolution ;-/ do_last()
will become so much saner after that...
Let's at least verify that this is what's going on - remove
if (error == -EOPENSTALE)
goto stale_open;
from do_last() and see if that fixes the damn thing. Alternative solution
would be to turn that
if (nd->depth)
put_link(nd);
error = should_follow_link(nd, &path, nd->flags & LOOKUP_FOLLOW,
inode, seq);
if (unlikely(error))
return error;
in do_last() into
error = should_follow_link(nd, &path, nd->flags & LOOKUP_FOLLOW,
inode, seq);
if (unlikely(error)) {
if (nd->depth == 2) {
struct saved *last = nd->stack[0];
do_delayed_call(&last->done);
if (!(nd->flags & LOOKUP_RCU))
path_put(&last->link);
nd->stack[0] = nd->stack[1];
nd->depth--;
}
return error;
}
but I would really prefer the first approach - it allows to remove arseloads
of convoluted crap from do_last().
