Re: discussing about proc_misc_d_delete

Re: discussing about proc_misc_d_delete

[Alexey Dobriyan]

	[cc linux-kernel ]

On Mon, Mar 14, 2022 at 11:54:37AM +0800, hui li wrote:
> We noticed that, commit 1da4d377f94 (“proc: revalidate misc dentries”)
> introduced proc_misc_dentry_ops as default ops for /proc dentry,
> dentry ops for /proc/pid/net/stat/ is set as proc_net_dentry_ops,
> which will revalidate dentry each time when this path is resolved and
> dentry for the stat file is removed from dcache. This time, if files
> under /proc/pid/net/stat/ are in use, then dentries of these files
> will be put in lru when closed, which is meanlingless,  as parrent
> dentry (stat) of these files are remove from dcache.
> 
> This can be reproduced when use linux command "while :;do du
> /proc/;done”, then refcount of each dentry of /proc/pid/net/stat/ will
> increase rapidly which should be deleted at once.

Are you worried that reference count can overflow? Those dentries will be
flushed eventually and reference count goes back to normal values.
This is easy to see with "echo 3 >/proc/sys/vm/drop_caches".

> I think this problem may by solved by checking whether parrent
> dentries are in d_cache inside proc_misc_d_delete, or set
> proc_misc_dentry_ops->d_delete = always_delete_dentry, just as what is
> used in kernel version 4.x and 3.x.
> --- a/fs/proc/generic.c
> +++ b/fs/proc/generic.c
> @@ -236,6 +236,16 @@ static int proc_misc_d_revalidate(struct dentry
> *dentry, unsigned int flags)
> 
>  static int proc_misc_d_delete(const struct dentry *dentry)
>  {
> +       struct dentry *p;
> +       for (p = dentry->d_parent; !IS_ROOT(p); p = p->d_parent) {
> +               if (!spin_trylock(&p->d_lock))
> +                       break;
> +               if (unlikely(d_unhashed(p))){
> +                       spin_unlock(&p->d_lock);
> +                       return 1;
> +               }
> +               spin_unlock(&p->d_lock);
> +       }
>         return atomic_read(&PDE(d_inode(dentry))->in_use) < 0;
>  }

Re: discussing about proc_misc_d_delete

[hui li]

Yes, there may be overflow problems when increased very rapidly
(refcout of /proc/pid/net/). Dentries are put on lru as they may be
accessed again in the future, these dentries will never be accessible
for the system, keeping them on lru is a waste of memory, releasing
them may be a better choice.
In the production environment, we see more than fifty million
proc_inode_cache, using  "echo 3 >/proc/sys/vm/drop_caches" takes long
time and may cause performance problems as drop cache is  a heavy
work. Besides, we believe that in function drop_pagecache_sb there is
a chance that s_inode_list_lock may be held for long time as
"inode->i_mapping->nrpages == 0" is always true which may defer inode
creation and deletion under /proc when there are too much proc inode
caches.

