linux-kernel - Re: [PATCH RFC] proc: Don't retain negative dentries

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 8 Oct 2018 19:01:15 +0100
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Ahmad Fatoum <a.fatoum@...gutronix.de>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Alexey Dobriyan <adobriyan@...il.com>,
        David Howells <dhowells@...hat.com>,
        Florian Westphal <fw@...len.de>, linux-kernel@...r.kernel.org,
        kernel@...gutronix.de
Subject: Re: [PATCH RFC] proc: Don't retain negative dentries

On Mon, Oct 08, 2018 at 07:02:09PM +0200, Ahmad Fatoum wrote:
> Hello,
> 
> On 10/8/18 6:55 PM, Al Viro wrote:
> > 
> > What the hell does that have to do with negative dentries anywhere???
> 
> It's possible that this needs fixing at another place. I don't know,
> but this seems to work for me, that's why I prefixed with RFC.

OK, to elaborate: where have you seen negative dentries on procfs in
the first place?  I'm trying to find a way for such to happen, but
I don't see any.  And in any case, these ->d_delete() and ->d_revalidate()
instances would've been oopsing on such.

->d_delete() is about retaining _unused_ dentries in hash for future lookups;
nothing to do with positive/negative.  *And* ->d_delete() is called only when
refcount hits zero.  If another process opens the damn thing and keeps it opened,
->d_delete() won't be called at all and your patch won't change the behaviour
of the entire thing.

If anything, you might want to have separate ->d_op for /proc/*/net, so
that its ->d_revalidate() would return 0 if netns doesn't match.  Would
need a way to keep some information allowing to detect the switchover, of
course (either in PROC_I(inode) or in ->d_fsdata of that dentry - in the
latter case you'd want to do whatever you need to dispose of that in
->d_release()).  Check in revalidate should be along the lines of "do what's
currently done in get_proc_task_net(), compare the result with the memorized
value, bugger off on mismatch", perhaps with memorized value being
counted as a reference (in which case you'd want to do put_net() when
disposing of the inode or dentry, whichever you use to keep it in).  In
that case proc_tgid_net_lookup()/proc_tgid_net_getattr()/proc_tgid_net_readdir()
would simply use the stored reference instead of messing with get_proc_task_net()
and put_net().

You'd need separate dentry_operations for /proc/*/net and /proc/*/*/net,
instead of using pid_dentry_operations.  That would need to be recognized
in proc_pident_instantiate() (_without_ memcmp(p->name, "net", 4) on each
call of that thing, preferably).  I'd put that new instance of dentry_operations
(along with the methods in it, of course) into fs/proc/proc_net.c, where
we already have the inode and file methods of /proc/*/net.