[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20110907224234.GD28162@sun>
Date: Thu, 8 Sep 2011 02:42:34 +0400
From: Cyrill Gorcunov <gorcunov@...il.com>
To: Andrew Morton <akpm@...ux-foundation.org>
Cc: Vasiliy Kulikov <segoon@...nwall.com>, Tejun Heo <tj@...nel.org>,
"Kirill A. Shutemov" <kirill@...temov.name>,
containers@...ts.osdl.org, linux-kernel@...r.kernel.org,
linux-fsdevel@...r.kernel.org, Nathan Lynch <ntl@...ox.com>,
kernel-hardening@...ts.openwall.com,
Oren Laadan <orenl@...columbia.edu>,
Daniel Lezcano <dlezcano@...ibm.com>,
Glauber Costa <glommer@...allels.com>,
James Bottomley <jbottomley@...allels.com>,
Alexey Dobriyan <adobriyan@...il.com>,
Al Viro <viro@...IV.linux.org.uk>,
Pavel Emelyanov <xemul@...allels.com>
Subject: Re: [patch 2/2] fs, proc: Introduce the /proc/<pid>/map_files/
directory v6
On Wed, Sep 07, 2011 at 03:13:23PM -0700, Andrew Morton wrote:
...
> > >
> > > Andrew, are you OK with closing the hole with pid_no_revalidate()
> > > and 0600 /proc/slabinfo? If so, I feel I have to start this discussion
> > > with people participating in the discussion above: Theodore, Dan, Linus, etc.
>
> I fell asleep a long time ago and don't know what pid_no_revalidate()
> and slabinfo permissions have to do with this. Perhaps summarising the
> issues in the changelog would be appropriate, dunno.
Well, time to poke Vasiliy ;)
...
> >
> > I fear we still need to use two passes in proc_map_files_readdir, I found no way
> > to escape lockdep complains when doing all work in one pass with mmap_sem taken.
> > The /maps does the same thing -- ie it fills maps file with mmap_sem taken to produce
> > robust data.
>
> The code's using three passes.
Yes, and I didn't find thy way to escape it (actually if there would not
be filldir+might_fault tuple I would create this all under mmap_sem and
would not need this flex_array or any temporary storage at all and code
would be a way simplier).
>
> > And I'm not really sure what you mean with problematic put_filp?
>
> I was thinking fput(), which can do a hell of a lot of stuff if it's
> the final put on the inode.
Ouch, somehow missed it, thanks!
> > +err:
> > + up_read(&mm->mmap_sem);
> > +
> > + for (i = 0; i < used && !ret; i++) {
>
> The "&& !ret" is unneeded?
No, it's needed, since it makes sure that if "impossible"
scenario happens and flex-arrays fails with preallocated
data so we will reach this point with used > 0 and ret = -ENOMEM
and thus will not call for proc_map_files_instantiate as needed.
>
> > + p = flex_array_get(fa, i);
> > + ret = proc_fill_cache(filp, dirent, filldir,
> > + p->name, p->len,
> > + proc_map_files_instantiate,
> > + task, p->file);
> > + if (ret)
> > + break;
1: Say we failed here
> > + filp->f_pos++;
> > + put_filp(p->file);
> > + }
> > +
> > + for (; i < used; i++) {
> > + p = flex_array_get(fa, i);
> > + put_filp(p->file);
> > + }
>
> Still unclear why we need the third loop.
Due to (1) -- so we will have a number of files reference
taken and need to put them back.
Cyrill
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
