Re: [PATCH 116/150] vfs,proc: guarantee unique inodes in /proc

[Luis Henriques]

On Wed, Mar 27, 2013 at 05:40:39AM +0000, Ben Hutchings wrote:
> On Tue, 2013-03-26 at 15:20 +0000, Luis Henriques wrote:
> > 3.5.7.9 -stable review patch.  If anyone has any objections, please let me know.
> > 
> > ------------------
> > 
> > From: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> > 
> > commit 51f0885e5415b4cc6535e9cdcc5145bfbc134353 upstream.
> > 
> > Dave Jones found another /proc issue with his Trinity tool: thanks to
> > the namespace model, we can have multiple /proc dentries that point to
> > the same inode, aliasing directories in /proc/<pid>/net/ for example.
> > 
> > This ends up being a total disaster, because it acts like hardlinked
> > directories, and causes locking problems.  We rely on the topological
> > sort of the inodes pointed to by dentries, and if we have aliased
> > directories, that odering becomes unreliable.
> > 
> > In short: don't do this.  Multiple dentries with the same (directory)
> > inode is just a bad idea, and the namespace code should never have
> > exposed things this way.  But we're kind of stuck with it.
> > 
> > This solves things by just always allocating a new inode during /proc
> > dentry lookup, instead of using "iget_locked()" to look up existing
> > inodes by superblock and number.  That actually simplies the code a bit,
> > at the cost of potentially doing more inode [de]allocations.
> > 
> > That said, the inode lookup wasn't free either (and did a lot of locking
> > of inodes), so it is probably not that noticeable.  We could easily keep
> > the old lookup model for non-directory entries, but rather than try to
> > be excessively clever this just implements the minimal and simplest
> > workaround for the problem.
> > 
> > Reported-and-tested-by: Dave Jones <davej@xxxxxxxxxx>
> > Analyzed-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> > [ luis: backported to 3.5; adjust context ]
> 
> Prior to commit d3d009cb965eae7e002ea5badf603ea8f4c34915, callers of
> proc_get_inode() don't expect it to call pde_put() before returning NULL
> - only when returning an existing inode, which it will never do after
> this.  So I think you must either cherry-pick that first, or delete
> 'else pde_put(de);' as part of this fix.

Nice catch, thanks Ben.  I will use your backport to 3.2 instead, which
fixes this issue.

Cheers,
--
Luis