[PATCH 0/8] netfs/cachefiles: Some bugfixes

[PATCH 0/8] netfs/cachefiles: Some bugfixes

[Zizhi Wo]

Hi!

We recently discovered some bugs through self-discovery and testing in
erofs ondemand loading mode, and this patchset is mainly used to fix
them. These patches are relatively simple changes, and I would be excited
to discuss them together with everyone. Below is a brief introduction to
each patch:

Patch 1: Fix for wrong block_number calculated in ondemand write.

Patch 2: Fix for wrong length return value in ondemand write.

Patch 3: Fix missing position update in ondemand write, for scenarios
involving read-ahead, invoking the write syscall.

Patch 4: Previously, the last redundant data was cleared during the umount
phase. This patch remove unnecessary data in advance.

Patch 5: Code clean up for cachefiles_commit_tmpfile().

Patch 6: Modify error return value in cachefiles_daemon_secctx().

Patch 7: Fix object->file Null-pointer-dereference problem.

Patch 8: Fix for memory out of order in fscache_create_volume().


Zizhi Wo (8):
  cachefiles: Fix incorrect block calculations in
    __cachefiles_prepare_write()
  cachefiles: Fix incorrect length return value in
    cachefiles_ondemand_fd_write_iter()
  cachefiles: Fix missing pos updates in
    cachefiles_ondemand_fd_write_iter()
  cachefiles: Clear invalid cache data in advance
  cachefiles: Clean up in cachefiles_commit_tmpfile()
  cachefiles: Modify inappropriate error return value in
    cachefiles_daemon_secctx()
  cachefiles: Fix NULL pointer dereference in object->file
  netfs/fscache: Add a memory barrier for FSCACHE_VOLUME_CREATING

 fs/cachefiles/daemon.c    |  2 +-
 fs/cachefiles/interface.c |  3 +++
 fs/cachefiles/io.c        | 10 +++++-----
 fs/cachefiles/namei.c     | 23 +++++++++++++----------
 fs/cachefiles/ondemand.c  | 38 +++++++++++++++++++++++++++++---------
 fs/netfs/fscache_volume.c |  3 +--
 6 files changed, 52 insertions(+), 27 deletions(-)

-- 
2.39.2

[PATCH 1/8] cachefiles: Fix incorrect block calculations in __cachefiles_prepare_write()

[Zizhi Wo]

In the __cachefiles_prepare_write function, DIO aligns blocks using
PAGE_SIZE as the unit. And currently cachefiles_add_cache() binds
cache->bsize with the requirement that it must not exceed PAGE_SIZE.
However, if cache->bsize is smaller than PAGE_SIZE, the calculated block
count will be incorrect in __cachefiles_prepare_write().

Set the block size to cache->bsize to resolve this issue.

Fixes: 047487c947e8 ("cachefiles: Implement the I/O routines")
Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
---
 fs/cachefiles/io.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/cachefiles/io.c b/fs/cachefiles/io.c
