[PATCH RFC] proc: Don't retain negative dentries

[PATCH RFC] proc: Don't retain negative dentries

[Ahmad Fatoum]

The referenced commit 1da4d377f94 ("proc: revalidate misc dentries")
caused following userspace code to access a stale /proc/net/dev
after the network namespace was changed:

    system("ip netns add testns");

    printf("default:\n"); {
        int devinfd = open("/proc/net/dev", O_RDONLY);
        sendfile(STDOUT_FILENO, devinfd, NULL, 4096);
        close(devinfd);
    }

    printf("testns:\n"); {
        int ns_fd = open("/var/run/netns/testns", O_RDONLY);
        setns(ns_fd, 0);

        int devinfd = open("/proc/net/dev", O_RDONLY);
        sendfile(STDOUT_FILENO, devinfd, NULL, 4096);
        close(devinfd);

        close(ns_fd);
    }

Despite switching the network namespace, the read access from the
newly opened file gave back what the very first read in the default
network namespace returned.

This doesn't occur if /proc/net/dev is opened within a new process,
which might explain why this wasn't noticed previously.

As I don't see a reason why one would keep negative dentries for procfs
at all, amend the code not to do this anymore.

Fixes: 1da4d377f94 ("proc: Don't retain negative dentries")
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---

Notes:
    Alexey, could you check this doesn't lead to a regression
    concerning the bug you fixed?

 fs/proc/generic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/proc/generic.c b/fs/proc/generic.c
index 8ae109429a88..412b3c52d5d5 100644
--- a/fs/proc/generic.c
+++ b/fs/proc/generic.c
@@ -231,7 +231,7 @@ static int proc_misc_d_revalidate(struct dentry *dentry, unsigned int flags)
 
 static int proc_misc_d_delete(const struct dentry *dentry)
 {
-	return atomic_read(&PDE(d_inode(dentry))->in_use) < 0;
+	return 1; // Don't retain negative dentries
 }
 
 static const struct dentry_operations proc_misc_dentry_ops = {
-- 
2.19.0

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

[Ahmad Fatoum]

On 10/8/18 6:50 PM, Ahmad Fatoum wrote:
> The referenced commit 1da4d377f94 ("proc: revalidate misc dentries")
> caused following userspace code to access a stale /proc/net/dev
> after the network namespace was changed:
> 
>     system("ip netns add testns");
> 
>     printf("default:\n"); {
>         int devinfd = open("/proc/net/dev", O_RDONLY);
>         sendfile(STDOUT_FILENO, devinfd, NULL, 4096);
>         close(devinfd);
>     }
> 
>     printf("testns:\n"); {
>         int ns_fd = open("/var/run/netns/testns", O_RDONLY);
>         setns(ns_fd, 0);
> 
>         int devinfd = open("/proc/net/dev", O_RDONLY);
>         sendfile(STDOUT_FILENO, devinfd, NULL, 4096);
>         close(devinfd);
> 
>         close(ns_fd);
>     }
> 
> Despite switching the network namespace, the read access from the
> newly opened file gave back what the very first read in the default
> network namespace returned.
> 
> This doesn't occur if /proc/net/dev is opened within a new process,
> which might explain why this wasn't noticed previously.
> 
> As I don't see a reason why one would keep negative dentries for procfs
> at all, amend the code not to do this anymore.
> 
> Fixes: 1da4d377f94 ("proc: Don't retain negative dentries")

This should read

Fixes: 1da4d377f94 ("proc: revalidate misc dentries") 

instead...

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

[Al Viro]

On Mon, Oct 08, 2018 at 06:50:10PM +0200, Ahmad Fatoum wrote:
> The referenced commit 1da4d377f94 ("proc: revalidate misc dentries")
> caused following userspace code to access a stale /proc/net/dev
> after the network namespace was changed:

> Despite switching the network namespace, the read access from the
> newly opened file gave back what the very first read in the default
> network namespace returned.

What the hell does that have to do with negative dentries anywhere???

NAK.  Rationale makes no sense _and_ the patch has nothing to do
with claimed rationale.

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

[Ahmad Fatoum]

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.

> NAK.  Rationale makes no sense _and_ the patch has nothing to do
> with claimed rationale.

Please CC me when this gets fixed properly. I am curious. :-)


Cheers
Ahmad

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

[Al Viro]

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.

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

[Ahmad Fatoum]

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 |