[PATCH] fs: need to ensure that dentry is initialized before it is added to parent dentry

[PATCH] fs: need to ensure that dentry is initialized before it is added to parent dentry

[zhengbin]

Use a script to test kernel(arm64) in the following steps:
(filesystem is tmpfs, dirA already have 10 files, dirB
have 12 files)
1. keep open filenotexist(O_RDWR) in dirA
2. keep open filenotexist(O_RDWR) in dirB
3. keep ls dirB

After 10 minutes, there will be an oops:
Unable to handle kernel paging request at virtual address 00000000003564ad
Process ls (pid: 142652, stack limit = 0x0000000055c452f6)
Call trace:
 dcache_readdir+0xf8/0x1b0
 iterate_dir+0x8c/0x1a8
 ksys_getdents64+0xa4/0x190
 __arm64_sys_getdents64+0x28/0x38
 el0_svc_common+0x78/0x130
 el0_svc_handler+0x38/0x78
 el0_svc+0x8/0xc

The reason is as follows:
1. dirA create new dentryA(dentryA->next = fileA1), and will delete it
lookup_open
  d_alloc_parallel
    d_alloc
    dput -->prev allocated dentry has been added to dentry_hashtable

dput remove dentryA from dirA, dentryA->next is still fileA1.

2. dirB create new dentry(use dentryA), and add it to dirB
d_alloc -->This will need dirB shared lock
   __d_alloc
     INIT_LIST_HEAD(&dentry->d_child)
   spin_lock(&parent->d_lock)
   list_add(&dentry->d_child, &parent->d_subdirs)

3. At the same time, ls dirB -->This will need dirB shared lock
dcache_readdir
  p = &dentry->d_subdirs
  next_positive
    p = from->next

Although d_alloc has spin_lock, next_positive does not have it since
commit ebaaa80e8f20 ("lockless next_positive()").

Let's look about __d_alloc:
INIT_LIST_HEAD(&dentry->d_child) have 2 steps:
1. dentry->d_child.next = &dentry->d_child
2. dentry->d_child.prev = &dentry->d_child

list_add have 4 steps:
3. parent->d_subdirs.next->prev = &dentry->d_child
4. dentry->d_child.next = parent->d_subdirs.next
5. dentry->d_child.prev = &parent->d_subdirs
6. parent->d_subdirs.next = &dentry->d_child

In arm64, CPU may run out of order. It only guarantees 4 is after 1,
5 is after 2. Maybe set parent->d_subdirs->next first, while
dentry->d_child.next is still uninitialized.

dentryA->next is still fileA1, So ls dirB will goto fileA1 which
belongs to dirA, thus oops happens.

Need to ensure that dentry is initialized before it is added to
parent dentry.
PS: After add smp_wmb, we test it in 10 hours, the oops does not happen.

Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
 fs/dcache.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/fs/dcache.c b/fs/dcache.c
index e88cf05..0a07671 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1767,6 +1767,16 @@ struct dentry *d_alloc(struct dentry * parent, const struct qstr *name)
 	struct dentry *dentry = __d_alloc(parent->d_sb, name);
 	if (!dentry)
 		return NULL;
+
+	/*
+	 * need to ensure that dentry->d_child.next is initialized
+	 * (__d_alloc->INIT_LIST_HEAD) before dentry is added to
+	 * parent->d_subdirs, Otherwise in next_positive(do not have
+	 * spin_lock), we may visit uninitialized value because of cpu
+	 * run optimization(first add dentry to parent->d_subdirs).
+	 */
+	smp_wmb();
+
 	spin_lock(&parent->d_lock);
 	/*
 	 * don't need child lock because it is not subject
--
2.7.4

Re: [PATCH] fs: need to ensure that dentry is initialized before it is added to parent dentry

[Al Viro]

On Wed, Sep 11, 2019 at 01:05:37AM +0800, zhengbin wrote:
> diff --git a/fs/dcache.c b/fs/dcache.c
> index e88cf05..0a07671 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -1767,6 +1767,16 @@ struct dentry *d_alloc(struct dentry * parent, const struct qstr *name)
>  	struct dentry *dentry = __d_alloc(parent->d_sb, name);
>  	if (!dentry)
>  		return NULL;
> +
> +	/*
> +	 * need to ensure that dentry->d_child.next is initialized
> +	 * (__d_alloc->INIT_LIST_HEAD) before dentry is added to
> +	 * parent->d_subdirs, Otherwise in next_positive(do not have
> +	 * spin_lock), we may visit uninitialized value because of cpu
> +	 * run optimization(first add dentry to parent->d_subdirs).
> +	 */
> +	smp_wmb();
> +
>  	spin_lock(&parent->d_lock);
>  	/*
>  	 * don't need child lock because it is not subject
> --
> 2.7.4

Unfortunately, that's not all - see the reply upthread ;-/