diff mbox series

[v1,1/3] fs: add parameter use_fpos to iterate_dir function

Message ID 20211123181010.1607630-2-shr@fb.com (mailing list archive)
State New, archived
Headers show
Series io_uring: add getdents64 support | expand

Commit Message

Stefan Roesch Nov. 23, 2021, 6:10 p.m. UTC 
This adds the use_fpos parameter to the iterate_dir function.
If use_fpos is true it uses the file position in the file
structure (existing behavior). If use_fpos is false, it uses
the pos in the context structure.

This change is required to support getdents in io_uring.

Signed-off-by: Stefan Roesch <shr@fb.com>
---
 fs/exportfs/expfs.c    |  2 +-
 fs/nfsd/nfs4recover.c  |  2 +-
 fs/nfsd/vfs.c          |  2 +-
 fs/overlayfs/readdir.c |  6 +++---
 fs/readdir.c           | 28 ++++++++++++++++++++--------
 include/linux/fs.h     |  2 +-
 6 files changed, 27 insertions(+), 15 deletions(-)

Comments

kernel test robot Nov. 24, 2021, 7:58 p.m. UTC | #1 
Hi Stefan,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.16-rc2 next-20211124]
[cannot apply to mszeredi-vfs/overlayfs-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Stefan-Roesch/io_uring-add-getdents64-support/20211124-022809
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 136057256686de39cc3a07c2e39ef6bc43003ff6
config: x86_64-randconfig-r006-20211124 (https://download.01.org/0day-ci/archive/20211125/202111250356.yBAHK4KL-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 67a1c45def8a75061203461ab0060c75c864df1c)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/94fab53b56d471270b8b7b9afe6d73a8098448be
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Stefan-Roesch/io_uring-add-getdents64-support/20211124-022809
        git checkout 94fab53b56d471270b8b7b9afe6d73a8098448be
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash fs/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> fs/ksmbd/vfs.c:1139:47: error: too few arguments to function call, expected 3, have 2
           err = iterate_dir(fp->filp, &readdir_data.ctx);
                 ~~~~~~~~~~~                            ^
   include/linux/fs.h:3346:12: note: 'iterate_dir' declared here
   extern int iterate_dir(struct file *file, struct dir_context *ctx, bool use_fpos);
              ^
   fs/ksmbd/vfs.c:1189:44: error: too few arguments to function call, expected 3, have 2
           ret = iterate_dir(dfilp, &readdir_data.ctx);
                 ~~~~~~~~~~~                         ^
   include/linux/fs.h:3346:12: note: 'iterate_dir' declared here
   extern int iterate_dir(struct file *file, struct dir_context *ctx, bool use_fpos);
              ^
   2 errors generated.
--
>> fs/ksmbd/smb2pdu.c:3926:58: error: too few arguments to function call, expected 3, have 2
           rc = iterate_dir(dir_fp->filp, &dir_fp->readdir_data.ctx);
                ~~~~~~~~~~~                                        ^
   include/linux/fs.h:3346:12: note: 'iterate_dir' declared here
   extern int iterate_dir(struct file *file, struct dir_context *ctx, bool use_fpos);
              ^
   1 error generated.


vim +1139 fs/ksmbd/vfs.c

f44158485826c0 fs/cifsd/vfs.c Namjae Jeon 2021-03-16  1122  
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon 2021-03-16  1123  /**
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon 2021-03-16  1124   * ksmbd_vfs_empty_dir() - check for empty directory
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon 2021-03-16  1125   * @fp:	ksmbd file pointer
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon 2021-03-16  1126   *
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon 2021-03-16  1127   * Return:	true if directory empty, otherwise false
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon 2021-03-16  1128   */
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon 2021-03-16  1129  int ksmbd_vfs_empty_dir(struct ksmbd_file *fp)
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon 2021-03-16  1130  {
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon 2021-03-16  1131  	int err;
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon 2021-03-16  1132  	struct ksmbd_readdir_data readdir_data;
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon 2021-03-16  1133  
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon 2021-03-16  1134  	memset(&readdir_data, 0, sizeof(struct ksmbd_readdir_data));
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon 2021-03-16  1135  
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon 2021-03-16  1136  	set_ctx_actor(&readdir_data.ctx, __dir_empty);
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon 2021-03-16  1137  	readdir_data.dirent_count = 0;
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon 2021-03-16  1138  
e8c061917133dd fs/cifsd/vfs.c Namjae Jeon 2021-06-22 @1139  	err = iterate_dir(fp->filp, &readdir_data.ctx);
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon 2021-03-16  1140  	if (readdir_data.dirent_count > 2)
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon 2021-03-16  1141  		err = -ENOTEMPTY;
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon 2021-03-16  1142  	else
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon 2021-03-16  1143  		err = 0;
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon 2021-03-16  1144  	return err;
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon 2021-03-16  1145  }
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon 2021-03-16  1146  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Miklos Szeredi Nov. 25, 2021, 2:45 p.m. UTC | #2 
On Tue, 23 Nov 2021 at 19:10, Stefan Roesch <shr@fb.com> wrote:
>
> This adds the use_fpos parameter to the iterate_dir function.
> If use_fpos is true it uses the file position in the file
> structure (existing behavior). If use_fpos is false, it uses
> the pos in the context structure.

Is there a reason not to introduce a iterate_dir_no_fpos() variant and
call this from iterate_dir() with the necessary fpos update?

Thanks,
Miklos
Pavel Begunkov Nov. 25, 2021, 3:47 p.m. UTC | #3 
On 11/23/21 18:10, Stefan Roesch wrote:
> This adds the use_fpos parameter to the iterate_dir function.
> If use_fpos is true it uses the file position in the file
> structure (existing behavior). If use_fpos is false, it uses
> the pos in the context structure.

Looks sane, one question below

> 
> This change is required to support getdents in io_uring.
> 
> Signed-off-by: Stefan Roesch <shr@fb.com>
> ---
>   fs/exportfs/expfs.c    |  2 +-
>   fs/nfsd/nfs4recover.c  |  2 +-
>   fs/nfsd/vfs.c          |  2 +-
>   fs/overlayfs/readdir.c |  6 +++---
>   fs/readdir.c           | 28 ++++++++++++++++++++--------
>   include/linux/fs.h     |  2 +-
>   6 files changed, 27 insertions(+), 15 deletions(-)
> 
[...]
> diff --git a/fs/readdir.c b/fs/readdir.c
> index 09e8ed7d4161..8ea5b5f45a78 100644
> --- a/fs/readdir.c
> +++ b/fs/readdir.c
> @@ -21,6 +21,7 @@
>   #include <linux/unistd.h>
>   #include <linux/compat.h>
>   #include <linux/uaccess.h>
> +#include "internal.h"

Don't see this header is used in this patch. Just to be clear,
it is here only for 2/3, right?

[...]
Pavel Begunkov Nov. 25, 2021, 3:55 p.m. UTC | #4 
On 11/23/21 18:10, Stefan Roesch wrote:
> This adds the use_fpos parameter to the iterate_dir function.
> If use_fpos is true it uses the file position in the file
> structure (existing behavior). If use_fpos is false, it uses
> the pos in the context structure.
> 
> This change is required to support getdents in io_uring.
> 
> Signed-off-by: Stefan Roesch <shr@fb.com>
> ---
>   fs/exportfs/expfs.c    |  2 +-
>   fs/nfsd/nfs4recover.c  |  2 +-
>   fs/nfsd/vfs.c          |  2 +-
>   fs/overlayfs/readdir.c |  6 +++---
>   fs/readdir.c           | 28 ++++++++++++++++++++--------
>   include/linux/fs.h     |  2 +-
>   6 files changed, 27 insertions(+), 15 deletions(-)
> 
[...]
> diff --git a/fs/readdir.c b/fs/readdir.c
> index 09e8ed7d4161..8ea5b5f45a78 100644
> --- a/fs/readdir.c
> +++ b/fs/readdir.c
> @@ -21,6 +21,7 @@
>   #include <linux/unistd.h>
>   #include <linux/compat.h>
>   #include <linux/uaccess.h>
> +#include "internal.h"
>   
>   #include <asm/unaligned.h>
>   
> @@ -36,8 +37,14 @@
>   	unsafe_copy_to_user(dst, src, len, label);		\
>   } while (0)
>   
> -
> -int iterate_dir(struct file *file, struct dir_context *ctx)
> +/**
> + * iterate_dir - iterate over directory
> + * @file    : pointer to file struct of directory
> + * @ctx     : pointer to directory ctx structure
> + * @use_fpos: true : use file offset
> + *            false: use pos in ctx structure
> + */
> +int iterate_dir(struct file *file, struct dir_context *ctx, bool use_fpos)
>   {
>   	struct inode *inode = file_inode(file);
>   	bool shared = false;
> @@ -60,12 +67,17 @@ int iterate_dir(struct file *file, struct dir_context *ctx)
>   
>   	res = -ENOENT;
>   	if (!IS_DEADDIR(inode)) {
> -		ctx->pos = file->f_pos;
> +		if (use_fpos)
> +			ctx->pos = file->f_pos;

One more thing I haven't noticed before, should pos be sanitised
somehow if passed from the userspace? Do filesystems handle it
well?


> +
>   		if (shared)
>   			res = file->f_op->iterate_shared(file, ctx);
>   		else
>   			res = file->f_op->iterate(file, ctx);
> -		file->f_pos = ctx->pos;
> +
> +		if (use_fpos)
> +			file->f_pos = ctx->pos;
> +
>   		fsnotify_access(file);
>   		file_accessed(file);
>   	}
Stefan Roesch Nov. 25, 2021, 11:17 p.m. UTC | #5 
On 11/25/21 7:47 AM, Pavel Begunkov wrote:
> On 11/23/21 18:10, Stefan Roesch wrote:
>> This adds the use_fpos parameter to the iterate_dir function.
>> If use_fpos is true it uses the file position in the file
>> structure (existing behavior). If use_fpos is false, it uses
>> the pos in the context structure.
> 
> Looks sane, one question below
> 
>>
>> This change is required to support getdents in io_uring.
>>
>> Signed-off-by: Stefan Roesch <shr@fb.com>
>> ---
>>   fs/exportfs/expfs.c    |  2 +-
>>   fs/nfsd/nfs4recover.c  |  2 +-
>>   fs/nfsd/vfs.c          |  2 +-
>>   fs/overlayfs/readdir.c |  6 +++---
>>   fs/readdir.c           | 28 ++++++++++++++++++++--------
>>   include/linux/fs.h     |  2 +-
>>   6 files changed, 27 insertions(+), 15 deletions(-)
>>
> [...]
>> diff --git a/fs/readdir.c b/fs/readdir.c
>> index 09e8ed7d4161..8ea5b5f45a78 100644
>> --- a/fs/readdir.c
>> +++ b/fs/readdir.c
>> @@ -21,6 +21,7 @@
>>   #include <linux/unistd.h>
>>   #include <linux/compat.h>
>>   #include <linux/uaccess.h>
>> +#include "internal.h"
> 
> Don't see this header is used in this patch. Just to be clear,
> it is here only for 2/3, right?
>

This is not needed. It will be removed with the next version (v3)
of the patch.
 
> [...]
>
Stefan Roesch Nov. 25, 2021, 11:18 p.m. UTC | #6 
On 11/25/21 6:45 AM, Miklos Szeredi wrote:
> On Tue, 23 Nov 2021 at 19:10, Stefan Roesch <shr@fb.com> wrote:
>>
>> This adds the use_fpos parameter to the iterate_dir function.
>> If use_fpos is true it uses the file position in the file
>> structure (existing behavior). If use_fpos is false, it uses
>> the pos in the context structure.
> 
> Is there a reason not to introduce a iterate_dir_no_fpos() variant and
> call this from iterate_dir() with the necessary fpos update?
>

This would cause some code duplication. With the next version of the patch
I'll keep the current function of iterate_dir and introduce a new do_iterate_dir
function that has the use_fpos parameter. With that change the function
signature of iterate_dir does not change and iterate_dir will call do_iterate_dir.
 
> Thanks,
> Miklos
>
Stefan Roesch Nov. 25, 2021, 11:19 p.m. UTC | #7 
On 11/25/21 7:55 AM, Pavel Begunkov wrote:
> On 11/23/21 18:10, Stefan Roesch wrote:
>> This adds the use_fpos parameter to the iterate_dir function.
>> If use_fpos is true it uses the file position in the file
>> structure (existing behavior). If use_fpos is false, it uses
>> the pos in the context structure.
>>
>> This change is required to support getdents in io_uring.
>>
>> Signed-off-by: Stefan Roesch <shr@fb.com>
>> ---
>>   fs/exportfs/expfs.c    |  2 +-
>>   fs/nfsd/nfs4recover.c  |  2 +-
>>   fs/nfsd/vfs.c          |  2 +-
>>   fs/overlayfs/readdir.c |  6 +++---
>>   fs/readdir.c           | 28 ++++++++++++++++++++--------
>>   include/linux/fs.h     |  2 +-
>>   6 files changed, 27 insertions(+), 15 deletions(-)
>>
> [...]
>> diff --git a/fs/readdir.c b/fs/readdir.c
>> index 09e8ed7d4161..8ea5b5f45a78 100644
>> --- a/fs/readdir.c
>> +++ b/fs/readdir.c
>> @@ -21,6 +21,7 @@
>>   #include <linux/unistd.h>
>>   #include <linux/compat.h>
>>   #include <linux/uaccess.h>
>> +#include "internal.h"
>>     #include <asm/unaligned.h>
>>   @@ -36,8 +37,14 @@
>>       unsafe_copy_to_user(dst, src, len, label);        \
>>   } while (0)
>>   -
>> -int iterate_dir(struct file *file, struct dir_context *ctx)
>> +/**
>> + * iterate_dir - iterate over directory
>> + * @file    : pointer to file struct of directory
>> + * @ctx     : pointer to directory ctx structure
>> + * @use_fpos: true : use file offset
>> + *            false: use pos in ctx structure
>> + */
>> +int iterate_dir(struct file *file, struct dir_context *ctx, bool use_fpos)
>>   {
>>       struct inode *inode = file_inode(file);
>>       bool shared = false;
>> @@ -60,12 +67,17 @@ int iterate_dir(struct file *file, struct dir_context *ctx)
>>         res = -ENOENT;
>>       if (!IS_DEADDIR(inode)) {
>> -        ctx->pos = file->f_pos;
>> +        if (use_fpos)
>> +            ctx->pos = file->f_pos;
> 
> One more thing I haven't noticed before, should pos be sanitised
> somehow if passed from the userspace? Do filesystems handle it
> well?
> 

I checked a couple of filesystems and they all check that the pos value is reasonable.

> 
>> +
>>           if (shared)
>>               res = file->f_op->iterate_shared(file, ctx);
>>           else
>>               res = file->f_op->iterate(file, ctx);
>> -        file->f_pos = ctx->pos;
>> +
>> +        if (use_fpos)
>> +            file->f_pos = ctx->pos;
>> +
>>           fsnotify_access(file);
>>           file_accessed(file);
>>       }
> 
>
diff mbox series

Patch

diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
index 0106eba46d5a..0f303356f907 100644
--- a/fs/exportfs/expfs.c
+++ b/fs/exportfs/expfs.c
@@ -323,7 +323,7 @@  static int get_name(const struct path *path, char *name, struct dentry *child)
 	while (1) {
 		int old_seq = buffer.sequence;
 
-		error = iterate_dir(file, &buffer.ctx);
+		error = iterate_dir(file, &buffer.ctx, true);
 		if (buffer.found) {
 			error = 0;
 			break;
diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index 6fedc49726bf..013b1a3530c9 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -307,7 +307,7 @@  nfsd4_list_rec_dir(recdir_func *f, struct nfsd_net *nn)
 		return status;
 	}
 
-	status = iterate_dir(nn->rec_file, &ctx.ctx);
+	status = iterate_dir(nn->rec_file, &ctx.ctx, true);
 	inode_lock_nested(d_inode(dir), I_MUTEX_PARENT);
 
 	list_for_each_entry_safe(entry, tmp, &ctx.names, list) {
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index c99857689e2c..cd7a7d783fa7 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1980,7 +1980,7 @@  static __be32 nfsd_buffered_readdir(struct file *file, struct svc_fh *fhp,
 		buf.used = 0;
 		buf.full = 0;
 
-		host_err = iterate_dir(file, &buf.ctx);
+		host_err = iterate_dir(file, &buf.ctx, true);
 		if (buf.full)
 			host_err = 0;
 
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index 150fdf3bc68d..089150315942 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -306,7 +306,7 @@  static inline int ovl_dir_read(struct path *realpath,
 	do {
 		rdd->count = 0;
 		rdd->err = 0;
-		err = iterate_dir(realfile, &rdd->ctx);
+		err = iterate_dir(realfile, &rdd->ctx, true);
 		if (err >= 0)
 			err = rdd->err;
 	} while (!err && rdd->count);
@@ -722,7 +722,7 @@  static int ovl_iterate_real(struct file *file, struct dir_context *ctx)
 			return PTR_ERR(rdt.cache);
 	}
 
-	err = iterate_dir(od->realfile, &rdt.ctx);
+	err = iterate_dir(od->realfile, &rdt.ctx, true);
 	ctx->pos = rdt.ctx.pos;
 
 	return err;
@@ -753,7 +753,7 @@  static int ovl_iterate(struct file *file, struct dir_context *ctx)
 		      OVL_TYPE_MERGE(ovl_path_type(dentry->d_parent))))) {
 			err = ovl_iterate_real(file, ctx);
 		} else {
-			err = iterate_dir(od->realfile, ctx);
+			err = iterate_dir(od->realfile, ctx, true);
 		}
 		goto out;
 	}
diff --git a/fs/readdir.c b/fs/readdir.c
index 09e8ed7d4161..8ea5b5f45a78 100644
--- a/fs/readdir.c
+++ b/fs/readdir.c
@@ -21,6 +21,7 @@ 
 #include <linux/unistd.h>
 #include <linux/compat.h>
 #include <linux/uaccess.h>
+#include "internal.h"
 
 #include <asm/unaligned.h>
 
@@ -36,8 +37,14 @@ 
 	unsafe_copy_to_user(dst, src, len, label);		\
 } while (0)
 
