Message-ID: <93e26435-985a-0a1e-9bd5-18f9bcc6142d@pengutronix.de>
Date:   Mon, 15 Oct 2018 11:56:44 +0200
From:   Ahmad Fatoum <a.fatoum@...gutronix.de>
To:     Al Viro <viro@...IV.linux.org.uk>
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

Hello,

Thanks very much for the elaboration!

On 8/10/18 20:01, Al Viro wrote:
> On Mon, Oct 08, 2018 at 07:02:09PM +0200, Ahmad Fatoum wrote:
> 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.

Sorry for the misunderstanding on my part. The referenced commit replaced
simple_dentry_operations whose only non-NULL member points at:

/*
 * Retaining negative dentries for an in-memory filesystem just wastes
 * memory and lookup time: arrange for them to be deleted immediately.
 */
int always_delete_dentry(const struct dentry *dentry)
{
	return 1;
}

which is what I based my rationale on.

> ->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.

I see. Would that mean that previously it only worked by chance?

> 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.
> 

I will need to understand why my patch seemed to fix it and would try to
find some time later this month to implement it properly (or if someone
finds that time now, I would appreciate that too!)


Thanks again
Ahmad

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Powered by blists - more mailing lists