|
|
|
|
|
|
|
|
| Re: [PATCH] struct file leakage [message #4486 is a reply to message #4410] |
Tue, 11 July 2006 12:04   |
Trond Myklebust
Messages: 24
Registered: July 2006
|
Junior Member |
|
|
On Mon, 2006-07-10 at 03:05 -0700, Andrew Morton wrote:
> On Mon, 10 Jul 2006 13:05:35 +0400
> Kirill Korotaev <dev@sw.ru> wrote:
>
> > Hello!
> >
> > Andrew, this is a patch from Alexey Kuznetsov for 2.6.16.
> > I believe 2.6.17 still has this leak.
> >
> > ------------------------------------------------------------ -
> >
> > 2.6.16 leaks like hell. While testing, I found massive leakage
> > (reproduced in openvz) in:
> >
> > *filp
> > *size-4096
> >
> > And 1 object leaks in
> > *size-32
> > *size-64
> > *size-128
> >
> >
> > It is the fix for the first one. filp leaks in the bowels
> > of namei.c.
> >
> > Seems, size-4096 is file table leaking in expand_fdtables.
>
> I suspect that's been there for a long time.
>
> > I have no idea what are the rest and why they show only
> > accompaniing another leaks. Some debugging structs?
>
> I don't understand this. Are you implying that there are other bugs.
>
> > Signed-Off-By: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
> > CC: Kirill Korotaev <dev@openvz.org>
> >
>
> > --- linux-2.6.16-w/fs/namei.c 2006-07-10 11:43:11.000000000 +0400
> > +++ linux-2.6.16/fs/namei.c 2006-07-10 11:53:36.000000000 +0400
> > @@ -1774,8 +1774,15 @@ do_link:
> > if (error)
> > goto exit_dput;
> > error = __do_follow_link(&path, nd);
> > - if (error)
> > + if (error) {
> > + /* Does someone understand code flow here? Or it is only
> > + * me so stupid? Anathema to whoever designed this non-sense
> > + * with "intent.open".
> > + */
> > + if (!IS_ERR(nd->intent.open.file))
> > + release_open_intent(nd);
> > return error;
> > + }
> > nd->flags &= ~LOOKUP_PARENT;
> > if (nd->last_type == LAST_BIND)
> > goto ok;
> >
>
> It's good to have some more Alexeycomments in the tree.
>
> I wonder if we're also needing a path_release() here. And if not, whether
> it is still safe to run release_open_intent() against this nameidata?
>
> Hopefully Trond can recall what's going on in there...
The patch looks correct, except that I believe we can skip the IS_ERR()
test there: if we're following links then we presumably have not tried
to open any files yet, so the call to release_open_intent(nd) can be
made unconditional.
Cheers,
Trond
|
|
|
|
|
|
| Re: [PATCH] struct file leakage [message #4493 is a reply to message #4492] |
Wed, 12 July 2006 00:26 |
Trond Myklebust
Messages: 24
Registered: July 2006
|
Junior Member |
|
|
On Tue, 2006-07-11 at 16:32 -0700, Andrew Morton wrote:
> Trond Myklebust <trond.myklebust@fys.uio.no> wrote:
> >
> > > > - if (error)
> > > > + if (error) {
> > > > + /* Does someone understand code flow here? Or it is only
> > > > + * me so stupid? Anathema to whoever designed this non-sense
> > > > + * with "intent.open".
> > > > + */
> > > > + if (!IS_ERR(nd->intent.open.file))
> > > > + release_open_intent(nd);
> > > > return error;
> > > > + }
> > > > nd->flags &= ~LOOKUP_PARENT;
> > > > if (nd->last_type == LAST_BIND)
> > > > goto ok;
> > > >
> > >
> > > It's good to have some more Alexeycomments in the tree.
> > >
> > > I wonder if we're also needing a path_release() here. And if not, whether
> > > it is still safe to run release_open_intent() against this nameidata?
> > >
> > > Hopefully Trond can recall what's going on in there...
> >
> > The patch looks correct, except that I believe we can skip the IS_ERR()
> > test there: if we're following links then we presumably have not tried
> > to open any files yet, so the call to release_open_intent(nd) can be
> > made unconditional.
>
> Sorry, but phrases like "looks correct" and "I believe" don't inspire
> confidence. (Although what you say looks correct ;)) Are you sure?
We do need the call to release_open_intent(), since otherwise we will
leak a struct file. The question is whether we can optimise away the
IS_ERR() test. In my opinion, we can.
> And do we also need a path_release(nd) in there?
No. do_follow_link() should release the path for us on error. Replacing
with a 'goto exit' would therefore be a mistake.
Cheers,
Trond
|
|
|
|
OpenVZ Forum
Members
Search
Help
Register
Login
Home