-
-int iterate_dir(struct file *file, struct dir_context *ctx)
+/**
+ * iterate_dir - iterate over directory
+ * @file    : pointer to file struct of directory
+ * @ctx     : pointer to directory ctx structure
+ * @use_fpos: true : use file offset
+ *            false: use pos in ctx structure
+ */
+int iterate_dir(struct file *file, struct dir_context *ctx, bool use_fpos)
 {
 	struct inode *inode = file_inode(file);
 	bool shared = false;
@@ -60,12 +67,17 @@  int iterate_dir(struct file *file, struct dir_context *ctx)
 
 	res = -ENOENT;
 	if (!IS_DEADDIR(inode)) {
-		ctx->pos = file->f_pos;
+		if (use_fpos)
+			ctx->pos = file->f_pos;
+
 		if (shared)
 			res = file->f_op->iterate_shared(file, ctx);
 		else
 			res = file->f_op->iterate(file, ctx);
-		file->f_pos = ctx->pos;
+
+		if (use_fpos)
+			file->f_pos = ctx->pos;
+
 		fsnotify_access(file);
 		file_accessed(file);
 	}
@@ -190,7 +202,7 @@  SYSCALL_DEFINE3(old_readdir, unsigned int, fd,
 	if (!f.file)
 		return -EBADF;
 
-	error = iterate_dir(f.file, &buf.ctx);
+	error = iterate_dir(f.file, &buf.ctx, true);
 	if (buf.result)
 		error = buf.result;
 
@@ -283,7 +295,7 @@  SYSCALL_DEFINE3(getdents, unsigned int, fd,
 	if (!f.file)
 		return -EBADF;
 
-	error = iterate_dir(f.file, &buf.ctx);
+	error = iterate_dir(f.file, &buf.ctx, true);
 	if (error >= 0)
 		error = buf.error;
 	if (buf.prev_reclen) {
@@ -448,7 +460,7 @@  COMPAT_SYSCALL_DEFINE3(old_readdir, unsigned int, fd,
 	if (!f.file)
 		return -EBADF;
 
-	error = iterate_dir(f.file, &buf.ctx);
+	error = iterate_dir(f.file, &buf.ctx, true);
 	if (buf.result)
 		error = buf.result;
 
@@ -534,7 +546,7 @@  COMPAT_SYSCALL_DEFINE3(getdents, unsigned int, fd,
 	if (!f.file)
 		return -EBADF;
 
-	error = iterate_dir(f.file, &buf.ctx);
+	error = iterate_dir(f.file, &buf.ctx, true);
 	if (error >= 0)
 		error = buf.error;
 	if (buf.prev_reclen) {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 1cb616fc1105..ba4f49c4ac41 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3343,7 +3343,7 @@  const char *simple_get_link(struct dentry *, struct inode *,
 			    struct delayed_call *);
 extern const struct inode_operations simple_symlink_inode_operations;
 
-extern int iterate_dir(struct file *, struct dir_context *);
+extern int iterate_dir(struct file *file, struct dir_context *ctx, bool use_fpos);
 
 int vfs_fstatat(int dfd, const char __user *filename, struct kstat *stat,
 		int flags);