Alexey Dobriyan <adobriyan@gmail.com> 于2022年3月17日周四 17:54写道：
>
>         [cc linux-kernel ]
>
> On Mon, Mar 14, 2022 at 11:54:37AM +0800, hui li wrote:
> > We noticed that, commit 1da4d377f94 (“proc: revalidate misc dentries”)
> > introduced proc_misc_dentry_ops as default ops for /proc dentry,
> > dentry ops for /proc/pid/net/stat/ is set as proc_net_dentry_ops,
> > which will revalidate dentry each time when this path is resolved and
> > dentry for the stat file is removed from dcache. This time, if files
> > under /proc/pid/net/stat/ are in use, then dentries of these files
> > will be put in lru when closed, which is meanlingless,  as parrent
> > dentry (stat) of these files are remove from dcache.
> >
> > This can be reproduced when use linux command "while :;do du
> > /proc/;done”, then refcount of each dentry of /proc/pid/net/stat/ will
> > increase rapidly which should be deleted at once.
>
> Are you worried that reference count can overflow? Those dentries will be
> flushed eventually and reference count goes back to normal values.
> This is easy to see with "echo 3 >/proc/sys/vm/drop_caches".
>
> > I think this problem may by solved by checking whether parrent
> > dentries are in d_cache inside proc_misc_d_delete, or set
> > proc_misc_dentry_ops->d_delete = always_delete_dentry, just as what is
> > used in kernel version 4.x and 3.x.
> > --- a/fs/proc/generic.c
> > +++ b/fs/proc/generic.c
> > @@ -236,6 +236,16 @@ static int proc_misc_d_revalidate(struct dentry
> > *dentry, unsigned int flags)
> >
> >  static int proc_misc_d_delete(const struct dentry *dentry)
> >  {
> > +       struct dentry *p;
> > +       for (p = dentry->d_parent; !IS_ROOT(p); p = p->d_parent) {
> > +               if (!spin_trylock(&p->d_lock))
> > +                       break;
> > +               if (unlikely(d_unhashed(p))){
> > +                       spin_unlock(&p->d_lock);
> > +                       return 1;
> > +               }
> > +               spin_unlock(&p->d_lock);
> > +       }
> >         return atomic_read(&PDE(d_inode(dentry))->in_use) < 0;
> >  }

[PATCH] proc: fix dentry/inode overinstantiating under /proc/${pid}/net

[Alexey Dobriyan]

When a process exits, /proc/${pid}, and /proc/${pid}/net dentries are flushed.
However some leaf dentries like /proc/${pid}/net/arp_cache aren't.
That's because respective PDEs have proc_misc_d_revalidate() hook which
returns 1 and leaves dentries/inodes in the LRU.

Force revalidation/lookup on everything under /proc/${pid}/net by inheriting
proc_net_dentry_ops.

Fixes: c6c75deda813 ("proc: fix lookup in /proc/net subdirectories after setns(2)")
Reported-by: hui li <juanfengpy@gmail.com>
Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 fs/proc/generic.c  |    4 ++++
 fs/proc/proc_net.c |    3 +++
 2 files changed, 7 insertions(+)