index a91acd03ee12..59c5c08f921a 100644
--- a/fs/cachefiles/io.c
+++ b/fs/cachefiles/io.c
@@ -524,10 +524,10 @@ int __cachefiles_prepare_write(struct cachefiles_object *object,
 	struct cachefiles_cache *cache = object->volume->cache;
 	loff_t start = *_start, pos;
 	size_t len = *_len;
-	int ret;
+	int ret, block_size = cache->bsize;
 
 	/* Round to DIO size */
-	start = round_down(*_start, PAGE_SIZE);
+	start = round_down(*_start, block_size);
 	if (start != *_start || *_len > upper_len) {
 		/* Probably asked to cache a streaming write written into the
 		 * pagecache when the cookie was temporarily out of service to
@@ -537,7 +537,7 @@ int __cachefiles_prepare_write(struct cachefiles_object *object,
 		return -ENOBUFS;
 	}
 
-	*_len = round_up(len, PAGE_SIZE);
+	*_len = round_up(len, block_size);
 
 	/* We need to work out whether there's sufficient disk space to perform
 	 * the write - but we can skip that check if we have space already
@@ -563,7 +563,7 @@ int __cachefiles_prepare_write(struct cachefiles_object *object,
 	 * space, we need to see if it's fully allocated.  If it's not, we may
 	 * want to cull it.
 	 */
-	if (cachefiles_has_space(cache, 0, *_len / PAGE_SIZE,
+	if (cachefiles_has_space(cache, 0, *_len / block_size,
 				 cachefiles_has_space_check) == 0)
 		return 0; /* Enough space to simply overwrite the whole block */
 
@@ -595,7 +595,7 @@ int __cachefiles_prepare_write(struct cachefiles_object *object,
 	return ret;
 
 check_space:
-	return cachefiles_has_space(cache, 0, *_len / PAGE_SIZE,
+	return cachefiles_has_space(cache, 0, *_len / block_size,
 				    cachefiles_has_space_for_write);
 }
 
-- 
2.39.2

[PATCH 2/8] cachefiles: Fix incorrect length return value in cachefiles_ondemand_fd_write_iter()

[Zizhi Wo]

In the current on-demand loading mode, cachefiles_ondemand_fd_write_iter()
function first aligns "pos" and "len" to block boundaries. When calling
__cachefiles_write(), the aligned "pos" is passed in, but "len" is the
original unaligned value(iter->count). Additionally, the returned length of
the write operation is the modified "len" aligned by block size, which is
unreasonable.

The alignment of "pos" and "len" is intended only to check whether the
cache has enough space in erofs ondemand mode. But the modified len should
not be used as the return value of cachefiles_ondemand_fd_write_iter().
Doing so would result in a mismatch in the data written on-demand. For
example, if the length of the user state passed in is not aligned to the
block size (the preread scene / DIO writes only need 512 alignment), the
length of the write will differ from the actual length of the return.

To solve this issue, since the __cachefiles_prepare_write() modifies the
size of "len", we pass "aligned_len" to __cachefiles_prepare_write() to
calculate the free blocks and use the original "len" as the return value of
cachefiles_ondemand_fd_write_iter().

Fixes: c8383054506c ("cachefiles: notify the user daemon when looking up cookie")
Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
---
 fs/cachefiles/ondemand.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
index 470c96658385..bdd321017f1c 100644
--- a/fs/cachefiles/ondemand.c
+++ b/fs/cachefiles/ondemand.c
@@ -61,7 +61,7 @@ static ssize_t cachefiles_ondemand_fd_write_iter(struct kiocb *kiocb,
 	struct cachefiles_object *object = kiocb->ki_filp->private_data;
 	struct cachefiles_cache *cache = object->volume->cache;
 	struct file *file = object->file;
-	size_t len = iter->count;
+	size_t len = iter->count, aligned_len = len;
 	loff_t pos = kiocb->ki_pos;
 	const struct cred *saved_cred;
 	int ret;
@@ -70,7 +70,7 @@ static ssize_t cachefiles_ondemand_fd_write_iter(struct kiocb *kiocb,
 		return -ENOBUFS;
 
 	cachefiles_begin_secure(cache, &saved_cred);
-	ret = __cachefiles_prepare_write(object, file, &pos, &len, len, true);
+	ret = __cachefiles_prepare_write(object, file, &pos, &aligned_len, len, true);
 	cachefiles_end_secure(cache, saved_cred);
 	if (ret < 0)
 		return ret;
-- 
2.39.2

[PATCH 3/8] cachefiles: Fix missing pos updates in cachefiles_ondemand_fd_write_iter()

[Zizhi Wo]

In the erofs on-demand loading scenario, read and write operations are
usually delivered through "off" and "len" contained in read req in user
mode. Naturally, pwrite is used to specify a specific offset to complete
write operations.

However, if the write(not pwrite) syscall is called multiple times in the
read-ahead scenario, we need to manually update ki_pos after each write
operation to update file->f_pos.

This step is currently missing from the cachefiles_ondemand_fd_write_iter
function, added to address this issue.

Fixes: c8383054506c ("cachefiles: notify the user daemon when looking up cookie")
Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
---
 fs/cachefiles/ondemand.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
index bdd321017f1c..38ca6dce8ef2 100644
--- a/fs/cachefiles/ondemand.c
+++ b/fs/cachefiles/ondemand.c
@@ -77,8 +77,10 @@ static ssize_t cachefiles_ondemand_fd_write_iter(struct kiocb *kiocb,
 
 	trace_cachefiles_ondemand_fd_write(object, file_inode(file), pos, len);
 	ret = __cachefiles_write(object, file, pos, iter, NULL, NULL);
-	if (!ret)
+	if (!ret) {
 		ret = len;
+		kiocb->ki_pos += ret;
+	}
 
 	return ret;
 }
-- 
2.39.2

[PATCH 4/8] cachefiles: Clear invalid cache data in advance

[Zizhi Wo]

In the current on-demand loading scenario, when umount is called, the
cachefiles_commit_tmpfile() is invoked. When checking the inode
corresponding to object->file is inconsistent with the dentry,
cachefiles_unlink() is called to perform cleanup to prevent invalid data
from occupying space.

The above operation does not apply to the first mount, because the cache
dentry generated by the first mount must be negative. Moreover, there is no
need to clear it during the first umount because this part of the data may
be reusable in the future. But the problem is that, the clean operation can
currently only be called through cachefiles_withdraw_cookie(), in other
words the redundant data does not cleaned until the second umount. This
means that during the second mount, the old cache data generated from the
first mount still occupies space. So if the user does not manually clean up
the previous cache before the next mount, it may return insufficient space
during the second mount phase.

This patch adds an additional cleanup process in the cachefiles_open_file()
function. When the auxdata check fails, the remaining old cache data is no
longer needed, the file and dentry corresponding to the object are also
put. As there is no need to clear it until umount, we can directly clear it
during the mount process.

Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
---
 fs/cachefiles/namei.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
index f53977169db4..70b0b3477085 100644
--- a/fs/cachefiles/namei.c
+++ b/fs/cachefiles/namei.c
@@ -542,7 +542,7 @@ static bool cachefiles_create_file(struct cachefiles_object *object)
  * stale.
  */
 static bool cachefiles_open_file(struct cachefiles_object *object,
-				 struct dentry *dentry)
+				 struct dentry *dir, struct dentry *dentry)
 {
 	struct cachefiles_cache *cache = object->volume->cache;
 	struct file *file;
@@ -601,10 +601,18 @@ static bool cachefiles_open_file(struct cachefiles_object *object,
 check_failed:
 	fscache_cookie_lookup_negative(object->cookie);
 	cachefiles_unmark_inode_in_use(object, file);
-	fput(file);
-	dput(dentry);
-	if (ret == -ESTALE)
+	__fput_sync(file);
+	if (ret == -ESTALE) {
+		/* When the auxdata check fails, the remaining old cache data
+		 * is no longer needed, and we will clear it here first.
+		 */
+		inode_lock_nested(d_inode(dir), I_MUTEX_PARENT);
+		cachefiles_unlink(cache, object, dir, dentry, FSCACHE_OBJECT_IS_STALE);
+		inode_unlock(d_inode(dir));
+		dput(dentry);
 		return cachefiles_create_file(object);
+	}
+	dput(dentry);
 	return false;
 
 error_fput:
@@ -654,7 +662,7 @@ bool cachefiles_look_up_object(struct cachefiles_object *object)
 		goto new_file;
 	}
 
-	if (!cachefiles_open_file(object, dentry))
+	if (!cachefiles_open_file(object, fan, dentry))
 		return false;
 
 	_leave(" = t [%lu]", file_inode(object->file)->i_ino);
-- 
2.39.2

[PATCH 5/8] cachefiles: Clean up in cachefiles_commit_tmpfile()

[Zizhi Wo]

Currently, cachefiles_commit_tmpfile() will only be called if object->flags
is set to CACHEFILES_OBJECT_USING_TMPFILE. Only cachefiles_create_file()
and cachefiles_invalidate_cookie() set this flag. Both of these functions
replace object->file with the new tmpfile, and both are called by
fscache_cookie_state_machine(), so there are no concurrency issues.

So the equation "d_backing_inode(dentry) == file_inode(object->file)" in
cachefiles_commit_tmpfile() will never hold true according to the above
conditions. This patch removes this part of the redundant code and does not
involve any other logical changes.

Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
---
 fs/cachefiles/namei.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
index 70b0b3477085..ce9d558ae4fa 100644
--- a/fs/cachefiles/namei.c
+++ b/fs/cachefiles/namei.c
@@ -700,11 +700,6 @@ bool cachefiles_commit_tmpfile(struct cachefiles_cache *cache,
 	}
 
 	if (!d_is_negative(dentry)) {
-		if (d_backing_inode(dentry) == file_inode(object->file)) {
-			success = true;
-			goto out_dput;
-		}
-
 		ret = cachefiles_unlink(volume->cache, object, fan, dentry,
 					FSCACHE_OBJECT_IS_STALE);
 		if (ret < 0)
-- 
2.39.2

[PATCH 6/8] cachefiles: Modify inappropriate error return value in cachefiles_daemon_secctx()

[Zizhi Wo]

In cachefiles_daemon_secctx(), if it is detected that secctx has been
written to the cache, the error code returned is -EINVAL, which is
inappropriate and does not distinguish the situation well.

Like cachefiles_daemon_dir(), fix this issue by return -EEXIST to the user
if it has already been defined once.

Fixes: 8667d434b2a9 ("cachefiles: Register a miscdev and parse commands over it")
Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
---
 fs/cachefiles/daemon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cachefiles/daemon.c b/fs/cachefiles/daemon.c
index 89b11336a836..59e576951c68 100644
--- a/fs/cachefiles/daemon.c
+++ b/fs/cachefiles/daemon.c
@@ -587,7 +587,7 @@ static int cachefiles_daemon_secctx(struct cachefiles_cache *cache, char *args)
 
 	if (cache->secctx) {
 		pr_err("Second security context specified\n");
-		return -EINVAL;
+		return -EEXIST;
 	}
 
 	secctx = kstrdup(args, GFP_KERNEL);
-- 
2.39.2

[PATCH 7/8] cachefiles: Fix NULL pointer dereference in object->file

[Zizhi Wo]

At present, the object->file has the NULL pointer dereference problem in
ondemand-mode. The root cause is that the allocated fd and object->file
lifetime are inconsistent, and the user-space invocation to anon_fd uses
object->file. Following is the process that triggers the issue:

	  [write fd]				[umount]
cachefiles_ondemand_fd_write_iter
				       fscache_cookie_state_machine
					 cachefiles_withdraw_cookie
  if (!file) return -ENOBUFS
					   cachefiles_clean_up_object
					     cachefiles_unmark_inode_in_use
					     fput(object->file)
					     object->file = NULL
  // file NULL pointer dereference!
  __cachefiles_write(..., file, ...)

Fix this issue by add an additional reference count to the object->file
before write/llseek, and decrement after it finished.

Fixes: c8383054506c ("cachefiles: notify the user daemon when looking up cookie")
Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
---
 fs/cachefiles/interface.c |  3 +++
 fs/cachefiles/ondemand.c  | 30 ++++++++++++++++++++++++------
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/fs/cachefiles/interface.c b/fs/cachefiles/interface.c
index 35ba2117a6f6..d30127ead911 100644
--- a/fs/cachefiles/interface.c
+++ b/fs/cachefiles/interface.c
@@ -342,10 +342,13 @@ static void cachefiles_clean_up_object(struct cachefiles_object *object,
 	}
 
 	cachefiles_unmark_inode_in_use(object, object->file);
+
+	spin_lock(&object->lock);
 	if (object->file) {
 		fput(object->file);
 		object->file = NULL;
 	}
+	spin_unlock(&object->lock);
 }
 
 /*
diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
index 38ca6dce8ef2..fe3de9ad57bf 100644
--- a/fs/cachefiles/ondemand.c
+++ b/fs/cachefiles/ondemand.c
@@ -60,20 +60,26 @@ static ssize_t cachefiles_ondemand_fd_write_iter(struct kiocb *kiocb,
 {
 	struct cachefiles_object *object = kiocb->ki_filp->private_data;
 	struct cachefiles_cache *cache = object->volume->cache;
-	struct file *file = object->file;
+	struct file *file;
 	size_t len = iter->count, aligned_len = len;
 	loff_t pos = kiocb->ki_pos;
 	const struct cred *saved_cred;
 	int ret;
 
-	if (!file)
+	spin_lock(&object->lock);
+	file = object->file;
+	if (!file) {
+		spin_unlock(&object->lock);
 		return -ENOBUFS;
+	}
+	get_file(file);
+	spin_unlock(&object->lock);
 
 	cachefiles_begin_secure(cache, &saved_cred);
 	ret = __cachefiles_prepare_write(object, file, &pos, &aligned_len, len, true);
 	cachefiles_end_secure(cache, saved_cred);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	trace_cachefiles_ondemand_fd_write(object, file_inode(file), pos, len);
 	ret = __cachefiles_write(object, file, pos, iter, NULL, NULL);
@@ -82,6 +88,8 @@ static ssize_t cachefiles_ondemand_fd_write_iter(struct kiocb *kiocb,
 		kiocb->ki_pos += ret;
 	}
 
+out:
+	fput(file);
 	return ret;
 }
 
@@ -89,12 +97,22 @@ static loff_t cachefiles_ondemand_fd_llseek(struct file *filp, loff_t pos,
 					    int whence)
 {
 	struct cachefiles_object *object = filp->private_data;
-	struct file *file = object->file;
+	struct file *file;
+	loff_t ret;
 
-	if (!file)
+	spin_lock(&object->lock);
+	file = object->file;
+	if (!file) {
+		spin_unlock(&object->lock);
 		return -ENOBUFS;
+	}
+	get_file(file);
+	spin_unlock(&object->lock);
 
-	return vfs_llseek(file, pos, whence);
+	ret = vfs_llseek(file, pos, whence);
+	fput(file);
+
+	return ret;
 }
 
 static long cachefiles_ondemand_fd_ioctl(struct file *filp, unsigned int ioctl,
-- 
2.39.2

[PATCH 8/8] netfs/fscache: Add a memory barrier for FSCACHE_VOLUME_CREATING

[Zizhi Wo]

In fscache_create_volume(), there is a missing memory barrier between the
bit-clearing operation and the wake-up operation. This may cause a
situation where, after a wake-up, the bit-clearing operation hasn't been
detected yet, leading to an indefinite wait. The triggering process is as
follows:

  [cookie1]                [cookie2]                  [volume_work]
fscache_perform_lookup
  fscache_create_volume
                        fscache_perform_lookup
                          fscache_create_volume
			                        fscache_create_volume_work
                                                  cachefiles_acquire_volume
                                                  clear_and_wake_up_bit
    test_and_set_bit
                            test_and_set_bit
                              goto maybe_wait
      goto no_wait

In the above process, cookie1 and cookie2 has the same volume. When cookie1
enters the -no_wait- process, it will clear the bit and wake up the waiting
process. If a barrier is missing, it may cause cookie2 to remain in the
-wait- process indefinitely.

In commit 3288666c7256 ("fscache: Use clear_and_wake_up_bit() in
fscache_create_volume_work()"), barriers were added to similar operations
in fscache_create_volume_work(), but fscache_create_volume() was missed.

By combining the clear and wake operations into clear_and_wake_up_bit() to
fix this issue.

Fixes: bfa22da3ed65 ("fscache: Provide and use cache methods to lookup/create/free a volume")
Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
---
 fs/netfs/fscache_volume.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/netfs/fscache_volume.c b/fs/netfs/fscache_volume.c
index cb75c07b5281..ced14ac78cc1 100644
--- a/fs/netfs/fscache_volume.c
+++ b/fs/netfs/fscache_volume.c
@@ -322,8 +322,7 @@ void fscache_create_volume(struct fscache_volume *volume, bool wait)
 	}
 	return;
 no_wait:
-	clear_bit_unlock(FSCACHE_VOLUME_CREATING, &volume->flags);
-	wake_up_bit(&volume->flags, FSCACHE_VOLUME_CREATING);
+	clear_and_wake_up_bit(FSCACHE_VOLUME_CREATING, &volume->flags);
 }
 
 /*
-- 
2.39.2

Re: [PATCH 0/8] netfs/cachefiles: Some bugfixes

[Zizhi Wo]

Hi!

This patchset involves some general cachefiles workflows and the on-
demand loading process. For example, the eighth patch fixes a memory
ordering issue in cachefiles, and the fifth patch includes some cleanup.
These all related to changes in the general cachefiles workflow, and I
think these deserve some attention.

Additionally, although the current EROFS on-demand loading mode based on
cachefiles interaction might be considered for switching to the fanotify
mode in the future, I believe the code based on the current cachefiles
on-demand loading mode still requires maintenance. The first few patches
here are bugfixes specifically for that.

Therefore, I would greatly appreciate it if anyone could take some time
to review these patches. So friendly ping.

Thanks,
Zizhi Wo


在 2024/8/21 10:42, Zizhi Wo 写道:
> Hi!
> 
> We recently discovered some bugs through self-discovery and testing in
> erofs ondemand loading mode, and this patchset is mainly used to fix
> them. These patches are relatively simple changes, and I would be excited
> to discuss them together with everyone. Below is a brief introduction to
> each patch:
> 
> Patch 1: Fix for wrong block_number calculated in ondemand write.
> 
> Patch 2: Fix for wrong length return value in ondemand write.
> 
> Patch 3: Fix missing position update in ondemand write, for scenarios
> involving read-ahead, invoking the write syscall.
> 
> Patch 4: Previously, the last redundant data was cleared during the umount
> phase. This patch remove unnecessary data in advance.
> 
> Patch 5: Code clean up for cachefiles_commit_tmpfile().
> 
> Patch 6: Modify error return value in cachefiles_daemon_secctx().
> 
> Patch 7: Fix object->file Null-pointer-dereference problem.
> 
> Patch 8: Fix for memory out of order in fscache_create_volume().
> 
> 
> Zizhi Wo (8):
>    cachefiles: Fix incorrect block calculations in
>      __cachefiles_prepare_write()
>    cachefiles: Fix incorrect length return value in
>      cachefiles_ondemand_fd_write_iter()
>    cachefiles: Fix missing pos updates in
>      cachefiles_ondemand_fd_write_iter()
>    cachefiles: Clear invalid cache data in advance
>    cachefiles: Clean up in cachefiles_commit_tmpfile()
>    cachefiles: Modify inappropriate error return value in
>      cachefiles_daemon_secctx()
>    cachefiles: Fix NULL pointer dereference in object->file
>    netfs/fscache: Add a memory barrier for FSCACHE_VOLUME_CREATING
> 
>   fs/cachefiles/daemon.c    |  2 +-
>   fs/cachefiles/interface.c |  3 +++
>   fs/cachefiles/io.c        | 10 +++++-----
>   fs/cachefiles/namei.c     | 23 +++++++++++++----------
>   fs/cachefiles/ondemand.c  | 38 +++++++++++++++++++++++++++++---------
>   fs/netfs/fscache_volume.c |  3 +--
>   6 files changed, 52 insertions(+), 27 deletions(-)
> 

Re: [PATCH 0/8] netfs/cachefiles: Some bugfixes

[Gao Xiang]

Hi Zizhi,

On 2024/10/10 11:08, Zizhi Wo wrote:
> Hi!
> 
> This patchset involves some general cachefiles workflows and the on-
> demand loading process. For example, the eighth patch fixes a memory
> ordering issue in cachefiles, and the fifth patch includes some cleanup.
> These all related to changes in the general cachefiles workflow, and I
> think these deserve some attention.
> 
> Additionally, although the current EROFS on-demand loading mode based on
> cachefiles interaction might be considered for switching to the fanotify
> mode in the future, I believe the code based on the current cachefiles
> on-demand loading mode still requires maintenance. The first few patches
> here are bugfixes specifically for that.

Yes, I also agree with you.  I pinged David weeks ago, because many
bugfixes are not only impacted to cachefiles on-demand feature but
also generic cachefiles, hopefully they could be addressed upstream.

Thanks,
Gao Xiang

> 
> Therefore, I would greatly appreciate it if anyone could take some time
> to review these patches. So friendly ping.
> 
> Thanks,
> Zizhi Wo
> 
> 
> 在 2024/8/21 10:42, Zizhi Wo 写道:
>> Hi!
>>
>> We recently discovered some bugs through self-discovery and testing in
>> erofs ondemand loading mode, and this patchset is mainly used to fix
>> them. These patches are relatively simple changes, and I would be excited
>> to discuss them together with everyone. Below is a brief introduction to
>> each patch:
>>
>> Patch 1: Fix for wrong block_number calculated in ondemand write.
>>
>> Patch 2: Fix for wrong length return value in ondemand write.
>>
>> Patch 3: Fix missing position update in ondemand write, for scenarios
>> involving read-ahead, invoking the write syscall.
>>
>> Patch 4: Previously, the last redundant data was cleared during the umount
>> phase. This patch remove unnecessary data in advance.
>>
>> Patch 5: Code clean up for cachefiles_commit_tmpfile().
>>
>> Patch 6: Modify error return value in cachefiles_daemon_secctx().
>>
>> Patch 7: Fix object->file Null-pointer-dereference problem.
>>
>> Patch 8: Fix for memory out of order in fscache_create_volume().
>>
>>
>> Zizhi Wo (8):
>>    cachefiles: Fix incorrect block calculations in
>>      __cachefiles_prepare_write()
>>    cachefiles: Fix incorrect length return value in
>>      cachefiles_ondemand_fd_write_iter()
>>    cachefiles: Fix missing pos updates in
>>      cachefiles_ondemand_fd_write_iter()
>>    cachefiles: Clear invalid cache data in advance
>>    cachefiles: Clean up in cachefiles_commit_tmpfile()
>>    cachefiles: Modify inappropriate error return value in
>>      cachefiles_daemon_secctx()
>>    cachefiles: Fix NULL pointer dereference in object->file
>>    netfs/fscache: Add a memory barrier for FSCACHE_VOLUME_CREATING
>>
>>   fs/cachefiles/daemon.c    |  2 +-
>>   fs/cachefiles/interface.c |  3 +++
>>   fs/cachefiles/io.c        | 10 +++++-----
>>   fs/cachefiles/namei.c     | 23 +++++++++++++----------
>>   fs/cachefiles/ondemand.c  | 38 +++++++++++++++++++++++++++++---------
>>   fs/netfs/fscache_volume.c |  3 +--
>>   6 files changed, 52 insertions(+), 27 deletions(-)
>>

Re: [PATCH 0/8] netfs/cachefiles: Some bugfixes

[Zizhi Wo]

在 2024/10/10 11:31, Gao Xiang 写道:
> Hi Zizhi,
> 
> On 2024/10/10 11:08, Zizhi Wo wrote:
>> Hi!
>>
>> This patchset involves some general cachefiles workflows and the on-
>> demand loading process. For example, the eighth patch fixes a memory
>> ordering issue in cachefiles, and the fifth patch includes some cleanup.
>> These all related to changes in the general cachefiles workflow, and I
>> think these deserve some attention.
>>
>> Additionally, although the current EROFS on-demand loading mode based on
>> cachefiles interaction might be considered for switching to the fanotify
>> mode in the future, I believe the code based on the current cachefiles
>> on-demand loading mode still requires maintenance. The first few patches
>> here are bugfixes specifically for that.
> 
> Yes, I also agree with you.  I pinged David weeks ago, because many
> bugfixes are not only impacted to cachefiles on-demand feature but
> also generic cachefiles, hopefully they could be addressed upstream.
> 

Thank you very much for your support and reply!

Thanks,
Zizhi Wo

> Thanks,
> Gao Xiang
>
>>
>> Therefore, I would greatly appreciate it if anyone could take some time
>> to review these patches. So friendly ping.
>>
>> Thanks,
>> Zizhi Wo
>>
>>
>> 在 2024/8/21 10:42, Zizhi Wo 写道:
>>> Hi!
>>>
>>> We recently discovered some bugs through self-discovery and testing in
>>> erofs ondemand loading mode, and this patchset is mainly used to fix
>>> them. These patches are relatively simple changes, and I would be 
>>> excited
>>> to discuss them together with everyone. Below is a brief introduction to
>>> each patch:
>>>
>>> Patch 1: Fix for wrong block_number calculated in ondemand write.
>>>
>>> Patch 2: Fix for wrong length return value in ondemand write.
>>>
>>> Patch 3: Fix missing position update in ondemand write, for scenarios
>>> involving read-ahead, invoking the write syscall.
>>>
>>> Patch 4: Previously, the last redundant data was cleared during the 
>>> umount
>>> phase. This patch remove unnecessary data in advance.
>>>
>>> Patch 5: Code clean up for cachefiles_commit_tmpfile().
>>>
>>> Patch 6: Modify error return value in cachefiles_daemon_secctx().
>>>
>>> Patch 7: Fix object->file Null-pointer-dereference problem.
>>>
>>> Patch 8: Fix for memory out of order in fscache_create_volume().
>>>
>>>
>>> Zizhi Wo (8):
>>>    cachefiles: Fix incorrect block calculations in
>>>      __cachefiles_prepare_write()
>>>    cachefiles: Fix incorrect length return value in
>>>      cachefiles_ondemand_fd_write_iter()
>>>    cachefiles: Fix missing pos updates in
>>>      cachefiles_ondemand_fd_write_iter()
>>>    cachefiles: Clear invalid cache data in advance
>>>    cachefiles: Clean up in cachefiles_commit_tmpfile()
>>>    cachefiles: Modify inappropriate error return value in
>>>      cachefiles_daemon_secctx()
>>>    cachefiles: Fix NULL pointer dereference in object->file
>>>    netfs/fscache: Add a memory barrier for FSCACHE_VOLUME_CREATING
>>>
>>>   fs/cachefiles/daemon.c    |  2 +-
>>>   fs/cachefiles/interface.c |  3 +++
>>>   fs/cachefiles/io.c        | 10 +++++-----
>>>   fs/cachefiles/namei.c     | 23 +++++++++++++----------
>>>   fs/cachefiles/ondemand.c  | 38 +++++++++++++++++++++++++++++---------
>>>   fs/netfs/fscache_volume.c |  3 +--
>>>   6 files changed, 52 insertions(+), 27 deletions(-)
>>>
> 

Re: [PATCH 1/8] cachefiles: Fix incorrect block calculations in __cachefiles_prepare_write()

[David Howells]

Zizhi Wo <wozizhi@huawei.com> wrote:

> In the __cachefiles_prepare_write function, DIO aligns blocks using
> PAGE_SIZE as the unit. And currently cachefiles_add_cache() binds
> cache->bsize with the requirement that it must not exceed PAGE_SIZE.
> However, if cache->bsize is smaller than PAGE_SIZE, the calculated block
> count will be incorrect in __cachefiles_prepare_write().
> 
> Set the block size to cache->bsize to resolve this issue.

Have you tested this with 9p, afs, cifs, ceph and/or nfs?  This may cause an
issue there as it assumed that the cache file will be padded out to
PAGE_SIZE (see cachefiles_adjust_size()).

David

Re: [PATCH 1/8] cachefiles: Fix incorrect block calculations in __cachefiles_prepare_write()

[Zizhi Wo]

在 2024/10/10 18:34, David Howells 写道:
> Zizhi Wo <wozizhi@huawei.com> wrote:
> 
>> In the __cachefiles_prepare_write function, DIO aligns blocks using
>> PAGE_SIZE as the unit. And currently cachefiles_add_cache() binds
>> cache->bsize with the requirement that it must not exceed PAGE_SIZE.
>> However, if cache->bsize is smaller than PAGE_SIZE, the calculated block
>> count will be incorrect in __cachefiles_prepare_write().
>>
>> Set the block size to cache->bsize to resolve this issue.
> 
> Have you tested this with 9p, afs, cifs, ceph and/or nfs?  This may cause an
> issue there as it assumed that the cache file will be padded out to
> PAGE_SIZE (see cachefiles_adjust_size()).
> 
> David
> 
> 

In my opinion, cachefiles_add_cache() will pass the corresponding size
to cache->bsize. For scenarios such as nfs/cifs, the corresponding bsize
is PAGE_SIZE aligned, which is fine. For scenarios where cache->bsize is
specified for non-PAGE_SIZE alignment (such as erofs on demand mode),
imposing PAGE_SIZE here can be problematic. So modify cache->bsize to be
more generic.

Thanks,
Zizhi Wo

Re: [PATCH 1/8] cachefiles: Fix incorrect block calculations in __cachefiles_prepare_write()

[David Howells]

Zizhi Wo <wozizhi@huawei.com> wrote:

> For scenarios such as nfs/cifs, the corresponding bsize is PAGE_SIZE aligned

cache->bsize is a property of the cache device, not the network filesystems
that might be making use of it (and it might be shared between multiple
volumes from multiple network filesystems, all of which could, in theory, have
different 'block sizes', inasmuch as network filesystems have block sizes).

David

Re: [PATCH 1/8] cachefiles: Fix incorrect block calculations in __cachefiles_prepare_write()

[Zizhi Wo]

在 2024/10/10 19:36, David Howells 写道:
> Zizhi Wo <wozizhi@huawei.com> wrote:
> 
>> For scenarios such as nfs/cifs, the corresponding bsize is PAGE_SIZE aligned
> 
> cache->bsize is a property of the cache device, not the network filesystems
> that might be making use of it (and it might be shared between multiple
> volumes from multiple network filesystems, all of which could, in theory, have
> different 'block sizes', inasmuch as network filesystems have block sizes).
> 
> David
> 
> 

Then I was wrong. Thank you for pointing it out. I'll be thinking about
new solutions for non-PAGE_SIZE scenarios.

Thanks,
Zizhi Wo

Re: [PATCH 1/8] cachefiles: Fix incorrect block calculations in __cachefiles_prepare_write()

[Zizhi Wo]

在 2024/10/10 19:36, David Howells 写道:
> Zizhi Wo <wozizhi@huawei.com> wrote:
> 
>> For scenarios such as nfs/cifs, the corresponding bsize is PAGE_SIZE aligned
> 
> cache->bsize is a property of the cache device, not the network filesystems
> that might be making use of it (and it might be shared between multiple
> volumes from multiple network filesystems, all of which could, in theory, have
> different 'block sizes', inasmuch as network filesystems have block sizes).
> 
> David
> 

Sorry, I might have a slightly different idea now. The EROFS process
based on fscache is similar to NFS and others, supporting only
PAGE_SIZE-granularity reads/writes, which is fine. The issue now lies in
the __cachefiles_prepare_write() function, specifically in the block
count calculation when calling cachefiles_has_space(). By default, the
block size used there is PAGE_SIZE.

If the block size of the underlying cache filesystem is not PAGE_SIZE
(for example, smaller than PAGE_SIZE), it leads to an incorrect
calculation of the required block count, which makes the system think
there is enough space when there might not be. This should be the same
problem for all network file systems that specify the back-end cache
file system mode.

For example, assume len=12k, pagesize=4k, and the underlying cache
filesystem block size is 1k. The currently available blocks are 4, with
4k of space remaining. However, cachefiles_has_space() calculates the
block count as 12k/4k=3, and since 4 > 3, it incorrectly returns that
there is enough space.

Therefore, I believe this could be a general issue. If
cachefiles_add_cache() is part of a common workflow, then using
cache->bsize here might be more appropriate? Looking forward to your
reply.

Thanks,
Zizhi Wo

> 

Re: [PATCH 4/8] cachefiles: Clear invalid cache data in advance

[David Howells]

Zizhi Wo <wozizhi@huawei.com> wrote:

> In the current on-demand loading scenario, when umount is called, the
> cachefiles_commit_tmpfile() is invoked. When checking the inode
> corresponding to object->file is inconsistent with the dentry,
> cachefiles_unlink() is called to perform cleanup to prevent invalid data
> from occupying space.
> 
> The above operation does not apply to the first mount, because the cache
> dentry generated by the first mount must be negative. Moreover, there is no
> need to clear it during the first umount because this part of the data may
> be reusable in the future. But the problem is that, the clean operation can
> currently only be called through cachefiles_withdraw_cookie(), in other
> words the redundant data does not cleaned until the second umount. This
> means that during the second mount, the old cache data generated from the
> first mount still occupies space. So if the user does not manually clean up
> the previous cache before the next mount, it may return insufficient space
> during the second mount phase.
> 
> This patch adds an additional cleanup process in the cachefiles_open_file()
> function. When the auxdata check fails, the remaining old cache data is no
> longer needed, the file and dentry corresponding to the object are also
> put. As there is no need to clear it until umount, we can directly clear it
> during the mount process.
> 
> Signed-off-by: Zizhi Wo <wozizhi@huawei.com>

Okay, I think this is reasonable as it's done from a worker thread.  I wonder
if instead, though, cachefiles_create_file() should be called and then linked
over the top:

	https://lore.kernel.org/all/cover.1580251857.git.osandov@fb.com/

though AT_LINK_REPLACE seemed to get stuck.

Note that we can't just truncate the file to nothing instead because I/O might
be in progress on it.

David

Re: [PATCH 5/8] cachefiles: Clean up in cachefiles_commit_tmpfile()

[David Howells]

Zizhi Wo <wozizhi@huawei.com> wrote:

> Currently, cachefiles_commit_tmpfile() will only be called if object->flags
> is set to CACHEFILES_OBJECT_USING_TMPFILE. Only cachefiles_create_file()
> and cachefiles_invalidate_cookie() set this flag. Both of these functions
> replace object->file with the new tmpfile, and both are called by
> fscache_cookie_state_machine(), so there are no concurrency issues.
> 
> So the equation "d_backing_inode(dentry) == file_inode(object->file)" in
> cachefiles_commit_tmpfile() will never hold true according to the above
> conditions. This patch removes this part of the redundant code and does not
> involve any other logical changes.
> 
> Signed-off-by: Zizhi Wo <wozizhi@huawei.com>

Yeah, that's reasonable - and if it did hold true, all we do is unlink and
relink it.

Acked-by: David Howells <dhowells@redhat.com>

Re: [PATCH 8/8] netfs/fscache: Add a memory barrier for FSCACHE_VOLUME_CREATING

[David Howells]

Zizhi Wo <wozizhi@huawei.com> wrote:

> In fscache_create_volume(), there is a missing memory barrier between the
> bit-clearing operation and the wake-up operation. This may cause a
> situation where, after a wake-up, the bit-clearing operation hasn't been
> detected yet, leading to an indefinite wait. The triggering process is as
> follows:
> ...
> By combining the clear and wake operations into clear_and_wake_up_bit() to
> fix this issue.
> 
> Fixes: bfa22da3ed65 ("fscache: Provide and use cache methods to lookup/create/free a volume")
> Signed-off-by: Zizhi Wo <wozizhi@huawei.com>

Acked-by: David Howells <dhowells@redhat.com>

Re: [PATCH 7/8] cachefiles: Fix NULL pointer dereference in object->file

[David Howells]

Zizhi Wo <wozizhi@huawei.com> wrote:

> +	spin_lock(&object->lock);
>  	if (object->file) {
>  		fput(object->file);
>  		object->file = NULL;
>  	}
> +	spin_unlock(&object->lock);

I would suggest stashing the file pointer in a local var and then doing the
fput() outside of the locks.

David

Re: [PATCH 7/8] cachefiles: Fix NULL pointer dereference in object->file

[Zizhi Wo]

在 2024/10/10 19:26, David Howells 写道:
> Zizhi Wo <wozizhi@huawei.com> wrote:
> 
>> +	spin_lock(&object->lock);
>>   	if (object->file) {
>>   		fput(object->file);
>>   		object->file = NULL;
>>   	}
>> +	spin_unlock(&object->lock);
> 
> I would suggest stashing the file pointer in a local var and then doing the
> fput() outside of the locks.
> 
> David
> 
> 

If fput() is executed outside the lock, I am currently unsure how to
guarantee that file in __cachefiles_write() does not trigger null
pointer dereference...

Thanks,
Zizhi Wo

Re: [PATCH 7/8] cachefiles: Fix NULL pointer dereference in object->file

[David Howells]

Zizhi Wo <wozizhi@huawei.com> wrote:

> 在 2024/10/10 19:26, David Howells 写道:
> > Zizhi Wo <wozizhi@huawei.com> wrote:
> > 
> >> +	spin_lock(&object->lock);
> >>   	if (object->file) {
> >>   		fput(object->file);
> >>   		object->file = NULL;
> >>   	}
> >> +	spin_unlock(&object->lock);
> > I would suggest stashing the file pointer in a local var and then doing the
> > fput() outside of the locks.
> > David
> > 
> 
> If fput() is executed outside the lock, I am currently unsure how to
> guarantee that file in __cachefiles_write() does not trigger null
> pointer dereference...

I'm not sure why there's a problem here.  I was thinking along the lines of:

	struct file *tmp;
	spin_lock(&object->lock);
 	tmp = object->file)
	object->file = NULL;
	spin_unlock(&object->lock);
	if (tmp)
		fput(tmp);

Note that fput() may defer the actual work if the counter hits zero, so the
cleanup may not happen inside the lock; further, the cleanup done by __fput()
may sleep.

David

Re: [PATCH 7/8] cachefiles: Fix NULL pointer dereference in object->file

[Zizhi Wo]

在 2024/10/10 22:52, David Howells 写道:
> Zizhi Wo <wozizhi@huawei.com> wrote:
> 
>> 在 2024/10/10 19:26, David Howells 写道:
>>> Zizhi Wo <wozizhi@huawei.com> wrote:
>>>
>>>> +	spin_lock(&object->lock);
>>>>    	if (object->file) {
>>>>    		fput(object->file);
>>>>    		object->file = NULL;
>>>>    	}
>>>> +	spin_unlock(&object->lock);
>>> I would suggest stashing the file pointer in a local var and then doing the
>>> fput() outside of the locks.
>>> David
>>>
>>
>> If fput() is executed outside the lock, I am currently unsure how to
>> guarantee that file in __cachefiles_write() does not trigger null
>> pointer dereference...
> 
> I'm not sure why there's a problem here.  I was thinking along the lines of:
> 
> 	struct file *tmp;
> 	spin_lock(&object->lock);
>   	tmp = object->file)
> 	object->file = NULL;
> 	spin_unlock(&object->lock);
> 	if (tmp)
> 		fput(tmp);
> 
> Note that fput() may defer the actual work if the counter hits zero, so the
> cleanup may not happen inside the lock; further, the cleanup done by __fput()
> may sleep.
> 
> David
> 
> 
Oh, I see what you mean. I will sort it out and issue the second patch
as soon as possible.

Thanks,
Zizhi Wo

Re: [PATCH 3/8] cachefiles: Fix missing pos updates in cachefiles_ondemand_fd_write_iter()

[David Howells]

Zizhi Wo <wozizhi@huawei.com> wrote:

> In the erofs on-demand loading scenario, read and write operations are
> usually delivered through "off" and "len" contained in read req in user
> mode. Naturally, pwrite is used to specify a specific offset to complete
> write operations.
> 
> However, if the write(not pwrite) syscall is called multiple times in the
> read-ahead scenario, we need to manually update ki_pos after each write
> operation to update file->f_pos.
> 
> This step is currently missing from the cachefiles_ondemand_fd_write_iter
> function, added to address this issue.
> 
> Fixes: c8383054506c ("cachefiles: notify the user daemon when looking up cookie")
> Signed-off-by: Zizhi Wo <wozizhi@huawei.com>

Acked-by: David Howells <dhowells@redhat.com>

Re: [PATCH 6/8] cachefiles: Modify inappropriate error return value in cachefiles_daemon_secctx()

[David Howells]

Zizhi Wo <wozizhi@huawei.com> wrote:

> In cachefiles_daemon_secctx(), if it is detected that secctx has been
> written to the cache, the error code returned is -EINVAL, which is
> inappropriate and does not distinguish the situation well.

I disagree: it is an invalid parameter, not an already extant file, and a
message is logged for clarification.  I'd prefer to avoid filesystem errors as
we are also doing filesystem operations.

David

Re: [PATCH 6/8] cachefiles: Modify inappropriate error return value in cachefiles_daemon_secctx()

[Zizhi Wo]

在 2024/10/10 19:31, David Howells 写道:
> Zizhi Wo <wozizhi@huawei.com> wrote:
> 
>> In cachefiles_daemon_secctx(), if it is detected that secctx has been
>> written to the cache, the error code returned is -EINVAL, which is
>> inappropriate and does not distinguish the situation well.
> 
> I disagree: it is an invalid parameter, not an already extant file, and a
> message is logged for clarification.  I'd prefer to avoid filesystem errors as
> we are also doing filesystem operations.
> 
> David
> 
> 

Alright, what I originally intended was to differentiate the error codes
between cases where no arguments are specified and where cache->secctx
already exists.

Thanks,
Zizhi Wo