--- a/fs/proc/generic.c
+++ b/fs/proc/generic.c
@@ -448,6 +448,10 @@ static struct proc_dir_entry *__proc_create(struct proc_dir_entry **parent,
 	proc_set_user(ent, (*parent)->uid, (*parent)->gid);
 
 	ent->proc_dops = &proc_misc_dentry_ops;
+	/* Revalidate everything under /proc/${pid}/net */
+	if ((*parent)->proc_dops == &proc_net_dentry_ops) {
+		pde_force_lookup(ent);
+	}
 
 out:
 	return ent;
--- a/fs/proc/proc_net.c
+++ b/fs/proc/proc_net.c
@@ -376,6 +376,9 @@ static __net_init int proc_net_ns_init(struct net *net)
 
 	proc_set_user(netd, uid, gid);
 
+	/* Seed dentry revalidation for /proc/${pid}/net */
+	pde_force_lookup(netd);
+
 	err = -EEXIST;
 	net_statd = proc_net_mkdir(net, "stat", netd);
 	if (!net_statd)

Re: [PATCH] proc: fix dentry/inode overinstantiating under /proc/${pid}/net

[hui li]

proc_misc_dentry_ops is a general ops for dentry under /proc, except
for "/proc/${pid}/net"，other dentries may also use there own ops too,
so I think change proc_misc_d_delete may be better?
see patch under: https://lkml.org/lkml/2022/3/17/319

Alexey Dobriyan <adobriyan@gmail.com> 于2022年3月21日周一 00:24写道：
>
> When a process exits, /proc/${pid}, and /proc/${pid}/net dentries are flushed.
> However some leaf dentries like /proc/${pid}/net/arp_cache aren't.
> That's because respective PDEs have proc_misc_d_revalidate() hook which
> returns 1 and leaves dentries/inodes in the LRU.
>
> Force revalidation/lookup on everything under /proc/${pid}/net by inheriting
> proc_net_dentry_ops.
>
> Fixes: c6c75deda813 ("proc: fix lookup in /proc/net subdirectories after setns(2)")
> Reported-by: hui li <juanfengpy@gmail.com>
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> ---
>
>  fs/proc/generic.c  |    4 ++++
>  fs/proc/proc_net.c |    3 +++
>  2 files changed, 7 insertions(+)
>
> --- a/fs/proc/generic.c
> +++ b/fs/proc/generic.c
> @@ -448,6 +448,10 @@ static struct proc_dir_entry *__proc_create(struct proc_dir_entry **parent,
>         proc_set_user(ent, (*parent)->uid, (*parent)->gid);
>
>         ent->proc_dops = &proc_misc_dentry_ops;
> +       /* Revalidate everything under /proc/${pid}/net */
> +       if ((*parent)->proc_dops == &proc_net_dentry_ops) {
> +               pde_force_lookup(ent);
> +       }
>
>  out:
>         return ent;
> --- a/fs/proc/proc_net.c
> +++ b/fs/proc/proc_net.c
> @@ -376,6 +376,9 @@ static __net_init int proc_net_ns_init(struct net *net)
>
>         proc_set_user(netd, uid, gid);
>
> +       /* Seed dentry revalidation for /proc/${pid}/net */
> +       pde_force_lookup(netd);
> +
>         err = -EEXIST;
>         net_statd = proc_net_mkdir(net, "stat", netd);
>         if (!net_statd)

Re: [PATCH] proc: fix dentry/inode overinstantiating under /proc/${pid}/net

[Alexey Dobriyan]

On Mon, Mar 21, 2022 at 05:15:02PM +0800, hui li wrote:
> Alexey Dobriyan <adobriyan@gmail.com> 于2022年3月21日周一 00:24写道：
> >
> > When a process exits, /proc/${pid}, and /proc/${pid}/net dentries are flushed.
> > However some leaf dentries like /proc/${pid}/net/arp_cache aren't.
> > That's because respective PDEs have proc_misc_d_revalidate() hook which
> > returns 1 and leaves dentries/inodes in the LRU.
> >
> > Force revalidation/lookup on everything under /proc/${pid}/net by inheriting
> > proc_net_dentry_ops.
> >
> > Fixes: c6c75deda813 ("proc: fix lookup in /proc/net subdirectories after setns(2)")
> > Reported-by: hui li <juanfengpy@gmail.com>
> > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> > ---
> >
> >  fs/proc/generic.c  |    4 ++++
> >  fs/proc/proc_net.c |    3 +++
> >  2 files changed, 7 insertions(+)
> >
> > --- a/fs/proc/generic.c
> > +++ b/fs/proc/generic.c
> > @@ -448,6 +448,10 @@ static struct proc_dir_entry *__proc_create(struct proc_dir_entry **parent,
> >         proc_set_user(ent, (*parent)->uid, (*parent)->gid);
> >
> >         ent->proc_dops = &proc_misc_dentry_ops;
> > +       /* Revalidate everything under /proc/${pid}/net */
> > +       if ((*parent)->proc_dops == &proc_net_dentry_ops) {
> > +               pde_force_lookup(ent);
> > +       }
> >
> >  out:
> >         return ent;
> > --- a/fs/proc/proc_net.c
> > +++ b/fs/proc/proc_net.c
> > @@ -376,6 +376,9 @@ static __net_init int proc_net_ns_init(struct net *net)
> >
> >         proc_set_user(netd, uid, gid);
> >
> > +       /* Seed dentry revalidation for /proc/${pid}/net */
> > +       pde_force_lookup(netd);
> > +
> >         err = -EEXIST;
> >         net_statd = proc_net_mkdir(net, "stat", netd);
> >         if (!net_statd)

> proc_misc_dentry_ops is a general ops for dentry under /proc, except
> for "/proc/${pid}/net"，other dentries may also use there own ops too,
> so I think change proc_misc_d_delete may be better?
> see patch under: https://lkml.org/lkml/2022/3/17/319

I don't think so.

proc_misc_d_delete covers "everything else" part under /proc/ and
/proc/net which are 2 separate trees. Now /proc/net/ requires
revalidation because of

	commit c6c75deda81344c3a95d1d1f606d5cee109e5d54
	proc: fix lookup in /proc/net subdirectories after setns(2)

so the bug is that the above commit was applied only partially.
In particular, /proc/*/net/stat/arp_cache was created with
proc_create_seq_data(), avoiding proc_net_* APIs.

And there is probably the same "lookup after setns find wrong file"
if you search hard enough in /proc/*/net/

This is the logic. Please test on your systems.

Re: [PATCH] proc: fix dentry/inode overinstantiating under /proc/${pid}/net

[cael]

I see, I will test on my system.

Alexey Dobriyan <adobriyan@gmail.com> 于2022年3月21日周一 18:40写道：
>
> On Mon, Mar 21, 2022 at 05:15:02PM +0800, hui li wrote:
> > Alexey Dobriyan <adobriyan@gmail.com> 于2022年3月21日周一 00:24写道：
> > >
> > > When a process exits, /proc/${pid}, and /proc/${pid}/net dentries are flushed.
> > > However some leaf dentries like /proc/${pid}/net/arp_cache aren't.
> > > That's because respective PDEs have proc_misc_d_revalidate() hook which
> > > returns 1 and leaves dentries/inodes in the LRU.
> > >
> > > Force revalidation/lookup on everything under /proc/${pid}/net by inheriting
> > > proc_net_dentry_ops.
> > >
> > > Fixes: c6c75deda813 ("proc: fix lookup in /proc/net subdirectories after setns(2)")
> > > Reported-by: hui li <juanfengpy@gmail.com>
> > > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> > > ---
> > >
> > >  fs/proc/generic.c  |    4 ++++
> > >  fs/proc/proc_net.c |    3 +++
> > >  2 files changed, 7 insertions(+)
> > >
> > > --- a/fs/proc/generic.c
> > > +++ b/fs/proc/generic.c
> > > @@ -448,6 +448,10 @@ static struct proc_dir_entry *__proc_create(struct proc_dir_entry **parent,
> > >         proc_set_user(ent, (*parent)->uid, (*parent)->gid);
> > >
> > >         ent->proc_dops = &proc_misc_dentry_ops;
> > > +       /* Revalidate everything under /proc/${pid}/net */
> > > +       if ((*parent)->proc_dops == &proc_net_dentry_ops) {
> > > +               pde_force_lookup(ent);
> > > +       }
> > >
> > >  out:
> > >         return ent;
> > > --- a/fs/proc/proc_net.c
> > > +++ b/fs/proc/proc_net.c
> > > @@ -376,6 +376,9 @@ static __net_init int proc_net_ns_init(struct net *net)
> > >
> > >         proc_set_user(netd, uid, gid);
> > >
> > > +       /* Seed dentry revalidation for /proc/${pid}/net */
> > > +       pde_force_lookup(netd);
> > > +
> > >         err = -EEXIST;
> > >         net_statd = proc_net_mkdir(net, "stat", netd);
> > >         if (!net_statd)
>
> > proc_misc_dentry_ops is a general ops for dentry under /proc, except
> > for "/proc/${pid}/net"，other dentries may also use there own ops too,
> > so I think change proc_misc_d_delete may be better?
> > see patch under: https://lkml.org/lkml/2022/3/17/319
>
> I don't think so.
>
> proc_misc_d_delete covers "everything else" part under /proc/ and
> /proc/net which are 2 separate trees. Now /proc/net/ requires
> revalidation because of
>
>         commit c6c75deda81344c3a95d1d1f606d5cee109e5d54
>         proc: fix lookup in /proc/net subdirectories after setns(2)
>
> so the bug is that the above commit was applied only partially.
> In particular, /proc/*/net/stat/arp_cache was created with
> proc_create_seq_data(), avoiding proc_net_* APIs.
>
> And there is probably the same "lookup after setns find wrong file"
> if you search hard enough in /proc/*/net/
>
> This is the logic. Please test on your systems.