[PATCH RFC 00/20] file: remove f_version

[PATCH RFC 00/20] file: remove f_version

[Christian Brauner]

Probably not for this merge window anymore but let me get it out now:

The f_version member in struct file isn't particularly well-defined. It
is mainly used as a cookie to detect concurrent seeks when iterating
directories. But it is also abused by some subsystems for completely
unrelated things.

It is mostly a directory specific thing that doesn't really need to live
in struct file and with its wonky semantics it really lacks a specific
function.

For pipes, f_version is (ab)used to defer poll notifications until a
write has happened. And struct pipe_inode_info is used by multiple
struct files in their ->private_data so there's no chance of pushing
that down into file->private_data without introducing another pointer
indirection.

But this should be a solvable problem. Only regular files with
FMODE_ATOMIC_POS and directories require f_pos_lock. Pipes and other
files don't. So this adds a union into struct file encompassing
f_pos_lock and a pipe specific f_pipe member that pipes can use. This
union of course can be extended to other file types and is similar to
what we do in struct inode already.

I hope I got the details right. It at least holds up in xfstests and LTP
and my hand-rolled getdents-seek stressor.
  

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
Christian Brauner (20):
      file: remove pointless comment
      adi: remove unused f_version
      ceph: remove unused f_version
      s390: remove unused f_version
      fs: add vfs_setpos_cookie()
      fs: add must_set_pos()
      fs: use must_set_pos()
      fs: add generic_llseek_cookie()
      affs: store cookie in private data
      ext2: store cookie in private data
      ext4: store cookie in private data
      input: remove f_version abuse
      ocfs2: store cookie in private data
      proc: store cookie in private data
      udf: store cookie in private data
      ufs: store cookie in private data
      ubifs: store cookie in private data
      fs: add f_pipe
      pipe: use f_pipe
      fs: remove f_version

 drivers/char/adi.c             |   1 -
 drivers/input/input.c          |   8 +-
 drivers/s390/char/hmcdrv_dev.c |   3 -
 fs/affs/dir.c                  |  44 +++++++++--
 fs/ceph/dir.c                  |   1 -
 fs/ext2/dir.c                  |  28 ++++++-
 fs/ext4/dir.c                  |  50 ++++++------
 fs/ext4/ext4.h                 |   2 +
 fs/ext4/inline.c               |   7 +-
 fs/file_table.c                |   1 -
 fs/ocfs2/dir.c                 |   3 +-
 fs/ocfs2/file.c                |  11 ++-
 fs/ocfs2/file.h                |   1 +
 fs/pipe.c                      |   6 +-
 fs/proc/base.c                 |  18 +++--
 fs/read_write.c                | 169 +++++++++++++++++++++++++++++++----------
 fs/ubifs/dir.c                 |  65 +++++++++++-----
 fs/udf/dir.c                   |  28 ++++++-
 fs/ufs/dir.c                   |  28 ++++++-
 include/linux/fs.h             |  14 +++-
 20 files changed, 366 insertions(+), 122 deletions(-)
---
base-commit: 79868ff5ce9156228f9aef351d8bf76fb8540230
change-id: 20240829-vfs-file-f_version-66321e3dafeb

[PATCH RFC 01/20] file: remove pointless comment

[Christian Brauner]

There's really no need to mention f_version.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/file_table.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/file_table.c b/fs/file_table.c
index 3ef558f27a1c..bf1cbe47c93d 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -159,7 +159,6 @@ static int init_file(struct file *f, int flags, const struct cred *cred)
 	mutex_init(&f->f_pos_lock);
 	f->f_flags = flags;
 	f->f_mode = OPEN_FMODE(flags);
-	/* f->f_version: 0 */
 
 	/*
 	 * We're SLAB_TYPESAFE_BY_RCU so initialize f_count last. While

-- 
2.45.2

Re: [PATCH RFC 01/20] file: remove pointless comment

[Jan Kara]

On Fri 30-08-24 15:04:42, Christian Brauner wrote:
> There's really no need to mention f_version.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/file_table.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/file_table.c b/fs/file_table.c
> index 3ef558f27a1c..bf1cbe47c93d 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -159,7 +159,6 @@ static int init_file(struct file *f, int flags, const struct cred *cred)
>  	mutex_init(&f->f_pos_lock);
>  	f->f_flags = flags;
>  	f->f_mode = OPEN_FMODE(flags);
> -	/* f->f_version: 0 */
>  
>  	/*
>  	 * We're SLAB_TYPESAFE_BY_RCU so initialize f_count last. While
> 
> -- 
> 2.45.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RFC 02/20] adi: remove unused f_version

[Christian Brauner]

It's not used for adi so don't bother with it at all.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 drivers/char/adi.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/char/adi.c b/drivers/char/adi.c
index 751d7cc0da1b..c091a0282ad0 100644
--- a/drivers/char/adi.c
+++ b/drivers/char/adi.c
@@ -196,7 +196,6 @@ static loff_t adi_llseek(struct file *file, loff_t offset, int whence)
 
 	if (offset != file->f_pos) {
 		file->f_pos = offset;
-		file->f_version = 0;
 		ret = offset;
 	}
 

-- 
2.45.2

Re: [PATCH RFC 02/20] adi: remove unused f_version

[Jan Kara]

On Fri 30-08-24 15:04:43, Christian Brauner wrote:
> It's not used for adi so don't bother with it at all.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Furthermore we allocate new files with kzalloc. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  drivers/char/adi.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/char/adi.c b/drivers/char/adi.c
> index 751d7cc0da1b..c091a0282ad0 100644
> --- a/drivers/char/adi.c
> +++ b/drivers/char/adi.c
> @@ -196,7 +196,6 @@ static loff_t adi_llseek(struct file *file, loff_t offset, int whence)
>  
>  	if (offset != file->f_pos) {
>  		file->f_pos = offset;
> -		file->f_version = 0;
>  		ret = offset;
>  	}
>  
> 
> -- 
> 2.45.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RFC 03/20] ceph: remove unused f_version

[Christian Brauner]

It's not used for ceph so don't bother with it at all.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/ceph/dir.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 18c72b305858..ddec8c9244ee 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -707,7 +707,6 @@ static loff_t ceph_dir_llseek(struct file *file, loff_t offset, int whence)
 
 		if (offset != file->f_pos) {
 			file->f_pos = offset;
-			file->f_version = 0;
 			dfi->file_info.flags &= ~CEPH_F_ATEND;
 		}
 		retval = offset;

-- 
2.45.2

Re: [PATCH RFC 03/20] ceph: remove unused f_version

[Jan Kara]

On Fri 30-08-24 15:04:44, Christian Brauner wrote:
> It's not used for ceph so don't bother with it at all.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ceph/dir.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index 18c72b305858..ddec8c9244ee 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -707,7 +707,6 @@ static loff_t ceph_dir_llseek(struct file *file, loff_t offset, int whence)
>  
>  		if (offset != file->f_pos) {
>  			file->f_pos = offset;
> -			file->f_version = 0;
>  			dfi->file_info.flags &= ~CEPH_F_ATEND;
>  		}
>  		retval = offset;
> 
> -- 
> 2.45.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RFC 04/20] s390: remove unused f_version

[Christian Brauner]

It's not used so don't bother with it at all.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 drivers/s390/char/hmcdrv_dev.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/s390/char/hmcdrv_dev.c b/drivers/s390/char/hmcdrv_dev.c
index 8d50c894711f..e069dd685899 100644
--- a/drivers/s390/char/hmcdrv_dev.c
+++ b/drivers/s390/char/hmcdrv_dev.c
@@ -186,9 +186,6 @@ static loff_t hmcdrv_dev_seek(struct file *fp, loff_t pos, int whence)
 	if (pos < 0)
 		return -EINVAL;
 
-	if (fp->f_pos != pos)
-		++fp->f_version;
-
 	fp->f_pos = pos;
 	return pos;
 }

-- 
2.45.2

Re: [PATCH RFC 04/20] s390: remove unused f_version

[Jan Kara]

On Fri 30-08-24 15:04:45, Christian Brauner wrote:
> It's not used so don't bother with it at all.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  drivers/s390/char/hmcdrv_dev.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/s390/char/hmcdrv_dev.c b/drivers/s390/char/hmcdrv_dev.c
> index 8d50c894711f..e069dd685899 100644
> --- a/drivers/s390/char/hmcdrv_dev.c
> +++ b/drivers/s390/char/hmcdrv_dev.c
> @@ -186,9 +186,6 @@ static loff_t hmcdrv_dev_seek(struct file *fp, loff_t pos, int whence)
>  	if (pos < 0)
>  		return -EINVAL;
>  
> -	if (fp->f_pos != pos)
> -		++fp->f_version;
> -
>  	fp->f_pos = pos;
>  	return pos;
>  }
> 
> -- 
> 2.45.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RFC 05/20] fs: add vfs_setpos_cookie()

[Christian Brauner]

Add a new helper and make vfs_setpos() call it. We will use it in
follow-up patches.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/read_write.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index 90e283b31ca1..66ff52860496 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -40,18 +40,20 @@ static inline bool unsigned_offsets(struct file *file)
 }
 
 /**
- * vfs_setpos - update the file offset for lseek
+ * vfs_setpos_cookie - update the file offset for lseek and reset cookie
  * @file:	file structure in question
  * @offset:	file offset to seek to
  * @maxsize:	maximum file size
+ * @cookie:	cookie to reset
  *
- * This is a low-level filesystem helper for updating the file offset to
- * the value specified by @offset if the given offset is valid and it is
- * not equal to the current file offset.
+ * Update the file offset to the value specified by @offset if the given
+ * offset is valid and it is not equal to the current file offset and
+ * reset the specified cookie to indicate that a seek happened.
  *
  * Return the specified offset on success and -EINVAL on invalid offset.
  */
-loff_t vfs_setpos(struct file *file, loff_t offset, loff_t maxsize)
+static loff_t vfs_setpos_cookie(struct file *file, loff_t offset,
+				loff_t maxsize, u64 *cookie)
 {
 	if (offset < 0 && !unsigned_offsets(file))
 		return -EINVAL;
@@ -60,10 +62,27 @@ loff_t vfs_setpos(struct file *file, loff_t offset, loff_t maxsize)
 
 	if (offset != file->f_pos) {
 		file->f_pos = offset;
-		file->f_version = 0;
+		*cookie = 0;
 	}
 	return offset;
 }
+
+/**
+ * vfs_setpos - update the file offset for lseek
+ * @file:	file structure in question
+ * @offset:	file offset to seek to
+ * @maxsize:	maximum file size
+ *
+ * This is a low-level filesystem helper for updating the file offset to
+ * the value specified by @offset if the given offset is valid and it is
+ * not equal to the current file offset.
+ *
+ * Return the specified offset on success and -EINVAL on invalid offset.
+ */
+loff_t vfs_setpos(struct file *file, loff_t offset, loff_t maxsize)
+{
+	return vfs_setpos_cookie(file, offset, maxsize, &file->f_version);
+}
 EXPORT_SYMBOL(vfs_setpos);
 
 /**

-- 
2.45.2

Re: [PATCH RFC 05/20] fs: add vfs_setpos_cookie()

[Jan Kara]

On Fri 30-08-24 15:04:46, Christian Brauner wrote:
> Add a new helper and make vfs_setpos() call it. We will use it in
> follow-up patches.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/read_write.c | 31 +++++++++++++++++++++++++------
>  1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 90e283b31ca1..66ff52860496 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -40,18 +40,20 @@ static inline bool unsigned_offsets(struct file *file)
>  }
>  
>  /**
> - * vfs_setpos - update the file offset for lseek
> + * vfs_setpos_cookie - update the file offset for lseek and reset cookie
>   * @file:	file structure in question
>   * @offset:	file offset to seek to
>   * @maxsize:	maximum file size
> + * @cookie:	cookie to reset
>   *
> - * This is a low-level filesystem helper for updating the file offset to
> - * the value specified by @offset if the given offset is valid and it is
> - * not equal to the current file offset.
> + * Update the file offset to the value specified by @offset if the given
> + * offset is valid and it is not equal to the current file offset and
> + * reset the specified cookie to indicate that a seek happened.
>   *
>   * Return the specified offset on success and -EINVAL on invalid offset.
>   */
> -loff_t vfs_setpos(struct file *file, loff_t offset, loff_t maxsize)
> +static loff_t vfs_setpos_cookie(struct file *file, loff_t offset,
> +				loff_t maxsize, u64 *cookie)
>  {
>  	if (offset < 0 && !unsigned_offsets(file))
>  		return -EINVAL;
> @@ -60,10 +62,27 @@ loff_t vfs_setpos(struct file *file, loff_t offset, loff_t maxsize)
>  
>  	if (offset != file->f_pos) {
>  		file->f_pos = offset;
> -		file->f_version = 0;
> +		*cookie = 0;
>  	}
>  	return offset;
>  }
> +
> +/**
> + * vfs_setpos - update the file offset for lseek
> + * @file:	file structure in question
> + * @offset:	file offset to seek to
> + * @maxsize:	maximum file size
> + *
> + * This is a low-level filesystem helper for updating the file offset to
> + * the value specified by @offset if the given offset is valid and it is
> + * not equal to the current file offset.
> + *
> + * Return the specified offset on success and -EINVAL on invalid offset.
> + */
> +loff_t vfs_setpos(struct file *file, loff_t offset, loff_t maxsize)
> +{
> +	return vfs_setpos_cookie(file, offset, maxsize, &file->f_version);
> +}
>  EXPORT_SYMBOL(vfs_setpos);
>  
>  /**
> 
> -- 
> 2.45.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RFC 06/20] fs: add must_set_pos()

[Christian Brauner]

Add a new must_set_pos() helper. We will use it in follow-up patches.
Temporarily mark it as unused. This is only done to keep the diff small
and reviewable.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/read_write.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/fs/read_write.c b/fs/read_write.c
index 66ff52860496..acee26989d95 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -85,6 +85,58 @@ loff_t vfs_setpos(struct file *file, loff_t offset, loff_t maxsize)
 }
 EXPORT_SYMBOL(vfs_setpos);
 
+/**
+ * must_set_pos - check whether f_pos has to be updated
+ * @offset: offset to use
+ * @whence: type of seek operation
+ * @eof: end of file
+ *
+ * Check whether f_pos needs to be updated and update @offset according
+ * to @whence.
+ *
+ * Return: 0 if f_pos doesn't need to be updated, 1 if f_pos has to be
+ * updated, and negative error code on failure.
+ */
+static __maybe_unused int must_set_pos(struct file *file, loff_t *offset, int whence, loff_t eof)
+{
+	switch (whence) {
+	case SEEK_END:
+		*offset += eof;
+		break;
+	case SEEK_CUR:
+		/*
+		 * Here we special-case the lseek(fd, 0, SEEK_CUR)
+		 * position-querying operation.  Avoid rewriting the "same"
+		 * f_pos value back to the file because a concurrent read(),
+		 * write() or lseek() might have altered it
+		 */
+		if (*offset == 0) {
+			*offset = file->f_pos;
+			return 0;
+		}
+		break;
+	case SEEK_DATA:
+		/*
+		 * In the generic case the entire file is data, so as long as
+		 * offset isn't at the end of the file then the offset is data.
+		 */
+		if ((unsigned long long)*offset >= eof)
+			return -ENXIO;
+		break;
+	case SEEK_HOLE:
+		/*
+		 * There is a virtual hole at the end of the file, so as long as
+		 * offset isn't i_size or larger, return i_size.
+		 */
+		if ((unsigned long long)*offset >= eof)
+			return -ENXIO;
+		*offset = eof;
+		break;
+	}
+
+	return 1;
+}
+
 /**
  * generic_file_llseek_size - generic llseek implementation for regular files
  * @file:	file structure to seek on

-- 
2.45.2

Re: [PATCH RFC 06/20] fs: add must_set_pos()

[Jan Kara]

On Fri 30-08-24 15:04:47, Christian Brauner wrote:
> Add a new must_set_pos() helper. We will use it in follow-up patches.
> Temporarily mark it as unused. This is only done to keep the diff small
> and reviewable.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/read_write.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
> 
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 66ff52860496..acee26989d95 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -85,6 +85,58 @@ loff_t vfs_setpos(struct file *file, loff_t offset, loff_t maxsize)
>  }
>  EXPORT_SYMBOL(vfs_setpos);
>  
> +/**
> + * must_set_pos - check whether f_pos has to be updated
> + * @offset: offset to use
> + * @whence: type of seek operation
> + * @eof: end of file
> + *
> + * Check whether f_pos needs to be updated and update @offset according
> + * to @whence.
> + *
> + * Return: 0 if f_pos doesn't need to be updated, 1 if f_pos has to be
> + * updated, and negative error code on failure.
> + */
> +static __maybe_unused int must_set_pos(struct file *file, loff_t *offset, int whence, loff_t eof)
> +{
> +	switch (whence) {
> +	case SEEK_END:
> +		*offset += eof;
> +		break;
> +	case SEEK_CUR:
> +		/*
> +		 * Here we special-case the lseek(fd, 0, SEEK_CUR)
> +		 * position-querying operation.  Avoid rewriting the "same"
> +		 * f_pos value back to the file because a concurrent read(),
> +		 * write() or lseek() might have altered it
> +		 */
> +		if (*offset == 0) {
> +			*offset = file->f_pos;
> +			return 0;
> +		}
> +		break;
> +	case SEEK_DATA:
> +		/*
> +		 * In the generic case the entire file is data, so as long as
> +		 * offset isn't at the end of the file then the offset is data.
> +		 */
> +		if ((unsigned long long)*offset >= eof)
> +			return -ENXIO;
> +		break;
> +	case SEEK_HOLE:
> +		/*
> +		 * There is a virtual hole at the end of the file, so as long as
> +		 * offset isn't i_size or larger, return i_size.
> +		 */
> +		if ((unsigned long long)*offset >= eof)
> +			return -ENXIO;
> +		*offset = eof;
> +		break;
> +	}
> +
> +	return 1;
> +}
> +
>  /**
>   * generic_file_llseek_size - generic llseek implementation for regular files
>   * @file:	file structure to seek on
> 
> -- 
> 2.45.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RFC 07/20] fs: use must_set_pos()

[Christian Brauner]

Make generic_file_llseek_size() use must_set_pos(). We'll use
must_set_pos() in another helper in a minutes. Remove __maybe_unused
from must_set_pos() now that it's used.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/read_write.c | 51 ++++++++++++++-------------------------------------
 1 file changed, 14 insertions(+), 37 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index acee26989d95..ad93b72cc378 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -97,7 +97,7 @@ EXPORT_SYMBOL(vfs_setpos);
  * Return: 0 if f_pos doesn't need to be updated, 1 if f_pos has to be
  * updated, and negative error code on failure.
  */
-static __maybe_unused int must_set_pos(struct file *file, loff_t *offset, int whence, loff_t eof)
+static int must_set_pos(struct file *file, loff_t *offset, int whence, loff_t eof)
 {
 	switch (whence) {
 	case SEEK_END:
@@ -157,45 +157,22 @@ loff_t
 generic_file_llseek_size(struct file *file, loff_t offset, int whence,
 		loff_t maxsize, loff_t eof)
 {
-	switch (whence) {
-	case SEEK_END:
-		offset += eof;
-		break;
-	case SEEK_CUR:
-		/*
-		 * Here we special-case the lseek(fd, 0, SEEK_CUR)
-		 * position-querying operation.  Avoid rewriting the "same"
-		 * f_pos value back to the file because a concurrent read(),
-		 * write() or lseek() might have altered it
-		 */
-		if (offset == 0)
-			return file->f_pos;
-		/*
-		 * f_lock protects against read/modify/write race with other
-		 * SEEK_CURs. Note that parallel writes and reads behave
-		 * like SEEK_SET.
-		 */
-		spin_lock(&file->f_lock);
-		offset = vfs_setpos(file, file->f_pos + offset, maxsize);
-		spin_unlock(&file->f_lock);
+	int ret;
+
+	ret = must_set_pos(file, &offset, whence, eof);
+	if (ret < 0)
+		return ret;
+	if (ret == 0)
 		return offset;
-	case SEEK_DATA:
-		/*
-		 * In the generic case the entire file is data, so as long as
-		 * offset isn't at the end of the file then the offset is data.
-		 */
-		if ((unsigned long long)offset >= eof)
-			return -ENXIO;
-		break;
-	case SEEK_HOLE:
+
+	if (whence == SEEK_CUR) {
 		/*
-		 * There is a virtual hole at the end of the file, so as long as
-		 * offset isn't i_size or larger, return i_size.
+		 * f_lock protects against read/modify/write race with
+		 * other SEEK_CURs. Note that parallel writes and reads
+		 * behave like SEEK_SET.
 		 */
-		if ((unsigned long long)offset >= eof)
-			return -ENXIO;
-		offset = eof;
-		break;
+		guard(spinlock)(&file->f_lock);
+		return vfs_setpos(file, file->f_pos + offset, maxsize);
 	}
 
 	return vfs_setpos(file, offset, maxsize);

-- 
2.45.2

Re: [PATCH RFC 07/20] fs: use must_set_pos()

[Jan Kara]

On Fri 30-08-24 15:04:48, Christian Brauner wrote:
> Make generic_file_llseek_size() use must_set_pos(). We'll use
> must_set_pos() in another helper in a minutes. Remove __maybe_unused
> from must_set_pos() now that it's used.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Frankly, it would have been a bit easier to review for me if 6 & 7 patches
were together as one code refactoring patch...

> +		guard(spinlock)(&file->f_lock);

You really love guards, don't you? :) Frankly, in this case I don't see the
point and it makes my visual pattern matching fail but I guess I'll get
used to it. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

> +		return vfs_setpos(file, file->f_pos + offset, maxsize);
>  	}
>  
>  	return vfs_setpos(file, offset, maxsize);

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH RFC 07/20] fs: use must_set_pos()

[Christian Brauner]

On Tue, Sep 03, 2024 at 01:30:10PM GMT, Jan Kara wrote:
> On Fri 30-08-24 15:04:48, Christian Brauner wrote:
> > Make generic_file_llseek_size() use must_set_pos(). We'll use
> > must_set_pos() in another helper in a minutes. Remove __maybe_unused
> > from must_set_pos() now that it's used.
> > 
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> 
> Frankly, it would have been a bit easier to review for me if 6 & 7 patches
> were together as one code refactoring patch...

Yeah, I had it that way but the resulting diff was really difficult to read.
I could've probably tried to use a different diff algorithm but this way
was easier.

> 
> > +		guard(spinlock)(&file->f_lock);
> 
> You really love guards, don't you? :) Frankly, in this case I don't see the

Yes. :)

> point and it makes my visual pattern matching fail but I guess I'll get
> used to it. Feel free to add:

I can remove it. I don't mind.

[PATCH RFC 08/20] fs: add generic_llseek_cookie()

[Christian Brauner]

This is similar to generic_file_llseek() but allows the caller to
specify a cookie that will be updated to indicate that a seek happened.
Caller's requiring that information in their readdir implementations can
use that.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/read_write.c    | 44 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h |  2 ++
 2 files changed, 46 insertions(+)

diff --git a/fs/read_write.c b/fs/read_write.c
index ad93b72cc378..47f7b4e32a53 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -179,6 +179,50 @@ generic_file_llseek_size(struct file *file, loff_t offset, int whence,
 }
 EXPORT_SYMBOL(generic_file_llseek_size);
 
+/**
+ * generic_llseek_cookie - versioned llseek implementation
+ * @file:	file structure to seek on
+ * @offset:	file offset to seek to
+ * @whence:	type of seek
+ * @cookie:	cookie to update
+ *
+ * See generic_file_llseek for a general description and locking assumptions.
+ *
+ * In contrast to generic_file_llseek, this function also resets a
+ * specified cookie to indicate a seek took place.
+ */
+loff_t generic_llseek_cookie(struct file *file, loff_t offset, int whence,
+			     u64 *cookie)
+{
+	struct inode *inode = file->f_mapping->host;
+	loff_t maxsize = inode->i_sb->s_maxbytes;
+	loff_t eof = i_size_read(inode);
+	int ret;
+
+	if (WARN_ON_ONCE(!cookie))
+		return -EINVAL;
+
+	ret = must_set_pos(file, &offset, whence, eof);
+	if (ret < 0)
+		return ret;
+	if (ret == 0)
+		return offset;
+
+	if (whence == SEEK_CUR) {
+		/*
+		 * f_lock protects against read/modify/write race with
+		 * other SEEK_CURs. Note that parallel writes and reads
+		 * behave like SEEK_SET.
+		 */
+		guard(spinlock)(&file->f_lock);
+		return vfs_setpos_cookie(file, file->f_pos + offset, maxsize,
+					 cookie);
+	}
+
+	return vfs_setpos_cookie(file, offset, maxsize, cookie);
+}
+EXPORT_SYMBOL(generic_llseek_cookie);
+
 /**
  * generic_file_llseek - generic llseek implementation for regular files
  * @file:	file structure to seek on
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 58c91a52cad1..3e6b3c1afb31 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3202,6 +3202,8 @@ extern loff_t vfs_setpos(struct file *file, loff_t offset, loff_t maxsize);
 extern loff_t generic_file_llseek(struct file *file, loff_t offset, int whence);
 extern loff_t generic_file_llseek_size(struct file *file, loff_t offset,
 		int whence, loff_t maxsize, loff_t eof);
+loff_t generic_llseek_cookie(struct file *file, loff_t offset, int whence,
+			     u64 *cookie);
 extern loff_t fixed_size_llseek(struct file *file, loff_t offset,
 		int whence, loff_t size);
 extern loff_t no_seek_end_llseek_size(struct file *, loff_t, int, loff_t);

-- 
2.45.2

Re: [PATCH RFC 08/20] fs: add generic_llseek_cookie()

[Jan Kara]

On Fri 30-08-24 15:04:49, Christian Brauner wrote:
> This is similar to generic_file_llseek() but allows the caller to
> specify a cookie that will be updated to indicate that a seek happened.
> Caller's requiring that information in their readdir implementations can
> use that.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/read_write.c    | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/fs.h |  2 ++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/fs/read_write.c b/fs/read_write.c
> index ad93b72cc378..47f7b4e32a53 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -179,6 +179,50 @@ generic_file_llseek_size(struct file *file, loff_t offset, int whence,
>  }
>  EXPORT_SYMBOL(generic_file_llseek_size);
>  
> +/**
> + * generic_llseek_cookie - versioned llseek implementation
> + * @file:	file structure to seek on
> + * @offset:	file offset to seek to
> + * @whence:	type of seek
> + * @cookie:	cookie to update
> + *
> + * See generic_file_llseek for a general description and locking assumptions.
> + *
> + * In contrast to generic_file_llseek, this function also resets a
> + * specified cookie to indicate a seek took place.
> + */
> +loff_t generic_llseek_cookie(struct file *file, loff_t offset, int whence,
> +			     u64 *cookie)
> +{
> +	struct inode *inode = file->f_mapping->host;
> +	loff_t maxsize = inode->i_sb->s_maxbytes;
> +	loff_t eof = i_size_read(inode);
> +	int ret;
> +
> +	if (WARN_ON_ONCE(!cookie))
> +		return -EINVAL;
> +
> +	ret = must_set_pos(file, &offset, whence, eof);
> +	if (ret < 0)
> +		return ret;
> +	if (ret == 0)
> +		return offset;
> +
> +	if (whence == SEEK_CUR) {
> +		/*
> +		 * f_lock protects against read/modify/write race with
> +		 * other SEEK_CURs. Note that parallel writes and reads
> +		 * behave like SEEK_SET.
> +		 */
> +		guard(spinlock)(&file->f_lock);
> +		return vfs_setpos_cookie(file, file->f_pos + offset, maxsize,
> +					 cookie);
> +	}
> +
> +	return vfs_setpos_cookie(file, offset, maxsize, cookie);
> +}
> +EXPORT_SYMBOL(generic_llseek_cookie);
> +
>  /**
>   * generic_file_llseek - generic llseek implementation for regular files
>   * @file:	file structure to seek on
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 58c91a52cad1..3e6b3c1afb31 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3202,6 +3202,8 @@ extern loff_t vfs_setpos(struct file *file, loff_t offset, loff_t maxsize);
>  extern loff_t generic_file_llseek(struct file *file, loff_t offset, int whence);
>  extern loff_t generic_file_llseek_size(struct file *file, loff_t offset,
>  		int whence, loff_t maxsize, loff_t eof);
> +loff_t generic_llseek_cookie(struct file *file, loff_t offset, int whence,
> +			     u64 *cookie);
>  extern loff_t fixed_size_llseek(struct file *file, loff_t offset,
>  		int whence, loff_t size);
>  extern loff_t no_seek_end_llseek_size(struct file *, loff_t, int, loff_t);
> 
> -- 
> 2.45.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RFC 09/20] affs: store cookie in private data

[Christian Brauner]

Store the cookie to detect concurrent seeks on directories in
file->private_data.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/affs/dir.c | 44 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 38 insertions(+), 6 deletions(-)

diff --git a/fs/affs/dir.c b/fs/affs/dir.c
index b2bf7016e1b3..bd40d5f08810 100644
--- a/fs/affs/dir.c
+++ b/fs/affs/dir.c
@@ -17,13 +17,44 @@
 #include <linux/iversion.h>
 #include "affs.h"
 
+struct affs_dir_data {
+	unsigned long ino;
+	u64 cookie;
+};
+
 static int affs_readdir(struct file *, struct dir_context *);
 
+static loff_t affs_dir_llseek(struct file *file, loff_t offset, int whence)
+{
+	struct affs_dir_data *data = file->private_data;
+
+	return generic_llseek_cookie(file, offset, whence, &data->cookie);
+}
+
+static int affs_dir_open(struct inode *inode, struct file *file)
+{
+	struct affs_dir_data	*data;
+
+	data = kzalloc(sizeof(struct affs_dir_data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+	file->private_data = data;
+	return 0;
+}
+
+static int affs_dir_release(struct inode *inode, struct file *file)
+{
+	kfree(file->private_data);
+	return 0;
+}
+
 const struct file_operations affs_dir_operations = {
+	.open		= affs_dir_open,
 	.read		= generic_read_dir,
-	.llseek		= generic_file_llseek,
+	.llseek		= affs_dir_llseek,
 	.iterate_shared	= affs_readdir,
 	.fsync		= affs_file_fsync,
+	.release	= affs_dir_release,
 };
 
 /*
@@ -45,6 +76,7 @@ static int
 affs_readdir(struct file *file, struct dir_context *ctx)
 {
 	struct inode		*inode = file_inode(file);
+	struct affs_dir_data	*data = file->private_data;
 	struct super_block	*sb = inode->i_sb;
 	struct buffer_head	*dir_bh = NULL;
 	struct buffer_head	*fh_bh = NULL;
@@ -59,7 +91,7 @@ affs_readdir(struct file *file, struct dir_context *ctx)
 	pr_debug("%s(ino=%lu,f_pos=%llx)\n", __func__, inode->i_ino, ctx->pos);
 
 	if (ctx->pos < 2) {
-		file->private_data = (void *)0;
+		data->ino = 0;
 		if (!dir_emit_dots(file, ctx))
 			return 0;
 	}
@@ -80,8 +112,8 @@ affs_readdir(struct file *file, struct dir_context *ctx)
 	/* If the directory hasn't changed since the last call to readdir(),
 	 * we can jump directly to where we left off.
 	 */
-	ino = (u32)(long)file->private_data;
-	if (ino && inode_eq_iversion(inode, file->f_version)) {
+	ino = data->ino;
+	if (ino && inode_eq_iversion(inode, data->cookie)) {
 		pr_debug("readdir() left off=%d\n", ino);
 		goto inside;
 	}
@@ -131,8 +163,8 @@ affs_readdir(struct file *file, struct dir_context *ctx)
 		} while (ino);
 	}
 done:
-	file->f_version = inode_query_iversion(inode);
-	file->private_data = (void *)(long)ino;
+	data->cookie = inode_query_iversion(inode);
+	data->ino = ino;
 	affs_brelse(fh_bh);
 
 out_brelse_dir:

-- 
2.45.2

Re: [PATCH RFC 09/20] affs: store cookie in private data

[Jan Kara]

On Fri 30-08-24 15:04:50, Christian Brauner wrote:
> Store the cookie to detect concurrent seeks on directories in
> file->private_data.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/affs/dir.c | 44 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/affs/dir.c b/fs/affs/dir.c
> index b2bf7016e1b3..bd40d5f08810 100644
> --- a/fs/affs/dir.c
> +++ b/fs/affs/dir.c
> @@ -17,13 +17,44 @@
>  #include <linux/iversion.h>
>  #include "affs.h"
>  
> +struct affs_dir_data {
> +	unsigned long ino;
> +	u64 cookie;
> +};
> +
>  static int affs_readdir(struct file *, struct dir_context *);
>  
> +static loff_t affs_dir_llseek(struct file *file, loff_t offset, int whence)
> +{
> +	struct affs_dir_data *data = file->private_data;
> +
> +	return generic_llseek_cookie(file, offset, whence, &data->cookie);
> +}
> +
> +static int affs_dir_open(struct inode *inode, struct file *file)
> +{
> +	struct affs_dir_data	*data;
> +
> +	data = kzalloc(sizeof(struct affs_dir_data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +	file->private_data = data;
> +	return 0;
> +}
> +
> +static int affs_dir_release(struct inode *inode, struct file *file)
> +{
> +	kfree(file->private_data);
> +	return 0;
> +}
> +
>  const struct file_operations affs_dir_operations = {
> +	.open		= affs_dir_open,
>  	.read		= generic_read_dir,
> -	.llseek		= generic_file_llseek,
> +	.llseek		= affs_dir_llseek,
>  	.iterate_shared	= affs_readdir,
>  	.fsync		= affs_file_fsync,
> +	.release	= affs_dir_release,
>  };
>  
>  /*
> @@ -45,6 +76,7 @@ static int
>  affs_readdir(struct file *file, struct dir_context *ctx)
>  {
>  	struct inode		*inode = file_inode(file);
> +	struct affs_dir_data	*data = file->private_data;
>  	struct super_block	*sb = inode->i_sb;
>  	struct buffer_head	*dir_bh = NULL;
>  	struct buffer_head	*fh_bh = NULL;
> @@ -59,7 +91,7 @@ affs_readdir(struct file *file, struct dir_context *ctx)
>  	pr_debug("%s(ino=%lu,f_pos=%llx)\n", __func__, inode->i_ino, ctx->pos);
>  
>  	if (ctx->pos < 2) {
> -		file->private_data = (void *)0;
> +		data->ino = 0;
>  		if (!dir_emit_dots(file, ctx))
>  			return 0;
>  	}
> @@ -80,8 +112,8 @@ affs_readdir(struct file *file, struct dir_context *ctx)
>  	/* If the directory hasn't changed since the last call to readdir(),
>  	 * we can jump directly to where we left off.
>  	 */
> -	ino = (u32)(long)file->private_data;
> -	if (ino && inode_eq_iversion(inode, file->f_version)) {
> +	ino = data->ino;
> +	if (ino && inode_eq_iversion(inode, data->cookie)) {
>  		pr_debug("readdir() left off=%d\n", ino);
>  		goto inside;
>  	}
> @@ -131,8 +163,8 @@ affs_readdir(struct file *file, struct dir_context *ctx)
>  		} while (ino);
>  	}
>  done:
> -	file->f_version = inode_query_iversion(inode);
> -	file->private_data = (void *)(long)ino;
> +	data->cookie = inode_query_iversion(inode);
> +	data->ino = ino;
>  	affs_brelse(fh_bh);
>  
>  out_brelse_dir:
> 
> -- 
> 2.45.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RFC 10/20] ext2: store cookie in private data

[Christian Brauner]

Store the cookie to detect concurrent seeks on directories in
file->private_data.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/ext2/dir.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/fs/ext2/dir.c b/fs/ext2/dir.c
index 087457061c6e..6622c582f550 100644
--- a/fs/ext2/dir.c
+++ b/fs/ext2/dir.c
@@ -263,7 +263,7 @@ ext2_readdir(struct file *file, struct dir_context *ctx)
 	unsigned long n = pos >> PAGE_SHIFT;
 	unsigned long npages = dir_pages(inode);
 	unsigned chunk_mask = ~(ext2_chunk_size(inode)-1);
-	bool need_revalidate = !inode_eq_iversion(inode, file->f_version);
+	bool need_revalidate = !inode_eq_iversion(inode, *(u64 *)file->private_data);
 	bool has_filetype;
 
 	if (pos > inode->i_size - EXT2_DIR_REC_LEN(1))
@@ -290,7 +290,7 @@ ext2_readdir(struct file *file, struct dir_context *ctx)
 				offset = ext2_validate_entry(kaddr, offset, chunk_mask);
 				ctx->pos = (n<<PAGE_SHIFT) + offset;
 			}
-			file->f_version = inode_query_iversion(inode);
+			*(u64 *)file->private_data = inode_query_iversion(inode);
 			need_revalidate = false;
 		}
 		de = (ext2_dirent *)(kaddr+offset);
@@ -703,8 +703,30 @@ int ext2_empty_dir(struct inode *inode)
 	return 0;
 }
 
+static int ext2_dir_open(struct inode *inode, struct file *file)
+{
+	file->private_data = kzalloc(sizeof(u64), GFP_KERNEL);
+	if (!file->private_data)
+		return -ENOMEM;
+	return 0;
+}
+
+static int ext2_dir_release(struct inode *inode, struct file *file)
+{
+	kfree(file->private_data);
+	return 0;
+}
+
+static loff_t ext2_dir_llseek(struct file *file, loff_t offset, int whence)
+{
+	return generic_llseek_cookie(file, offset, whence,
+				     (u64 *)file->private_data);
+}
+
 const struct file_operations ext2_dir_operations = {
-	.llseek		= generic_file_llseek,
+	.open		= ext2_dir_open,
+	.release	= ext2_dir_release,
+	.llseek		= ext2_dir_llseek,
 	.read		= generic_read_dir,
 	.iterate_shared	= ext2_readdir,
 	.unlocked_ioctl = ext2_ioctl,

-- 
2.45.2

Re: [PATCH RFC 10/20] ext2: store cookie in private data

[Jan Kara]

On Fri 30-08-24 15:04:51, Christian Brauner wrote:
> Store the cookie to detect concurrent seeks on directories in
> file->private_data.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RFC 11/20] ext4: store cookie in private data

[Christian Brauner]

Store the cookie to detect concurrent seeks on directories in
file->private_data.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/ext4/dir.c    | 50 ++++++++++++++++++++++++++++----------------------
 fs/ext4/ext4.h   |  2 ++
 fs/ext4/inline.c |  7 ++++---
 3 files changed, 34 insertions(+), 25 deletions(-)

diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
index ff4514e4626b..13196afe55ce 100644
--- a/fs/ext4/dir.c
+++ b/fs/ext4/dir.c
@@ -133,6 +133,7 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx)
 	struct super_block *sb = inode->i_sb;
 	struct buffer_head *bh = NULL;
 	struct fscrypt_str fstr = FSTR_INIT(NULL, 0);
+	struct dir_private_info *info = file->private_data;
 
 	err = fscrypt_prepare_readdir(inode);
 	if (err)
@@ -229,7 +230,7 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx)
 		 * readdir(2), then we might be pointing to an invalid
 		 * dirent right now.  Scan from the start of the block
 		 * to make sure. */
-		if (!inode_eq_iversion(inode, file->f_version)) {
+		if (!inode_eq_iversion(inode, info->cookie)) {
 			for (i = 0; i < sb->s_blocksize && i < offset; ) {
 				de = (struct ext4_dir_entry_2 *)
 					(bh->b_data + i);
@@ -249,7 +250,7 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx)
 			offset = i;
 			ctx->pos = (ctx->pos & ~(sb->s_blocksize - 1))
 				| offset;
-			file->f_version = inode_query_iversion(inode);
+			info->cookie = inode_query_iversion(inode);
 		}
 
 		while (ctx->pos < inode->i_size
@@ -384,6 +385,7 @@ static inline loff_t ext4_get_htree_eof(struct file *filp)
 static loff_t ext4_dir_llseek(struct file *file, loff_t offset, int whence)
 {
 	struct inode *inode = file->f_mapping->host;
+	struct dir_private_info *info = file->private_data;
 	int dx_dir = is_dx_dir(inode);
 	loff_t ret, htree_max = ext4_get_htree_eof(file);
 
@@ -392,7 +394,7 @@ static loff_t ext4_dir_llseek(struct file *file, loff_t offset, int whence)
 						    htree_max, htree_max);
 	else
 		ret = ext4_llseek(file, offset, whence);
-	file->f_version = inode_peek_iversion(inode) - 1;
+	info->cookie = inode_peek_iversion(inode) - 1;
 	return ret;
 }
 
@@ -429,18 +431,15 @@ static void free_rb_tree_fname(struct rb_root *root)
 	*root = RB_ROOT;
 }
 
-
-static struct dir_private_info *ext4_htree_create_dir_info(struct file *filp,
-							   loff_t pos)
+static void ext4_htree_init_dir_info(struct file *filp, loff_t pos)
 {
-	struct dir_private_info *p;
-
-	p = kzalloc(sizeof(*p), GFP_KERNEL);
-	if (!p)
-		return NULL;
-	p->curr_hash = pos2maj_hash(filp, pos);
-	p->curr_minor_hash = pos2min_hash(filp, pos);
-	return p;
+	struct dir_private_info *p = filp->private_data;
+
+	if (is_dx_dir(file_inode(filp)) && !p->initialized) {
+		p->curr_hash = pos2maj_hash(filp, pos);
+		p->curr_minor_hash = pos2min_hash(filp, pos);
+		p->initialized = true;
+	}
 }
 
 void ext4_htree_free_dir_info(struct dir_private_info *p)
@@ -552,12 +551,7 @@ static int ext4_dx_readdir(struct file *file, struct dir_context *ctx)
 	struct fname *fname;
 	int ret = 0;
 
-	if (!info) {
-		info = ext4_htree_create_dir_info(file, ctx->pos);
-		if (!info)
-			return -ENOMEM;
-		file->private_data = info;
-	}
+	ext4_htree_init_dir_info(file, ctx->pos);
 
 	if (ctx->pos == ext4_get_htree_eof(file))
 		return 0;	/* EOF */
@@ -590,10 +584,10 @@ static int ext4_dx_readdir(struct file *file, struct dir_context *ctx)
 		 * cached entries.
 		 */
 		if ((!info->curr_node) ||
-		    !inode_eq_iversion(inode, file->f_version)) {
+		    !inode_eq_iversion(inode, info->cookie)) {
 			info->curr_node = NULL;
 			free_rb_tree_fname(&info->root);
-			file->f_version = inode_query_iversion(inode);
+			info->cookie = inode_query_iversion(inode);
 			ret = ext4_htree_fill_tree(file, info->curr_hash,
 						   info->curr_minor_hash,
 						   &info->next_hash);
@@ -664,7 +658,19 @@ int ext4_check_all_de(struct inode *dir, struct buffer_head *bh, void *buf,
 	return 0;
 }
 
+static int ext4_dir_open(struct inode *inode, struct file *file)
+{
+	struct dir_private_info *info;
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+	file->private_data = info;
+	return 0;
+}
+
 const struct file_operations ext4_dir_operations = {
+	.open		= ext4_dir_open,
 	.llseek		= ext4_dir_llseek,
 	.read		= generic_read_dir,
 	.iterate_shared	= ext4_readdir,
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 08acd152261e..d62a4b9b26ce 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2553,6 +2553,8 @@ struct dir_private_info {
 	__u32		curr_hash;
 	__u32		curr_minor_hash;
 	__u32		next_hash;
+	u64		cookie;
+	bool		initialized;
 };
 
 /* calculate the first block number of the group */
diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
index e7a09a99837b..4282e12dc405 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c
@@ -1460,6 +1460,7 @@ int ext4_read_inline_dir(struct file *file,
 	struct ext4_iloc iloc;
 	void *dir_buf = NULL;
 	int dotdot_offset, dotdot_size, extra_offset, extra_size;
+	struct dir_private_info *info = file->private_data;
 
 	ret = ext4_get_inode_loc(inode, &iloc);
 	if (ret)
@@ -1503,12 +1504,12 @@ int ext4_read_inline_dir(struct file *file,
 	extra_size = extra_offset + inline_size;
 
 	/*
-	 * If the version has changed since the last call to
+	 * If the cookie has changed since the last call to
 	 * readdir(2), then we might be pointing to an invalid
 	 * dirent right now.  Scan from the start of the inline
 	 * dir to make sure.
 	 */
-	if (!inode_eq_iversion(inode, file->f_version)) {
+	if (!inode_eq_iversion(inode, info->cookie)) {
 		for (i = 0; i < extra_size && i < offset;) {
 			/*
 			 * "." is with offset 0 and
@@ -1540,7 +1541,7 @@ int ext4_read_inline_dir(struct file *file,
 		}
 		offset = i;
 		ctx->pos = offset;
-		file->f_version = inode_query_iversion(inode);
+		info->cookie = inode_query_iversion(inode);
 	}
 
 	while (ctx->pos < extra_size) {

-- 
2.45.2

Re: [PATCH RFC 11/20] ext4: store cookie in private data

[Theodore Ts'o]

On Fri, Aug 30, 2024 at 03:04:52PM +0200, Christian Brauner wrote:
> Store the cookie to detect concurrent seeks on directories in
> file->private_data.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Acked-by: Theodore Ts'o <tytso@mit.edu>

Re: [PATCH RFC 11/20] ext4: store cookie in private data

[Jan Kara]

On Fri 30-08-24 15:04:52, Christian Brauner wrote:
> Store the cookie to detect concurrent seeks on directories in
> file->private_data.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/dir.c    | 50 ++++++++++++++++++++++++++++----------------------
>  fs/ext4/ext4.h   |  2 ++
>  fs/ext4/inline.c |  7 ++++---
>  3 files changed, 34 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
> index ff4514e4626b..13196afe55ce 100644
> --- a/fs/ext4/dir.c
> +++ b/fs/ext4/dir.c
> @@ -133,6 +133,7 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx)
>  	struct super_block *sb = inode->i_sb;
>  	struct buffer_head *bh = NULL;
>  	struct fscrypt_str fstr = FSTR_INIT(NULL, 0);
> +	struct dir_private_info *info = file->private_data;
>  
>  	err = fscrypt_prepare_readdir(inode);
>  	if (err)
> @@ -229,7 +230,7 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx)
>  		 * readdir(2), then we might be pointing to an invalid
>  		 * dirent right now.  Scan from the start of the block
>  		 * to make sure. */
> -		if (!inode_eq_iversion(inode, file->f_version)) {
> +		if (!inode_eq_iversion(inode, info->cookie)) {
>  			for (i = 0; i < sb->s_blocksize && i < offset; ) {
>  				de = (struct ext4_dir_entry_2 *)
>  					(bh->b_data + i);
> @@ -249,7 +250,7 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx)
>  			offset = i;
>  			ctx->pos = (ctx->pos & ~(sb->s_blocksize - 1))
>  				| offset;
> -			file->f_version = inode_query_iversion(inode);
> +			info->cookie = inode_query_iversion(inode);
>  		}
>  
>  		while (ctx->pos < inode->i_size
> @@ -384,6 +385,7 @@ static inline loff_t ext4_get_htree_eof(struct file *filp)
>  static loff_t ext4_dir_llseek(struct file *file, loff_t offset, int whence)
>  {
>  	struct inode *inode = file->f_mapping->host;
> +	struct dir_private_info *info = file->private_data;
>  	int dx_dir = is_dx_dir(inode);
>  	loff_t ret, htree_max = ext4_get_htree_eof(file);
>  
> @@ -392,7 +394,7 @@ static loff_t ext4_dir_llseek(struct file *file, loff_t offset, int whence)
>  						    htree_max, htree_max);
>  	else
>  		ret = ext4_llseek(file, offset, whence);
> -	file->f_version = inode_peek_iversion(inode) - 1;
> +	info->cookie = inode_peek_iversion(inode) - 1;
>  	return ret;
>  }
>  
> @@ -429,18 +431,15 @@ static void free_rb_tree_fname(struct rb_root *root)
>  	*root = RB_ROOT;
>  }
>  
> -
> -static struct dir_private_info *ext4_htree_create_dir_info(struct file *filp,
> -							   loff_t pos)
> +static void ext4_htree_init_dir_info(struct file *filp, loff_t pos)
>  {
> -	struct dir_private_info *p;
> -
> -	p = kzalloc(sizeof(*p), GFP_KERNEL);
> -	if (!p)
> -		return NULL;
> -	p->curr_hash = pos2maj_hash(filp, pos);
> -	p->curr_minor_hash = pos2min_hash(filp, pos);
> -	return p;
> +	struct dir_private_info *p = filp->private_data;
> +
> +	if (is_dx_dir(file_inode(filp)) && !p->initialized) {
> +		p->curr_hash = pos2maj_hash(filp, pos);
> +		p->curr_minor_hash = pos2min_hash(filp, pos);
> +		p->initialized = true;
> +	}
>  }
>  
>  void ext4_htree_free_dir_info(struct dir_private_info *p)
> @@ -552,12 +551,7 @@ static int ext4_dx_readdir(struct file *file, struct dir_context *ctx)
>  	struct fname *fname;
>  	int ret = 0;
>  
> -	if (!info) {
> -		info = ext4_htree_create_dir_info(file, ctx->pos);
> -		if (!info)
> -			return -ENOMEM;
> -		file->private_data = info;
> -	}
> +	ext4_htree_init_dir_info(file, ctx->pos);
>  
>  	if (ctx->pos == ext4_get_htree_eof(file))
>  		return 0;	/* EOF */
> @@ -590,10 +584,10 @@ static int ext4_dx_readdir(struct file *file, struct dir_context *ctx)
>  		 * cached entries.
>  		 */
>  		if ((!info->curr_node) ||
> -		    !inode_eq_iversion(inode, file->f_version)) {
> +		    !inode_eq_iversion(inode, info->cookie)) {
>  			info->curr_node = NULL;
>  			free_rb_tree_fname(&info->root);
> -			file->f_version = inode_query_iversion(inode);
> +			info->cookie = inode_query_iversion(inode);
>  			ret = ext4_htree_fill_tree(file, info->curr_hash,
>  						   info->curr_minor_hash,
>  						   &info->next_hash);
> @@ -664,7 +658,19 @@ int ext4_check_all_de(struct inode *dir, struct buffer_head *bh, void *buf,
>  	return 0;
>  }
>  
> +static int ext4_dir_open(struct inode *inode, struct file *file)
> +{
> +	struct dir_private_info *info;
> +
> +	info = kzalloc(sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +	file->private_data = info;
> +	return 0;
> +}
> +
>  const struct file_operations ext4_dir_operations = {
> +	.open		= ext4_dir_open,
>  	.llseek		= ext4_dir_llseek,
>  	.read		= generic_read_dir,
>  	.iterate_shared	= ext4_readdir,
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 08acd152261e..d62a4b9b26ce 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -2553,6 +2553,8 @@ struct dir_private_info {
>  	__u32		curr_hash;
>  	__u32		curr_minor_hash;
>  	__u32		next_hash;
> +	u64		cookie;
> +	bool		initialized;
>  };
>  
>  /* calculate the first block number of the group */
> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index e7a09a99837b..4282e12dc405 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
> @@ -1460,6 +1460,7 @@ int ext4_read_inline_dir(struct file *file,
>  	struct ext4_iloc iloc;
>  	void *dir_buf = NULL;
>  	int dotdot_offset, dotdot_size, extra_offset, extra_size;
> +	struct dir_private_info *info = file->private_data;
>  
>  	ret = ext4_get_inode_loc(inode, &iloc);
>  	if (ret)
> @@ -1503,12 +1504,12 @@ int ext4_read_inline_dir(struct file *file,
>  	extra_size = extra_offset + inline_size;
>  
>  	/*
> -	 * If the version has changed since the last call to
> +	 * If the cookie has changed since the last call to
>  	 * readdir(2), then we might be pointing to an invalid
>  	 * dirent right now.  Scan from the start of the inline
>  	 * dir to make sure.
>  	 */
> -	if (!inode_eq_iversion(inode, file->f_version)) {
> +	if (!inode_eq_iversion(inode, info->cookie)) {
>  		for (i = 0; i < extra_size && i < offset;) {
>  			/*
>  			 * "." is with offset 0 and
> @@ -1540,7 +1541,7 @@ int ext4_read_inline_dir(struct file *file,
>  		}
>  		offset = i;
>  		ctx->pos = offset;
> -		file->f_version = inode_query_iversion(inode);
> +		info->cookie = inode_query_iversion(inode);
>  	}
>  
>  	while (ctx->pos < extra_size) {
> 
> -- 
> 2.45.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RFC 12/20] input: remove f_version abuse

[Christian Brauner]

Remove the f_version abuse from input. Use seq_private_open() to stash
the information for poll.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 drivers/input/input.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index 54c57b267b25..b03ae43707d8 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -1081,9 +1081,11 @@ static inline void input_wakeup_procfs_readers(void)
 
 static __poll_t input_proc_devices_poll(struct file *file, poll_table *wait)
 {
+	struct seq_file *m = file->private_data;
+
 	poll_wait(file, &input_devices_poll_wait, wait);
-	if (file->f_version != input_devices_state) {
-		file->f_version = input_devices_state;
+	if (*(u64 *)m->private != input_devices_state) {
+		*(u64 *)m->private = input_devices_state;
 		return EPOLLIN | EPOLLRDNORM;
 	}
 
@@ -1210,7 +1212,7 @@ static const struct seq_operations input_devices_seq_ops = {
 
 static int input_proc_devices_open(struct inode *inode, struct file *file)
 {
-	return seq_open(file, &input_devices_seq_ops);
+	return seq_open_private(file, &input_devices_seq_ops, sizeof(u64));
 }
 
 static const struct proc_ops input_devices_proc_ops = {

-- 
2.45.2

Re: [PATCH RFC 12/20] input: remove f_version abuse

[Jan Kara]

On Fri 30-08-24 15:04:53, Christian Brauner wrote:
> Remove the f_version abuse from input. Use seq_private_open() to stash
> the information for poll.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  drivers/input/input.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index 54c57b267b25..b03ae43707d8 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -1081,9 +1081,11 @@ static inline void input_wakeup_procfs_readers(void)
>  
>  static __poll_t input_proc_devices_poll(struct file *file, poll_table *wait)
>  {
> +	struct seq_file *m = file->private_data;
> +
>  	poll_wait(file, &input_devices_poll_wait, wait);
> -	if (file->f_version != input_devices_state) {
> -		file->f_version = input_devices_state;
> +	if (*(u64 *)m->private != input_devices_state) {
> +		*(u64 *)m->private = input_devices_state;
>  		return EPOLLIN | EPOLLRDNORM;
>  	}
>  
> @@ -1210,7 +1212,7 @@ static const struct seq_operations input_devices_seq_ops = {
>  
>  static int input_proc_devices_open(struct inode *inode, struct file *file)
>  {
> -	return seq_open(file, &input_devices_seq_ops);
> +	return seq_open_private(file, &input_devices_seq_ops, sizeof(u64));
>  }
>  
>  static const struct proc_ops input_devices_proc_ops = {
> 
> -- 
> 2.45.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH RFC 12/20] input: remove f_version abuse

[Lai, Yi]

Hi Christian Brauner,

Greetings!

I used Syzkaller and found that there is BUG: unable to handle kernel paging request in input_proc_devices_poll in next-20240909.

After bisection and the first bad commit is:
"
7c3d158418c2 input: remove f_version abuse
"

All detailed into can be found at:
https://github.com/laifryiee/syzkaller_logs/tree/main/240911_155303_input_proc_devices_poll
Syzkaller repro code:
https://github.com/laifryiee/syzkaller_logs/blob/main/240911_155303_input_proc_devices_poll/repro.c
Syzkaller repro syscall steps:
https://github.com/laifryiee/syzkaller_logs/blob/main/240911_155303_input_proc_devices_poll/repro.prog
Syzkaller report:
https://github.com/laifryiee/syzkaller_logs/blob/main/240911_155303_input_proc_devices_poll/repro.report
Kconfig(make olddefconfig):
https://github.com/laifryiee/syzkaller_logs/blob/main/240911_155303_input_proc_devices_poll/kconfig_origin
Bisect info:
https://github.com/laifryiee/syzkaller_logs/blob/main/240911_155303_input_proc_devices_poll/bisect_info.log
bzImage:
https://github.com/laifryiee/syzkaller_logs/raw/main/240911_155303_input_proc_devices_poll/bzImage_100cc857359b5d731407d1038f7e76cd0e871d94
Issue dmesg:
https://github.com/laifryiee/syzkaller_logs/blob/main/240911_155303_input_proc_devices_poll/100cc857359b5d731407d1038f7e76cd0e871d94_dmesg.log

"
[   23.266063] ==================================================================
[   23.268350] BUG: KASAN: slab-out-of-bounds in input_proc_devices_poll+0x113/0x140
[   23.270806] Read of size 8 at addr ffff88801101fa40 by task repro/729
[   23.272537] 
[   23.272980] CPU: 1 UID: 0 PID: 729 Comm: repro Not tainted 6.11.0-rc7-next-20240909-100cc857359b-dirty #1
[   23.274230] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
[   23.275200] Call Trace:
[   23.275432]  <TASK>
[   23.275633]  dump_stack_lvl+0xea/0x150
[   23.275972]  print_report+0xce/0x610
[   23.276269]  ? input_proc_devices_poll+0x113/0x140
[   23.276650]  ? kasan_complete_mode_report_info+0x40/0x200
[   23.277058]  ? input_proc_devices_poll+0x113/0x140
[   23.277400]  kasan_report+0xcc/0x110
[   23.277668]  ? input_proc_devices_poll+0x113/0x140
[   23.277989]  ? __pfx___pollwait+0x10/0x10
[   23.278288]  __asan_report_load8_noabort+0x18/0x20
[   23.278634]  input_proc_devices_poll+0x113/0x140
[   23.278963]  ? __pfx_input_proc_devices_poll+0x10/0x10
[   23.279325]  proc_reg_poll+0x210/0x2e0
[   23.279607]  ? __pfx_proc_reg_poll+0x10/0x10
[   23.279917]  do_sys_poll+0x521/0xdd0
[   23.280188]  ? __pfx_do_sys_poll+0x10/0x10
[   23.280485]  ? __kasan_check_read+0x15/0x20
[   23.280791]  ? mark_lock.part.0+0xf3/0x17b0
[   23.281101]  ? __pfx_mark_lock.part.0+0x10/0x10
[   23.281427]  ? __kasan_check_read+0x15/0x20
[   23.281736]  ? mark_lock.part.0+0xf3/0x17b0
[   23.282039]  ? mutex_unlock+0x16/0x20
[   23.282311]  ? seq_read_iter+0x72/0x1300
[   23.282604]  ? __pfx_mark_lock.part.0+0x10/0x10
[   23.282935]  ? __pfx___pollwait+0x10/0x10
[   23.283235]  ? __pfx_pollwake+0x10/0x10
[   23.283526]  ? __pfx___lock_acquire+0x10/0x10
[   23.283848]  ? __this_cpu_preempt_check+0x21/0x30
[   23.284200]  ? __this_cpu_preempt_check+0x21/0x30
[   23.284542]  ? lock_release+0x441/0x870
[   23.284825]  ? __sanitizer_cov_trace_cmp8+0x1c/0x30
[   23.285180]  ? timespec64_add_safe+0x192/0x220
[   23.285505]  ? __pfx_timespec64_add_safe+0x10/0x10
[   23.285851]  ? __sanitizer_cov_trace_const_cmp1+0x1e/0x30
[   23.286236]  ? __sanitizer_cov_trace_const_cmp8+0x1c/0x30
[   23.286622]  ? ktime_get_ts64+0x1db/0x2e0
[   23.286925]  __x64_sys_poll+0x1bf/0x560
[   23.287205]  ? __pfx___x64_sys_poll+0x10/0x10
[   23.287524]  x64_sys_call+0x1294/0x2140
[   23.287808]  do_syscall_64+0x6d/0x140
[   23.288083]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   23.288457] RIP: 0033:0x7faf33c3ee5d
[   23.288721] Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 93 af 1b 00 f7 d8 64 89 01 48
[   23.289988] RSP: 002b:00007ffff3de1fe8 EFLAGS: 00000207 ORIG_RAX: 0000000000000007
[   23.290531] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007faf33c3ee5d
[   23.291025] RDX: 0000000000000029 RSI: 0000000000000005 RDI: 0000000020000040
[   23.291519] RBP: 00007ffff3de2000 R08: 00007ffff3de2000 R09: 00007ffff3de2000
[   23.292201] R10: 00007ffff3de2000 R11: 0000000000000207 R12: 00007ffff3de2158
[   23.292800] R13: 0000000000401810 R14: 0000000000403e08 R15: 00007faf33e55000
[   23.293320]  </TASK>
[   23.293487] 
[   23.293609] Allocated by task 1:
[   23.293862]  kasan_save_stack+0x2c/0x60
[   23.294153]  kasan_save_track+0x18/0x40
[   23.294435]  kasan_save_alloc_info+0x3c/0x50
[   23.294744]  __kasan_kmalloc+0x88/0xa0
[   23.295019]  __kmalloc_noprof+0x1cd/0x4a0
[   23.295316]  cgroup_mkdir+0x282/0x1320
[   23.295602]  kernfs_iop_mkdir+0x15a/0x1f0
[   23.295899]  vfs_mkdir+0x57d/0x860
[   23.296157]  do_mkdirat+0x2e2/0x3b0
[   23.296414]  __x64_sys_mkdir+0xfd/0x150
[   23.296692]  x64_sys_call+0x1c5a/0x2140
[   23.296974]  do_syscall_64+0x6d/0x140
[   23.297246]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   23.297611] 
[   23.297735] The buggy address belongs to the object at ffff88801101e000
[   23.297735]  which belongs to the cache kmalloc-4k of size 4096
[   23.298583] The buggy address is located 4584 bytes to the right of
[   23.298583]  allocated 2136-byte region [ffff88801101e000, ffff88801101e858)
[   23.299489] 
[   23.299611] The buggy address belongs to the physical page:
[   23.300004] page: refcount:1 mapcount:0 mapping:0000000000000000 index:0xffff88801101c000 pfn:0x11018
[   23.300643] head: order:3 mapcount:0 entire_mapcount:0 nr_pages_mapped:0 pincount:0
[   23.301177] flags: 0xfffffc0000240(workingset|head|node=0|zone=1|lastcpupid=0x1fffff)
[   23.301729] page_type: f5(slab)
[   23.301968] raw: 000fffffc0000240 ffff88800d442140 ffffea00004e3610 ffffea0000446410
[   23.302505] raw: ffff88801101c000 0000000000040002 00000001f5000000 0000000000000000
[   23.303048] head: 000fffffc0000240 ffff88800d442140 ffffea00004e3610 ffffea0000446410
[   23.303597] head: ffff88801101c000 0000000000040002 00000001f5000000 0000000000000000
[   23.304147] head: 000fffffc0000003 ffffea0000440601 ffffffffffffffff 0000000000000000
[   23.304692] head: ffff888000000008 0000000000000000 00000000ffffffff 0000000000000000
[   23.305238] page dumped because: kasan: bad access detected
[   23.305632] 
[   23.305752] Memory state around the buggy address:
[   23.306096]  ffff88801101f900: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[   23.306600]  ffff88801101f980: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[   23.307102] >ffff88801101fa00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[   23.307612]                                            ^
[   23.307984]  ffff88801101fa80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[   23.308490]  ffff88801101fb00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[   23.308993] ==================================================================
[   23.309546] Disabling lock debugging due to kernel taint

"

I hope you find it useful.

Regards,
Yi Lai

---

If you don't need the following environment to reproduce the problem or if you
already have one reproduced environment, please ignore the following information.

How to reproduce:
git clone https://gitlab.com/xupengfe/repro_vm_env.git
cd repro_vm_env
tar -xvf repro_vm_env.tar.gz
cd repro_vm_env; ./start3.sh  // it needs qemu-system-x86_64 and I used v7.1.0
  // start3.sh will load bzImage_2241ab53cbb5cdb08a6b2d4688feb13971058f65 v6.2-rc5 kernel
  // You could change the bzImage_xxx as you want
  // Maybe you need to remove line "-drive if=pflash,format=raw,readonly=on,file=./OVMF_CODE.fd \" for different qemu version
You could use below command to log in, there is no password for root.
ssh -p 10023 root@localhost

After login vm(virtual machine) successfully, you could transfer reproduced
binary to the vm by below way, and reproduce the problem in vm:
gcc -pthread -o repro repro.c
scp -P 10023 repro root@localhost:/root/

Get the bzImage for target kernel:
Please use target kconfig and copy it to kernel_src/.config
make olddefconfig
make -jx bzImage           //x should equal or less than cpu num your pc has

Fill the bzImage file into above start3.sh to load the target kernel in vm.

Tips:
If you already have qemu-system-x86_64, please ignore below info.
If you want to install qemu v7.1.0 version:
git clone https://github.com/qemu/qemu.git
cd qemu
git checkout -f v7.1.0
mkdir build
cd build
yum install -y ninja-build.x86_64
yum -y install libslirp-devel.x86_64
../configure --target-list=x86_64-softmmu --enable-kvm --enable-vnc --enable-gtk --enable-sdl --enable-usb-redir --enable-slirp
make
make install 

On Fri, Aug 30, 2024 at 03:04:53PM +0200, Christian Brauner wrote:
> Remove the f_version abuse from input. Use seq_private_open() to stash
> the information for poll.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
>  drivers/input/input.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index 54c57b267b25..b03ae43707d8 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -1081,9 +1081,11 @@ static inline void input_wakeup_procfs_readers(void)
>  
>  static __poll_t input_proc_devices_poll(struct file *file, poll_table *wait)
>  {
> +	struct seq_file *m = file->private_data;
> +
>  	poll_wait(file, &input_devices_poll_wait, wait);
> -	if (file->f_version != input_devices_state) {
> -		file->f_version = input_devices_state;
> +	if (*(u64 *)m->private != input_devices_state) {
> +		*(u64 *)m->private = input_devices_state;
>  		return EPOLLIN | EPOLLRDNORM;
>  	}
>  
> @@ -1210,7 +1212,7 @@ static const struct seq_operations input_devices_seq_ops = {
>  
>  static int input_proc_devices_open(struct inode *inode, struct file *file)
>  {
> -	return seq_open(file, &input_devices_seq_ops);
> +	return seq_open_private(file, &input_devices_seq_ops, sizeof(u64));
>  }
>  
>  static const struct proc_ops input_devices_proc_ops = {
> 
> -- 
> 2.45.2
> 

Re: [PATCH RFC 12/20] input: remove f_version abuse

[Christian Brauner]

[-- Attachment #1: Type: text/plain, Size: 1507 bytes --]

On Thu, Sep 12, 2024 at 10:52:16AM GMT, Lai, Yi wrote:
> Hi Christian Brauner,
> 
> Greetings!
> 
> I used Syzkaller and found that there is BUG: unable to handle kernel paging request in input_proc_devices_poll in next-20240909.
> 
> After bisection and the first bad commit is:
> "
> 7c3d158418c2 input: remove f_version abuse
> "
> 
> All detailed into can be found at:
> https://github.com/laifryiee/syzkaller_logs/tree/main/240911_155303_input_proc_devices_poll
> Syzkaller repro code:
> https://github.com/laifryiee/syzkaller_logs/blob/main/240911_155303_input_proc_devices_poll/repro.c
> Syzkaller repro syscall steps:
> https://github.com/laifryiee/syzkaller_logs/blob/main/240911_155303_input_proc_devices_poll/repro.prog
> Syzkaller report:
> https://github.com/laifryiee/syzkaller_logs/blob/main/240911_155303_input_proc_devices_poll/repro.report
> Kconfig(make olddefconfig):
> https://github.com/laifryiee/syzkaller_logs/blob/main/240911_155303_input_proc_devices_poll/kconfig_origin
> Bisect info:
> https://github.com/laifryiee/syzkaller_logs/blob/main/240911_155303_input_proc_devices_poll/bisect_info.log
> bzImage:
> https://github.com/laifryiee/syzkaller_logs/raw/main/240911_155303_input_proc_devices_poll/bzImage_100cc857359b5d731407d1038f7e76cd0e871d94
> Issue dmesg:
> https://github.com/laifryiee/syzkaller_logs/blob/main/240911_155303_input_proc_devices_poll/100cc857359b5d731407d1038f7e76cd0e871d94_dmesg.log

Thanks for all the info. I see what the issue is and pushed a fix out.

[-- Attachment #2: 0001-input-remove-f_version-abuse.patch --]
[-- Type: text/x-diff, Size: 5041 bytes --]

From 7a7ce8b3ba66754f5d275a71630b4ee8b507d266 Mon Sep 17 00:00:00 2001
From: Christian Brauner <brauner@kernel.org>
Date: Fri, 30 Aug 2024 15:04:53 +0200
Subject: [PATCH] input: remove f_version abuse

f_version is removed from struct file. Make input stop abusing f_version
for stashing information for poll. Move the input state counter into
input_seq_state and allocate it via seq_private_open() and free via
seq_release_private().

Link: https://lore.kernel.org/r/20240830-vfs-file-f_version-v1-12-6d3e4816aa7b@kernel.org
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 drivers/input/input.c | 47 ++++++++++++++++++++-----------------------
 1 file changed, 22 insertions(+), 25 deletions(-)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index 54c57b267b25..19ea1888da9f 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -1079,33 +1079,31 @@ static inline void input_wakeup_procfs_readers(void)
 	wake_up(&input_devices_poll_wait);
 }
 
+struct input_seq_state {
+	unsigned short pos;
+	bool mutex_acquired;
+	int input_devices_state;
+};
+
 static __poll_t input_proc_devices_poll(struct file *file, poll_table *wait)
 {
+	struct seq_file *seq = file->private_data;
+	struct input_seq_state *state = seq->private;
+
 	poll_wait(file, &input_devices_poll_wait, wait);
-	if (file->f_version != input_devices_state) {
-		file->f_version = input_devices_state;
+	if (state->input_devices_state != input_devices_state) {
+		state->input_devices_state = input_devices_state;
 		return EPOLLIN | EPOLLRDNORM;
 	}
 
 	return 0;
 }
 
-union input_seq_state {
-	struct {
-		unsigned short pos;
-		bool mutex_acquired;
-	};
-	void *p;
-};
-
 static void *input_devices_seq_start(struct seq_file *seq, loff_t *pos)
 {
-	union input_seq_state *state = (union input_seq_state *)&seq->private;
+	struct input_seq_state *state = seq->private;
 	int error;
 
-	/* We need to fit into seq->private pointer */
-	BUILD_BUG_ON(sizeof(union input_seq_state) != sizeof(seq->private));
-
 	error = mutex_lock_interruptible(&input_mutex);
 	if (error) {
 		state->mutex_acquired = false;
@@ -1124,7 +1122,7 @@ static void *input_devices_seq_next(struct seq_file *seq, void *v, loff_t *pos)
 
 static void input_seq_stop(struct seq_file *seq, void *v)
 {
-	union input_seq_state *state = (union input_seq_state *)&seq->private;
+	struct input_seq_state *state = seq->private;
 
 	if (state->mutex_acquired)
 		mutex_unlock(&input_mutex);
@@ -1210,7 +1208,8 @@ static const struct seq_operations input_devices_seq_ops = {
 
 static int input_proc_devices_open(struct inode *inode, struct file *file)
 {
-	return seq_open(file, &input_devices_seq_ops);
+	return seq_open_private(file, &input_devices_seq_ops,
+				sizeof(struct input_seq_state));
 }
 
 static const struct proc_ops input_devices_proc_ops = {
@@ -1218,17 +1217,14 @@ static const struct proc_ops input_devices_proc_ops = {
 	.proc_poll	= input_proc_devices_poll,
 	.proc_read	= seq_read,
 	.proc_lseek	= seq_lseek,
-	.proc_release	= seq_release,
+	.proc_release	= seq_release_private,
 };
 
 static void *input_handlers_seq_start(struct seq_file *seq, loff_t *pos)
 {
-	union input_seq_state *state = (union input_seq_state *)&seq->private;
+	struct input_seq_state *state = seq->private;
 	int error;
 
-	/* We need to fit into seq->private pointer */
-	BUILD_BUG_ON(sizeof(union input_seq_state) != sizeof(seq->private));
-
 	error = mutex_lock_interruptible(&input_mutex);
 	if (error) {
 		state->mutex_acquired = false;
@@ -1243,7 +1239,7 @@ static void *input_handlers_seq_start(struct seq_file *seq, loff_t *pos)
 
 static void *input_handlers_seq_next(struct seq_file *seq, void *v, loff_t *pos)
 {
-	union input_seq_state *state = (union input_seq_state *)&seq->private;
+	struct input_seq_state *state = seq->private;
 
 	state->pos = *pos + 1;
 	return seq_list_next(v, &input_handler_list, pos);
@@ -1252,7 +1248,7 @@ static void *input_handlers_seq_next(struct seq_file *seq, void *v, loff_t *pos)
 static int input_handlers_seq_show(struct seq_file *seq, void *v)
 {
 	struct input_handler *handler = container_of(v, struct input_handler, node);
-	union input_seq_state *state = (union input_seq_state *)&seq->private;
+	struct input_seq_state *state = seq->private;
 
 	seq_printf(seq, "N: Number=%u Name=%s", state->pos, handler->name);
 	if (handler->filter)
@@ -1273,14 +1269,15 @@ static const struct seq_operations input_handlers_seq_ops = {
 
 static int input_proc_handlers_open(struct inode *inode, struct file *file)
 {
-	return seq_open(file, &input_handlers_seq_ops);
+	return seq_open_private(file, &input_handlers_seq_ops,
+				sizeof(struct input_seq_state));
 }
 
 static const struct proc_ops input_handlers_proc_ops = {
 	.proc_open	= input_proc_handlers_open,
 	.proc_read	= seq_read,
 	.proc_lseek	= seq_lseek,
-	.proc_release	= seq_release,
+	.proc_release	= seq_release_private,
 };
 
 static int __init input_proc_init(void)
-- 
2.45.2

[PATCH RFC 13/20] ocfs2: store cookie in private data

[Christian Brauner]

Store the cookie to detect concurrent seeks on directories in
file->private_data.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/ocfs2/dir.c  |  3 ++-
 fs/ocfs2/file.c | 11 +++++++++--
 fs/ocfs2/file.h |  1 +
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c
index f0beb173dbba..ccef3f42b333 100644
--- a/fs/ocfs2/dir.c
+++ b/fs/ocfs2/dir.c
@@ -1932,6 +1932,7 @@ int ocfs2_readdir(struct file *file, struct dir_context *ctx)
 {
 	int error = 0;
 	struct inode *inode = file_inode(file);
+	struct ocfs2_file_private *fp = file->private_data;
 	int lock_level = 0;
 
 	trace_ocfs2_readdir((unsigned long long)OCFS2_I(inode)->ip_blkno);
@@ -1952,7 +1953,7 @@ int ocfs2_readdir(struct file *file, struct dir_context *ctx)
 		goto bail_nolock;
 	}
 
-	error = ocfs2_dir_foreach_blk(inode, &file->f_version, ctx, false);
+	error = ocfs2_dir_foreach_blk(inode, &fp->cookie, ctx, false);
 
 	ocfs2_inode_unlock(inode, lock_level);
 	if (error)
diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index ccc57038a977..115ab2172820 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -2750,6 +2750,13 @@ static loff_t ocfs2_remap_file_range(struct file *file_in, loff_t pos_in,
 	return remapped > 0 ? remapped : ret;
 }
 
+static loff_t ocfs2_dir_llseek(struct file *file, loff_t offset, int whence)
+{
+	struct ocfs2_file_private *fp = file->private_data;
+
+	return generic_llseek_cookie(file, offset, whence, &fp->cookie);
+}
+
 const struct inode_operations ocfs2_file_iops = {
 	.setattr	= ocfs2_setattr,
 	.getattr	= ocfs2_getattr,
@@ -2797,7 +2804,7 @@ const struct file_operations ocfs2_fops = {
 
 WRAP_DIR_ITER(ocfs2_readdir) // FIXME!
 const struct file_operations ocfs2_dops = {
-	.llseek		= generic_file_llseek,
+	.llseek		= ocfs2_dir_llseek,
 	.read		= generic_read_dir,
 	.iterate_shared	= shared_ocfs2_readdir,
 	.fsync		= ocfs2_sync_file,
@@ -2843,7 +2850,7 @@ const struct file_operations ocfs2_fops_no_plocks = {
 };
 
 const struct file_operations ocfs2_dops_no_plocks = {
-	.llseek		= generic_file_llseek,
+	.llseek		= ocfs2_dir_llseek,
 	.read		= generic_read_dir,
 	.iterate_shared	= shared_ocfs2_readdir,
 	.fsync		= ocfs2_sync_file,
diff --git a/fs/ocfs2/file.h b/fs/ocfs2/file.h
index 8e53e4ac1120..41e65e45a9f3 100644
--- a/fs/ocfs2/file.h
+++ b/fs/ocfs2/file.h
@@ -20,6 +20,7 @@ struct ocfs2_alloc_context;
 enum ocfs2_alloc_restarted;
 
 struct ocfs2_file_private {
+	u64			cookie;
 	struct file		*fp_file;
 	struct mutex		fp_mutex;
 	struct ocfs2_lock_res	fp_flock;

-- 
2.45.2

Re: [PATCH RFC 13/20] ocfs2: store cookie in private data

[Jan Kara]

On Fri 30-08-24 15:04:54, Christian Brauner wrote:
> Store the cookie to detect concurrent seeks on directories in
> file->private_data.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ocfs2/dir.c  |  3 ++-
>  fs/ocfs2/file.c | 11 +++++++++--
>  fs/ocfs2/file.h |  1 +
>  3 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c
> index f0beb173dbba..ccef3f42b333 100644
> --- a/fs/ocfs2/dir.c
> +++ b/fs/ocfs2/dir.c
> @@ -1932,6 +1932,7 @@ int ocfs2_readdir(struct file *file, struct dir_context *ctx)
>  {
>  	int error = 0;
>  	struct inode *inode = file_inode(file);
> +	struct ocfs2_file_private *fp = file->private_data;
>  	int lock_level = 0;
>  
>  	trace_ocfs2_readdir((unsigned long long)OCFS2_I(inode)->ip_blkno);
> @@ -1952,7 +1953,7 @@ int ocfs2_readdir(struct file *file, struct dir_context *ctx)
>  		goto bail_nolock;
>  	}
>  
> -	error = ocfs2_dir_foreach_blk(inode, &file->f_version, ctx, false);
> +	error = ocfs2_dir_foreach_blk(inode, &fp->cookie, ctx, false);
>  
>  	ocfs2_inode_unlock(inode, lock_level);
>  	if (error)
> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> index ccc57038a977..115ab2172820 100644
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -2750,6 +2750,13 @@ static loff_t ocfs2_remap_file_range(struct file *file_in, loff_t pos_in,
>  	return remapped > 0 ? remapped : ret;
>  }
>  
> +static loff_t ocfs2_dir_llseek(struct file *file, loff_t offset, int whence)
> +{
> +	struct ocfs2_file_private *fp = file->private_data;
> +
> +	return generic_llseek_cookie(file, offset, whence, &fp->cookie);
> +}
> +
>  const struct inode_operations ocfs2_file_iops = {
>  	.setattr	= ocfs2_setattr,
>  	.getattr	= ocfs2_getattr,
> @@ -2797,7 +2804,7 @@ const struct file_operations ocfs2_fops = {
>  
>  WRAP_DIR_ITER(ocfs2_readdir) // FIXME!
>  const struct file_operations ocfs2_dops = {
> -	.llseek		= generic_file_llseek,
> +	.llseek		= ocfs2_dir_llseek,
>  	.read		= generic_read_dir,
>  	.iterate_shared	= shared_ocfs2_readdir,
>  	.fsync		= ocfs2_sync_file,
> @@ -2843,7 +2850,7 @@ const struct file_operations ocfs2_fops_no_plocks = {
>  };
>  
>  const struct file_operations ocfs2_dops_no_plocks = {
> -	.llseek		= generic_file_llseek,
> +	.llseek		= ocfs2_dir_llseek,
>  	.read		= generic_read_dir,
>  	.iterate_shared	= shared_ocfs2_readdir,
>  	.fsync		= ocfs2_sync_file,
> diff --git a/fs/ocfs2/file.h b/fs/ocfs2/file.h
> index 8e53e4ac1120..41e65e45a9f3 100644
> --- a/fs/ocfs2/file.h
> +++ b/fs/ocfs2/file.h
> @@ -20,6 +20,7 @@ struct ocfs2_alloc_context;
>  enum ocfs2_alloc_restarted;
>  
>  struct ocfs2_file_private {
> +	u64			cookie;
>  	struct file		*fp_file;
>  	struct mutex		fp_mutex;
>  	struct ocfs2_lock_res	fp_flock;
> 
> -- 
> 2.45.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RFC 14/20] proc: store cookie in private data

[Christian Brauner]

Store the cookie to detect concurrent seeks on directories in
file->private_data.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/proc/base.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 72a1acd03675..8a8aab6b9801 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -3870,12 +3870,12 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx)
 	if (!dir_emit_dots(file, ctx))
 		return 0;
 
-	/* f_version caches the tgid value that the last readdir call couldn't
-	 * return. lseek aka telldir automagically resets f_version to 0.
+	/* We cache the tgid value that the last readdir call couldn't
+	 * return and lseek resets it to 0.
 	 */
 	ns = proc_pid_ns(inode->i_sb);
-	tid = (int)file->f_version;
-	file->f_version = 0;
+	tid = (int)(intptr_t)file->private_data;
+	file->private_data = NULL;
 	for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns);
 	     task;
 	     task = next_tid(task), ctx->pos++) {
@@ -3890,7 +3890,7 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx)
 				proc_task_instantiate, task, NULL)) {
 			/* returning this tgid failed, save it as the first
 			 * pid for the next readir call */
-			file->f_version = (u64)tid;
+			file->private_data = (void *)(intptr_t)tid;
 			put_task_struct(task);
 			break;
 		}
@@ -3915,6 +3915,12 @@ static int proc_task_getattr(struct mnt_idmap *idmap,
 	return 0;
 }
 
+static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence)
+{
+	return generic_llseek_cookie(file, offset, whence,
+				     (u64 *)(uintptr_t)&file->private_data);
+}
+
 static const struct inode_operations proc_task_inode_operations = {
 	.lookup		= proc_task_lookup,
 	.getattr	= proc_task_getattr,
@@ -3925,7 +3931,7 @@ static const struct inode_operations proc_task_inode_operations = {
 static const struct file_operations proc_task_operations = {
 	.read		= generic_read_dir,
 	.iterate_shared	= proc_task_readdir,
-	.llseek		= generic_file_llseek,
+	.llseek		= proc_dir_llseek,
 };
 
 void __init set_proc_pid_nlink(void)

-- 
2.45.2

Re: [PATCH RFC 14/20] proc: store cookie in private data

[Christian Brauner]

On Fri, Aug 30, 2024 at 03:04:55PM GMT, Christian Brauner wrote:
> Store the cookie to detect concurrent seeks on directories in
> file->private_data.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
>  fs/proc/base.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 72a1acd03675..8a8aab6b9801 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -3870,12 +3870,12 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx)
>  	if (!dir_emit_dots(file, ctx))
>  		return 0;
>  
> -	/* f_version caches the tgid value that the last readdir call couldn't
> -	 * return. lseek aka telldir automagically resets f_version to 0.
> +	/* We cache the tgid value that the last readdir call couldn't
> +	 * return and lseek resets it to 0.
>  	 */
>  	ns = proc_pid_ns(inode->i_sb);
> -	tid = (int)file->f_version;
> -	file->f_version = 0;
> +	tid = (int)(intptr_t)file->private_data;
> +	file->private_data = NULL;
>  	for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns);
>  	     task;
>  	     task = next_tid(task), ctx->pos++) {
> @@ -3890,7 +3890,7 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx)
>  				proc_task_instantiate, task, NULL)) {
>  			/* returning this tgid failed, save it as the first
>  			 * pid for the next readir call */
> -			file->f_version = (u64)tid;
> +			file->private_data = (void *)(intptr_t)tid;
>  			put_task_struct(task);
>  			break;
>  		}
> @@ -3915,6 +3915,12 @@ static int proc_task_getattr(struct mnt_idmap *idmap,
>  	return 0;
>  }
>  
> +static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence)
> +{
> +	return generic_llseek_cookie(file, offset, whence,
> +				     (u64 *)(uintptr_t)&file->private_data);

Btw, this is fixed in-tree (I did send out an unfixed version):

static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence)
{
	u64 cookie = 1;
	loff_t off;

	off = generic_llseek_cookie(file, offset, whence, &cookie);
	if (!cookie)
		file->private_data = NULL; /* serialized by f_pos_lock */
	return off;
}

Re: [PATCH RFC 14/20] proc: store cookie in private data

[Jan Kara]

On Tue 03-09-24 13:34:30, Christian Brauner wrote:
> On Fri, Aug 30, 2024 at 03:04:55PM GMT, Christian Brauner wrote:
> > Store the cookie to detect concurrent seeks on directories in
> > file->private_data.
> > 
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > ---
> >  fs/proc/base.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index 72a1acd03675..8a8aab6b9801 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -3870,12 +3870,12 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx)
> >  	if (!dir_emit_dots(file, ctx))
> >  		return 0;
> >  
> > -	/* f_version caches the tgid value that the last readdir call couldn't
> > -	 * return. lseek aka telldir automagically resets f_version to 0.
> > +	/* We cache the tgid value that the last readdir call couldn't
> > +	 * return and lseek resets it to 0.
> >  	 */
> >  	ns = proc_pid_ns(inode->i_sb);
> > -	tid = (int)file->f_version;
> > -	file->f_version = 0;
> > +	tid = (int)(intptr_t)file->private_data;
> > +	file->private_data = NULL;
> >  	for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns);
> >  	     task;
> >  	     task = next_tid(task), ctx->pos++) {
> > @@ -3890,7 +3890,7 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx)
> >  				proc_task_instantiate, task, NULL)) {
> >  			/* returning this tgid failed, save it as the first
> >  			 * pid for the next readir call */
> > -			file->f_version = (u64)tid;
> > +			file->private_data = (void *)(intptr_t)tid;
> >  			put_task_struct(task);
> >  			break;
> >  		}
> > @@ -3915,6 +3915,12 @@ static int proc_task_getattr(struct mnt_idmap *idmap,
> >  	return 0;
> >  }
> >  
> > +static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence)
> > +{
> > +	return generic_llseek_cookie(file, offset, whence,
> > +				     (u64 *)(uintptr_t)&file->private_data);
> 
> Btw, this is fixed in-tree (I did send out an unfixed version):
> 
> static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence)
> {
> 	u64 cookie = 1;
> 	loff_t off;
> 
> 	off = generic_llseek_cookie(file, offset, whence, &cookie);
> 	if (!cookie)
> 		file->private_data = NULL; /* serialized by f_pos_lock */
> 	return off;
> }

Ah, midair collision :). This looks better just why don't you store the
cookie unconditionally in file->private_data? This way proc_dir_llseek()
makes assumptions about how generic_llseek_cookie() uses the cookie which
unnecessarily spreads internal VFS knowledge into filesystems...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH RFC 14/20] proc: store cookie in private data

[Christian Brauner]

On Tue, Sep 03, 2024 at 03:35:48PM GMT, Jan Kara wrote:
> On Tue 03-09-24 13:34:30, Christian Brauner wrote:
> > On Fri, Aug 30, 2024 at 03:04:55PM GMT, Christian Brauner wrote:
> > > Store the cookie to detect concurrent seeks on directories in
> > > file->private_data.
> > > 
> > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > > ---
> > >  fs/proc/base.c | 18 ++++++++++++------
> > >  1 file changed, 12 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > > index 72a1acd03675..8a8aab6b9801 100644
> > > --- a/fs/proc/base.c
> > > +++ b/fs/proc/base.c
> > > @@ -3870,12 +3870,12 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx)
> > >  	if (!dir_emit_dots(file, ctx))
> > >  		return 0;
> > >  
> > > -	/* f_version caches the tgid value that the last readdir call couldn't
> > > -	 * return. lseek aka telldir automagically resets f_version to 0.
> > > +	/* We cache the tgid value that the last readdir call couldn't
> > > +	 * return and lseek resets it to 0.
> > >  	 */
> > >  	ns = proc_pid_ns(inode->i_sb);
> > > -	tid = (int)file->f_version;
> > > -	file->f_version = 0;
> > > +	tid = (int)(intptr_t)file->private_data;
> > > +	file->private_data = NULL;
> > >  	for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns);
> > >  	     task;
> > >  	     task = next_tid(task), ctx->pos++) {
> > > @@ -3890,7 +3890,7 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx)
> > >  				proc_task_instantiate, task, NULL)) {
> > >  			/* returning this tgid failed, save it as the first
> > >  			 * pid for the next readir call */
> > > -			file->f_version = (u64)tid;
> > > +			file->private_data = (void *)(intptr_t)tid;
> > >  			put_task_struct(task);
> > >  			break;
> > >  		}
> > > @@ -3915,6 +3915,12 @@ static int proc_task_getattr(struct mnt_idmap *idmap,
> > >  	return 0;
> > >  }
> > >  
> > > +static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence)
> > > +{
> > > +	return generic_llseek_cookie(file, offset, whence,
> > > +				     (u64 *)(uintptr_t)&file->private_data);
> > 
> > Btw, this is fixed in-tree (I did send out an unfixed version):
> > 
> > static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence)
> > {
> > 	u64 cookie = 1;
> > 	loff_t off;
> > 
> > 	off = generic_llseek_cookie(file, offset, whence, &cookie);
> > 	if (!cookie)
> > 		file->private_data = NULL; /* serialized by f_pos_lock */
> > 	return off;
> > }
> 
> Ah, midair collision :). This looks better just why don't you store the
> cookie unconditionally in file->private_data? This way proc_dir_llseek()
> makes assumptions about how generic_llseek_cookie() uses the cookie which
> unnecessarily spreads internal VFS knowledge into filesystems...

I tried to avoid an allocation for procfs (I assume that's what you're
getting at). That's basically all.

Re: [PATCH RFC 14/20] proc: store cookie in private data

[Jan Kara]

On Tue 03-09-24 16:00:56, Christian Brauner wrote:
> On Tue, Sep 03, 2024 at 03:35:48PM GMT, Jan Kara wrote:
> > On Tue 03-09-24 13:34:30, Christian Brauner wrote:
> > > On Fri, Aug 30, 2024 at 03:04:55PM GMT, Christian Brauner wrote:
> > > > Store the cookie to detect concurrent seeks on directories in
> > > > file->private_data.
> > > > 
> > > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > > > ---
> > > >  fs/proc/base.c | 18 ++++++++++++------
> > > >  1 file changed, 12 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > > > index 72a1acd03675..8a8aab6b9801 100644
> > > > --- a/fs/proc/base.c
> > > > +++ b/fs/proc/base.c
> > > > @@ -3870,12 +3870,12 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx)
> > > >  	if (!dir_emit_dots(file, ctx))
> > > >  		return 0;
> > > >  
> > > > -	/* f_version caches the tgid value that the last readdir call couldn't
> > > > -	 * return. lseek aka telldir automagically resets f_version to 0.
> > > > +	/* We cache the tgid value that the last readdir call couldn't
> > > > +	 * return and lseek resets it to 0.
> > > >  	 */
> > > >  	ns = proc_pid_ns(inode->i_sb);
> > > > -	tid = (int)file->f_version;
> > > > -	file->f_version = 0;
> > > > +	tid = (int)(intptr_t)file->private_data;
> > > > +	file->private_data = NULL;
> > > >  	for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns);
> > > >  	     task;
> > > >  	     task = next_tid(task), ctx->pos++) {
> > > > @@ -3890,7 +3890,7 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx)
> > > >  				proc_task_instantiate, task, NULL)) {
> > > >  			/* returning this tgid failed, save it as the first
> > > >  			 * pid for the next readir call */
> > > > -			file->f_version = (u64)tid;
> > > > +			file->private_data = (void *)(intptr_t)tid;
> > > >  			put_task_struct(task);
> > > >  			break;
> > > >  		}
> > > > @@ -3915,6 +3915,12 @@ static int proc_task_getattr(struct mnt_idmap *idmap,
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence)
> > > > +{
> > > > +	return generic_llseek_cookie(file, offset, whence,
> > > > +				     (u64 *)(uintptr_t)&file->private_data);
> > > 
> > > Btw, this is fixed in-tree (I did send out an unfixed version):
> > > 
> > > static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence)
> > > {
> > > 	u64 cookie = 1;
> > > 	loff_t off;
> > > 
> > > 	off = generic_llseek_cookie(file, offset, whence, &cookie);
> > > 	if (!cookie)
> > > 		file->private_data = NULL; /* serialized by f_pos_lock */
> > > 	return off;
> > > }
> > 
> > Ah, midair collision :). This looks better just why don't you store the
> > cookie unconditionally in file->private_data? This way proc_dir_llseek()
> > makes assumptions about how generic_llseek_cookie() uses the cookie which
> > unnecessarily spreads internal VFS knowledge into filesystems...
> 
> I tried to avoid an allocation for procfs (I assume that's what you're
> getting at). That's basically all.

Yes, I just meant I'd find it safer to have:

static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence)
{
	u64 cookie = (u64)file->private_data;
	loff_t off;

	off = generic_llseek_cookie(file, offset, whence, &cookie);
	file->private_data = (void *)cookie; /* serialized by f_pos_lock */
	return off;
}

So that we don't presume what generic_llseek_cookie() can do with the
cookie.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH RFC 14/20] proc: store cookie in private data

[Christian Brauner]

On Wed, Sep 04, 2024 at 04:16:07PM GMT, Jan Kara wrote:
> On Tue 03-09-24 16:00:56, Christian Brauner wrote:
> > On Tue, Sep 03, 2024 at 03:35:48PM GMT, Jan Kara wrote:
> > > On Tue 03-09-24 13:34:30, Christian Brauner wrote:
> > > > On Fri, Aug 30, 2024 at 03:04:55PM GMT, Christian Brauner wrote:
> > > > > Store the cookie to detect concurrent seeks on directories in
> > > > > file->private_data.
> > > > > 
> > > > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > > > > ---
> > > > >  fs/proc/base.c | 18 ++++++++++++------
> > > > >  1 file changed, 12 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > > > > index 72a1acd03675..8a8aab6b9801 100644
> > > > > --- a/fs/proc/base.c
> > > > > +++ b/fs/proc/base.c
> > > > > @@ -3870,12 +3870,12 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx)
> > > > >  	if (!dir_emit_dots(file, ctx))
> > > > >  		return 0;
> > > > >  
> > > > > -	/* f_version caches the tgid value that the last readdir call couldn't
> > > > > -	 * return. lseek aka telldir automagically resets f_version to 0.
> > > > > +	/* We cache the tgid value that the last readdir call couldn't
> > > > > +	 * return and lseek resets it to 0.
> > > > >  	 */
> > > > >  	ns = proc_pid_ns(inode->i_sb);
> > > > > -	tid = (int)file->f_version;
> > > > > -	file->f_version = 0;
> > > > > +	tid = (int)(intptr_t)file->private_data;
> > > > > +	file->private_data = NULL;
> > > > >  	for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns);
> > > > >  	     task;
> > > > >  	     task = next_tid(task), ctx->pos++) {
> > > > > @@ -3890,7 +3890,7 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx)
> > > > >  				proc_task_instantiate, task, NULL)) {
> > > > >  			/* returning this tgid failed, save it as the first
> > > > >  			 * pid for the next readir call */
> > > > > -			file->f_version = (u64)tid;
> > > > > +			file->private_data = (void *)(intptr_t)tid;
> > > > >  			put_task_struct(task);
> > > > >  			break;
> > > > >  		}
> > > > > @@ -3915,6 +3915,12 @@ static int proc_task_getattr(struct mnt_idmap *idmap,
> > > > >  	return 0;
> > > > >  }
> > > > >  
> > > > > +static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence)
> > > > > +{
> > > > > +	return generic_llseek_cookie(file, offset, whence,
> > > > > +				     (u64 *)(uintptr_t)&file->private_data);
> > > > 
> > > > Btw, this is fixed in-tree (I did send out an unfixed version):
> > > > 
> > > > static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence)
> > > > {
> > > > 	u64 cookie = 1;
> > > > 	loff_t off;
> > > > 
> > > > 	off = generic_llseek_cookie(file, offset, whence, &cookie);
> > > > 	if (!cookie)
> > > > 		file->private_data = NULL; /* serialized by f_pos_lock */
> > > > 	return off;
> > > > }
> > > 
> > > Ah, midair collision :). This looks better just why don't you store the
> > > cookie unconditionally in file->private_data? This way proc_dir_llseek()
> > > makes assumptions about how generic_llseek_cookie() uses the cookie which
> > > unnecessarily spreads internal VFS knowledge into filesystems...
> > 
> > I tried to avoid an allocation for procfs (I assume that's what you're
> > getting at). That's basically all.
> 
> Yes, I just meant I'd find it safer to have:
> 
> static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence)
> {
> 	u64 cookie = (u64)file->private_data;
> 	loff_t off;
> 
> 	off = generic_llseek_cookie(file, offset, whence, &cookie);
> 	file->private_data = (void *)cookie; /* serialized by f_pos_lock */
> 	return off;
> }
> 
> So that we don't presume what generic_llseek_cookie() can do with the
> cookie.

I switched to that, thanks!

Re: [PATCH RFC 14/20] proc: store cookie in private data

[Jan Kara]

On Fri 30-08-24 15:04:55, Christian Brauner wrote:
> Store the cookie to detect concurrent seeks on directories in
> file->private_data.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

...

>  	ns = proc_pid_ns(inode->i_sb);
> -	tid = (int)file->f_version;
> -	file->f_version = 0;
> +	tid = (int)(intptr_t)file->private_data;

What's the point of first casting to intptr_t?

> @@ -3890,7 +3890,7 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx)
>  				proc_task_instantiate, task, NULL)) {
>  			/* returning this tgid failed, save it as the first
>  			 * pid for the next readir call */
> -			file->f_version = (u64)tid;
> +			file->private_data = (void *)(intptr_t)tid;

The same here...

>  			put_task_struct(task);
>  			break;
>  		}
> @@ -3915,6 +3915,12 @@ static int proc_task_getattr(struct mnt_idmap *idmap,
>  	return 0;
>  }
>  
> +static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence)
> +{
> +	return generic_llseek_cookie(file, offset, whence,
> +				     (u64 *)(uintptr_t)&file->private_data);

So this I think is wrong for 32-bit archs because generic_llseek_cookie()
will store 8 bytes there while file->private_data has only 4 bytes...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RFC 15/20] udf: store cookie in private data

[Christian Brauner]

Store the cookie to detect concurrent seeks on directories in
file->private_data.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/udf/dir.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/fs/udf/dir.c b/fs/udf/dir.c
index f94f45fe2c91..5023dfe191e8 100644
--- a/fs/udf/dir.c
+++ b/fs/udf/dir.c
@@ -60,7 +60,7 @@ static int udf_readdir(struct file *file, struct dir_context *ctx)
 	 * identifying beginning of dir entry (names are under user control),
 	 * we need to scan the directory from the beginning.
 	 */
-	if (!inode_eq_iversion(dir, file->f_version)) {
+	if (!inode_eq_iversion(dir, *(u64 *)file->private_data)) {
 		emit_pos = nf_pos;
 		nf_pos = 0;
 	} else {
@@ -122,15 +122,37 @@ static int udf_readdir(struct file *file, struct dir_context *ctx)
 	udf_fiiter_release(&iter);
 out:
 	if (pos_valid)
-		file->f_version = inode_query_iversion(dir);
+		*(u64 *)file->private_data = inode_query_iversion(dir);
 	kfree(fname);
 
 	return ret;
 }
 
+static int udf_dir_open(struct inode *inode, struct file *file)
+{
+	file->private_data = kzalloc(sizeof(u64), GFP_KERNEL);
+	if (!file->private_data)
+		return -ENOMEM;
+	return 0;
+}
+
+static int udf_dir_release(struct inode *inode, struct file *file)
+{
+	kfree(file->private_data);
+	return 0;
+}
+
+static loff_t udf_dir_llseek(struct file *file, loff_t offset, int whence)
+{
+	return generic_llseek_cookie(file, offset, whence,
+				     (u64 *)file->private_data);
+}
+
 /* readdir and lookup functions */
 const struct file_operations udf_dir_operations = {
-	.llseek			= generic_file_llseek,
+	.open			= udf_dir_open,
+	.release		= udf_dir_release,
+	.llseek			= udf_dir_llseek,
 	.read			= generic_read_dir,
 	.iterate_shared		= udf_readdir,
 	.unlocked_ioctl		= udf_ioctl,

-- 
2.45.2

Re: [PATCH RFC 15/20] udf: store cookie in private data

[Jan Kara]

On Fri 30-08-24 15:04:56, Christian Brauner wrote:
> Store the cookie to detect concurrent seeks on directories in
> file->private_data.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/udf/dir.c | 28 +++++++++++++++++++++++++---
>  1 file changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/udf/dir.c b/fs/udf/dir.c
> index f94f45fe2c91..5023dfe191e8 100644
> --- a/fs/udf/dir.c
> +++ b/fs/udf/dir.c
> @@ -60,7 +60,7 @@ static int udf_readdir(struct file *file, struct dir_context *ctx)
>  	 * identifying beginning of dir entry (names are under user control),
>  	 * we need to scan the directory from the beginning.
>  	 */
> -	if (!inode_eq_iversion(dir, file->f_version)) {
> +	if (!inode_eq_iversion(dir, *(u64 *)file->private_data)) {
>  		emit_pos = nf_pos;
>  		nf_pos = 0;
>  	} else {
> @@ -122,15 +122,37 @@ static int udf_readdir(struct file *file, struct dir_context *ctx)
>  	udf_fiiter_release(&iter);
>  out:
>  	if (pos_valid)
> -		file->f_version = inode_query_iversion(dir);
> +		*(u64 *)file->private_data = inode_query_iversion(dir);
>  	kfree(fname);
>  
>  	return ret;
>  }
>  
> +static int udf_dir_open(struct inode *inode, struct file *file)
> +{
> +	file->private_data = kzalloc(sizeof(u64), GFP_KERNEL);
> +	if (!file->private_data)
> +		return -ENOMEM;
> +	return 0;
> +}
> +
> +static int udf_dir_release(struct inode *inode, struct file *file)
> +{
> +	kfree(file->private_data);
> +	return 0;
> +}
> +
> +static loff_t udf_dir_llseek(struct file *file, loff_t offset, int whence)
> +{
> +	return generic_llseek_cookie(file, offset, whence,
> +				     (u64 *)file->private_data);
> +}
> +
>  /* readdir and lookup functions */
>  const struct file_operations udf_dir_operations = {
> -	.llseek			= generic_file_llseek,
> +	.open			= udf_dir_open,
> +	.release		= udf_dir_release,
> +	.llseek			= udf_dir_llseek,
>  	.read			= generic_read_dir,
>  	.iterate_shared		= udf_readdir,
>  	.unlocked_ioctl		= udf_ioctl,
> 
> -- 
> 2.45.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RFC 16/20] ufs: store cookie in private data

[Christian Brauner]

Store the cookie to detect concurrent seeks on directories in
file->private_data.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/ufs/dir.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/fs/ufs/dir.c b/fs/ufs/dir.c
index 61f25d3cf3f7..335f0ae529b4 100644
--- a/fs/ufs/dir.c
+++ b/fs/ufs/dir.c
@@ -435,7 +435,7 @@ ufs_readdir(struct file *file, struct dir_context *ctx)
 	unsigned long n = pos >> PAGE_SHIFT;
 	unsigned long npages = dir_pages(inode);
 	unsigned chunk_mask = ~(UFS_SB(sb)->s_uspi->s_dirblksize - 1);
-	bool need_revalidate = !inode_eq_iversion(inode, file->f_version);
+	bool need_revalidate = !inode_eq_iversion(inode, *(u64 *)file->private_data);
 	unsigned flags = UFS_SB(sb)->s_flags;
 
 	UFSD("BEGIN\n");
@@ -462,7 +462,7 @@ ufs_readdir(struct file *file, struct dir_context *ctx)
 				offset = ufs_validate_entry(sb, kaddr, offset, chunk_mask);
 				ctx->pos = (n<<PAGE_SHIFT) + offset;
 			}
-			file->f_version = inode_query_iversion(inode);
+			*(u64 *)file->private_data = inode_query_iversion(inode);
 			need_revalidate = false;
 		}
 		de = (struct ufs_dir_entry *)(kaddr+offset);
@@ -646,9 +646,31 @@ int ufs_empty_dir(struct inode * inode)
 	return 0;
 }
 
+static int ufs_dir_open(struct inode *inode, struct file *file)
+{
+	file->private_data = kzalloc(sizeof(u64), GFP_KERNEL);
+	if (!file->private_data)
+		return -ENOMEM;
+	return 0;
+}
+
+static int ufs_dir_release(struct inode *inode, struct file *file)
+{
+	kfree(file->private_data);
+	return 0;
+}
+
+static loff_t ufs_dir_llseek(struct file *file, loff_t offset, int whence)
+{
+	return generic_llseek_cookie(file, offset, whence,
+				     (u64 *)file->private_data);
+}
+
 const struct file_operations ufs_dir_operations = {
+	.open		= ufs_dir_open,
+	.release	= ufs_dir_release,
 	.read		= generic_read_dir,
 	.iterate_shared	= ufs_readdir,
 	.fsync		= generic_file_fsync,
-	.llseek		= generic_file_llseek,
+	.llseek		= ufs_dir_llseek,
 };

-- 
2.45.2

Re: [PATCH RFC 16/20] ufs: store cookie in private data

[Jan Kara]

On Fri 30-08-24 15:04:57, Christian Brauner wrote:
> Store the cookie to detect concurrent seeks on directories in
> file->private_data.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ufs/dir.c | 28 +++++++++++++++++++++++++---
>  1 file changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ufs/dir.c b/fs/ufs/dir.c
> index 61f25d3cf3f7..335f0ae529b4 100644
> --- a/fs/ufs/dir.c
> +++ b/fs/ufs/dir.c
> @@ -435,7 +435,7 @@ ufs_readdir(struct file *file, struct dir_context *ctx)
>  	unsigned long n = pos >> PAGE_SHIFT;
>  	unsigned long npages = dir_pages(inode);
>  	unsigned chunk_mask = ~(UFS_SB(sb)->s_uspi->s_dirblksize - 1);
> -	bool need_revalidate = !inode_eq_iversion(inode, file->f_version);
> +	bool need_revalidate = !inode_eq_iversion(inode, *(u64 *)file->private_data);
>  	unsigned flags = UFS_SB(sb)->s_flags;
>  
>  	UFSD("BEGIN\n");
> @@ -462,7 +462,7 @@ ufs_readdir(struct file *file, struct dir_context *ctx)
>  				offset = ufs_validate_entry(sb, kaddr, offset, chunk_mask);
>  				ctx->pos = (n<<PAGE_SHIFT) + offset;
>  			}
> -			file->f_version = inode_query_iversion(inode);
> +			*(u64 *)file->private_data = inode_query_iversion(inode);
>  			need_revalidate = false;
>  		}
>  		de = (struct ufs_dir_entry *)(kaddr+offset);
> @@ -646,9 +646,31 @@ int ufs_empty_dir(struct inode * inode)
>  	return 0;
>  }
>  
> +static int ufs_dir_open(struct inode *inode, struct file *file)
> +{
> +	file->private_data = kzalloc(sizeof(u64), GFP_KERNEL);
> +	if (!file->private_data)
> +		return -ENOMEM;
> +	return 0;
> +}
> +
> +static int ufs_dir_release(struct inode *inode, struct file *file)
> +{
> +	kfree(file->private_data);
> +	return 0;
> +}
> +
> +static loff_t ufs_dir_llseek(struct file *file, loff_t offset, int whence)
> +{
> +	return generic_llseek_cookie(file, offset, whence,
> +				     (u64 *)file->private_data);
> +}
> +
>  const struct file_operations ufs_dir_operations = {
> +	.open		= ufs_dir_open,
> +	.release	= ufs_dir_release,
>  	.read		= generic_read_dir,
>  	.iterate_shared	= ufs_readdir,
>  	.fsync		= generic_file_fsync,
> -	.llseek		= generic_file_llseek,
> +	.llseek		= ufs_dir_llseek,
>  };
> 
> -- 
> 2.45.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RFC 17/20] ubifs: store cookie in private data

[Christian Brauner]

Store the cookie to detect concurrent seeks on directories in
file->private_data.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/ubifs/dir.c | 65 ++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 47 insertions(+), 18 deletions(-)

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index c77ea57fe696..76bcafa92200 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -555,6 +555,11 @@ static unsigned int vfs_dent_type(uint8_t type)
 	return 0;
 }
 
+struct ubifs_dir_data {
+	struct ubifs_dent_node *dent;
+	u64 cookie;
+};
+
 /*
  * The classical Unix view for directory is that it is a linear array of
  * (name, inode number) entries. Linux/VFS assumes this model as well.
@@ -582,6 +587,7 @@ static int ubifs_readdir(struct file *file, struct dir_context *ctx)
 	struct inode *dir = file_inode(file);
 	struct ubifs_info *c = dir->i_sb->s_fs_info;
 	bool encrypted = IS_ENCRYPTED(dir);
+	struct ubifs_dir_data *data = file->private_data;
 
 	dbg_gen("dir ino %lu, f_pos %#llx", dir->i_ino, ctx->pos);
 
@@ -604,27 +610,27 @@ static int ubifs_readdir(struct file *file, struct dir_context *ctx)
 		fstr_real_len = fstr.len;
 	}
 
-	if (file->f_version == 0) {
+	if (data->cookie == 0) {
 		/*
-		 * The file was seek'ed, which means that @file->private_data
+		 * The file was seek'ed, which means that @data->dent
 		 * is now invalid. This may also be just the first
 		 * 'ubifs_readdir()' invocation, in which case
-		 * @file->private_data is NULL, and the below code is
+		 * @data->dent is NULL, and the below code is
 		 * basically a no-op.
 		 */
-		kfree(file->private_data);
-		file->private_data = NULL;
+		kfree(data->dent);
+		data->dent = NULL;
 	}
 
 	/*
-	 * 'generic_file_llseek()' unconditionally sets @file->f_version to
-	 * zero, and we use this for detecting whether the file was seek'ed.
+	 * 'ubifs_dir_llseek()' sets @data->cookie to zero, and we use this
+	 * for detecting whether the file was seek'ed.
 	 */
-	file->f_version = 1;
+	data->cookie = 1;
 
 	/* File positions 0 and 1 correspond to "." and ".." */
 	if (ctx->pos < 2) {
-		ubifs_assert(c, !file->private_data);
+		ubifs_assert(c, !data->dent);
 		if (!dir_emit_dots(file, ctx)) {
 			if (encrypted)
 				fscrypt_fname_free_buffer(&fstr);
@@ -641,10 +647,10 @@ static int ubifs_readdir(struct file *file, struct dir_context *ctx)
 		}
 
 		ctx->pos = key_hash_flash(c, &dent->key);
-		file->private_data = dent;
+		data->dent = dent;
 	}
 
-	dent = file->private_data;
+	dent = data->dent;
 	if (!dent) {
 		/*
 		 * The directory was seek'ed to and is now readdir'ed.
@@ -658,7 +664,7 @@ static int ubifs_readdir(struct file *file, struct dir_context *ctx)
 			goto out;
 		}
 		ctx->pos = key_hash_flash(c, &dent->key);
-		file->private_data = dent;
+		data->dent = dent;
 	}
 
 	while (1) {
@@ -701,15 +707,15 @@ static int ubifs_readdir(struct file *file, struct dir_context *ctx)
 			goto out;
 		}
 
-		kfree(file->private_data);
+		kfree(data->dent);
 		ctx->pos = key_hash_flash(c, &dent->key);
-		file->private_data = dent;
+		data->dent = dent;
 		cond_resched();
 	}
 
 out:
-	kfree(file->private_data);
-	file->private_data = NULL;
+	kfree(data->dent);
+	data->dent = NULL;
 
 	if (encrypted)
 		fscrypt_fname_free_buffer(&fstr);
@@ -733,7 +739,10 @@ static int ubifs_readdir(struct file *file, struct dir_context *ctx)
 /* Free saved readdir() state when the directory is closed */
 static int ubifs_dir_release(struct inode *dir, struct file *file)
 {
-	kfree(file->private_data);
+	struct ubifs_dir_data *data = file->private_data;
+
+	kfree(data->dent);
+	kfree(data);
 	file->private_data = NULL;
 	return 0;
 }
@@ -1712,6 +1721,24 @@ int ubifs_getattr(struct mnt_idmap *idmap, const struct path *path,
 	return 0;
 }
 
+static int ubifs_dir_open(struct inode *inode, struct file *file)
+{
+	struct ubifs_dir_data	*data;
+
+	data = kzalloc(sizeof(struct ubifs_dir_data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+	file->private_data = data;
+	return 0;
+}
+
+static loff_t ubifs_dir_llseek(struct file *file, loff_t offset, int whence)
+{
+	struct ubifs_dir_data *data = file->private_data;
+
+	return generic_llseek_cookie(file, offset, whence, &data->cookie);
+}
+
 const struct inode_operations ubifs_dir_inode_operations = {
 	.lookup      = ubifs_lookup,
 	.create      = ubifs_create,
@@ -1732,7 +1759,9 @@ const struct inode_operations ubifs_dir_inode_operations = {
 };
 
 const struct file_operations ubifs_dir_operations = {
-	.llseek         = generic_file_llseek,
+	.open		= ubifs_dir_open,
+	.release	= ubifs_dir_release,
+	.llseek         = ubifs_dir_llseek,
 	.release        = ubifs_dir_release,
 	.read           = generic_read_dir,
 	.iterate_shared = ubifs_readdir,

-- 
2.45.2

Re: [PATCH RFC 17/20] ubifs: store cookie in private data

[Jan Kara]

On Fri 30-08-24 15:04:58, Christian Brauner wrote:
> Store the cookie to detect concurrent seeks on directories in
> file->private_data.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ubifs/dir.c | 65 ++++++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 47 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> index c77ea57fe696..76bcafa92200 100644
> --- a/fs/ubifs/dir.c
> +++ b/fs/ubifs/dir.c
> @@ -555,6 +555,11 @@ static unsigned int vfs_dent_type(uint8_t type)
>  	return 0;
>  }
>  
> +struct ubifs_dir_data {
> +	struct ubifs_dent_node *dent;
> +	u64 cookie;
> +};
> +
>  /*
>   * The classical Unix view for directory is that it is a linear array of
>   * (name, inode number) entries. Linux/VFS assumes this model as well.
> @@ -582,6 +587,7 @@ static int ubifs_readdir(struct file *file, struct dir_context *ctx)
>  	struct inode *dir = file_inode(file);
>  	struct ubifs_info *c = dir->i_sb->s_fs_info;
>  	bool encrypted = IS_ENCRYPTED(dir);
> +	struct ubifs_dir_data *data = file->private_data;
>  
>  	dbg_gen("dir ino %lu, f_pos %#llx", dir->i_ino, ctx->pos);
>  
> @@ -604,27 +610,27 @@ static int ubifs_readdir(struct file *file, struct dir_context *ctx)
>  		fstr_real_len = fstr.len;
>  	}
>  
> -	if (file->f_version == 0) {
> +	if (data->cookie == 0) {
>  		/*
> -		 * The file was seek'ed, which means that @file->private_data
> +		 * The file was seek'ed, which means that @data->dent
>  		 * is now invalid. This may also be just the first
>  		 * 'ubifs_readdir()' invocation, in which case
> -		 * @file->private_data is NULL, and the below code is
> +		 * @data->dent is NULL, and the below code is
>  		 * basically a no-op.
>  		 */
> -		kfree(file->private_data);
> -		file->private_data = NULL;
> +		kfree(data->dent);
> +		data->dent = NULL;
>  	}
>  
>  	/*
> -	 * 'generic_file_llseek()' unconditionally sets @file->f_version to
> -	 * zero, and we use this for detecting whether the file was seek'ed.
> +	 * 'ubifs_dir_llseek()' sets @data->cookie to zero, and we use this
> +	 * for detecting whether the file was seek'ed.
>  	 */
> -	file->f_version = 1;
> +	data->cookie = 1;
>  
>  	/* File positions 0 and 1 correspond to "." and ".." */
>  	if (ctx->pos < 2) {
> -		ubifs_assert(c, !file->private_data);
> +		ubifs_assert(c, !data->dent);
>  		if (!dir_emit_dots(file, ctx)) {
>  			if (encrypted)
>  				fscrypt_fname_free_buffer(&fstr);
> @@ -641,10 +647,10 @@ static int ubifs_readdir(struct file *file, struct dir_context *ctx)
>  		}
>  
>  		ctx->pos = key_hash_flash(c, &dent->key);
> -		file->private_data = dent;
> +		data->dent = dent;
>  	}
>  
> -	dent = file->private_data;
> +	dent = data->dent;
>  	if (!dent) {
>  		/*
>  		 * The directory was seek'ed to and is now readdir'ed.
> @@ -658,7 +664,7 @@ static int ubifs_readdir(struct file *file, struct dir_context *ctx)
>  			goto out;
>  		}
>  		ctx->pos = key_hash_flash(c, &dent->key);
> -		file->private_data = dent;
> +		data->dent = dent;
>  	}
>  
>  	while (1) {
> @@ -701,15 +707,15 @@ static int ubifs_readdir(struct file *file, struct dir_context *ctx)
>  			goto out;
>  		}
>  
> -		kfree(file->private_data);
> +		kfree(data->dent);
>  		ctx->pos = key_hash_flash(c, &dent->key);
> -		file->private_data = dent;
> +		data->dent = dent;
>  		cond_resched();
>  	}
>  
>  out:
> -	kfree(file->private_data);
> -	file->private_data = NULL;
> +	kfree(data->dent);
> +	data->dent = NULL;
>  
>  	if (encrypted)
>  		fscrypt_fname_free_buffer(&fstr);
> @@ -733,7 +739,10 @@ static int ubifs_readdir(struct file *file, struct dir_context *ctx)
>  /* Free saved readdir() state when the directory is closed */
>  static int ubifs_dir_release(struct inode *dir, struct file *file)
>  {
> -	kfree(file->private_data);
> +	struct ubifs_dir_data *data = file->private_data;
> +
> +	kfree(data->dent);
> +	kfree(data);
>  	file->private_data = NULL;
>  	return 0;
>  }
> @@ -1712,6 +1721,24 @@ int ubifs_getattr(struct mnt_idmap *idmap, const struct path *path,
>  	return 0;
>  }
>  
> +static int ubifs_dir_open(struct inode *inode, struct file *file)
> +{
> +	struct ubifs_dir_data	*data;
> +
> +	data = kzalloc(sizeof(struct ubifs_dir_data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +	file->private_data = data;
> +	return 0;
> +}
> +
> +static loff_t ubifs_dir_llseek(struct file *file, loff_t offset, int whence)
> +{
> +	struct ubifs_dir_data *data = file->private_data;
> +
> +	return generic_llseek_cookie(file, offset, whence, &data->cookie);
> +}
> +
>  const struct inode_operations ubifs_dir_inode_operations = {
>  	.lookup      = ubifs_lookup,
>  	.create      = ubifs_create,
> @@ -1732,7 +1759,9 @@ const struct inode_operations ubifs_dir_inode_operations = {
>  };
>  
>  const struct file_operations ubifs_dir_operations = {
> -	.llseek         = generic_file_llseek,
> +	.open		= ubifs_dir_open,
> +	.release	= ubifs_dir_release,
> +	.llseek         = ubifs_dir_llseek,
>  	.release        = ubifs_dir_release,
>  	.read           = generic_read_dir,
>  	.iterate_shared = ubifs_readdir,
> 
> -- 
> 2.45.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RFC 18/20] fs: add f_pipe

[Christian Brauner]

Only regular files with FMODE_ATOMIC_POS and directories need
f_pos_lock. Place a new f_pipe member in a union with f_pos_lock
that they can use and make them stop abusing f_version in follow-up
patches.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 include/linux/fs.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3e6b3c1afb31..ca4925008244 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1001,6 +1001,7 @@ static inline int ra_has_index(struct file_ra_state *ra, pgoff_t index)
  * @f_cred: stashed credentials of creator/opener
  * @f_path: path of the file
  * @f_pos_lock: lock protecting file position
+ * @f_pipe: specific to pipes
  * @f_pos: file position
  * @f_version: file version
  * @f_security: LSM security context of this file
@@ -1026,7 +1027,12 @@ struct file {
 	const struct cred		*f_cred;
 	/* --- cacheline 1 boundary (64 bytes) --- */
 	struct path			f_path;
-	struct mutex			f_pos_lock;
+	union {
+		/* regular files (with FMODE_ATOMIC_POS) and directories */
+		struct mutex		f_pos_lock;
+		/* pipes */
+		u64			f_pipe;
+	};
 	loff_t				f_pos;
 	u64				f_version;
 	/* --- cacheline 2 boundary (128 bytes) --- */

-- 
2.45.2

Re: [PATCH RFC 18/20] fs: add f_pipe

[Jan Kara]

On Fri 30-08-24 15:04:59, Christian Brauner wrote:
> Only regular files with FMODE_ATOMIC_POS and directories need
> f_pos_lock. Place a new f_pipe member in a union with f_pos_lock
> that they can use and make them stop abusing f_version in follow-up
> patches.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

What makes me a bit uneasy is that we do mutex_init() on the space in
struct file and then pipe_open() will clobber it. And then we eventually
call mutex_destroy() on the clobbered mutex. I think so far this does no
harm but mostly by luck. I think we need to make sure that when f_pos_lock
is unused, we don't even call mutex_init() / mutex_destroy() on it.

								Honza

> ---
>  include/linux/fs.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3e6b3c1afb31..ca4925008244 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1001,6 +1001,7 @@ static inline int ra_has_index(struct file_ra_state *ra, pgoff_t index)
>   * @f_cred: stashed credentials of creator/opener
>   * @f_path: path of the file
>   * @f_pos_lock: lock protecting file position
> + * @f_pipe: specific to pipes
>   * @f_pos: file position
>   * @f_version: file version
>   * @f_security: LSM security context of this file
> @@ -1026,7 +1027,12 @@ struct file {
>  	const struct cred		*f_cred;
>  	/* --- cacheline 1 boundary (64 bytes) --- */
>  	struct path			f_path;
> -	struct mutex			f_pos_lock;
> +	union {
> +		/* regular files (with FMODE_ATOMIC_POS) and directories */
> +		struct mutex		f_pos_lock;
> +		/* pipes */
> +		u64			f_pipe;
> +	};
>  	loff_t				f_pos;
>  	u64				f_version;
>  	/* --- cacheline 2 boundary (128 bytes) --- */
> 
> -- 
> 2.45.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH RFC 18/20] fs: add f_pipe

[Christian Brauner]

On Tue, Sep 03, 2024 at 03:50:55PM GMT, Jan Kara wrote:
> On Fri 30-08-24 15:04:59, Christian Brauner wrote:
> > Only regular files with FMODE_ATOMIC_POS and directories need
> > f_pos_lock. Place a new f_pipe member in a union with f_pos_lock
> > that they can use and make them stop abusing f_version in follow-up
> > patches.
> > 
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> 
> What makes me a bit uneasy is that we do mutex_init() on the space in
> struct file and then pipe_open() will clobber it. And then we eventually
> call mutex_destroy() on the clobbered mutex. I think so far this does no

We don't call mutex_destroy() on it and don't need to. And calling
mutex_init() is fine precisely because pipes do use it. It would be
really ugly do ensure that mutex_init() isn't called for pipes. But I'll
add a comment.

Re: [PATCH RFC 18/20] fs: add f_pipe

[Jan Kara]

On Tue 03-09-24 16:31:28, Christian Brauner wrote:
> On Tue, Sep 03, 2024 at 03:50:55PM GMT, Jan Kara wrote:
> > On Fri 30-08-24 15:04:59, Christian Brauner wrote:
> > > Only regular files with FMODE_ATOMIC_POS and directories need
> > > f_pos_lock. Place a new f_pipe member in a union with f_pos_lock
> > > that they can use and make them stop abusing f_version in follow-up
> > > patches.
> > > 
> > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > 
> > What makes me a bit uneasy is that we do mutex_init() on the space in
> > struct file and then pipe_open() will clobber it. And then we eventually
> > call mutex_destroy() on the clobbered mutex. I think so far this does no
> 
> We don't call mutex_destroy() on it and don't need to.

Ah, right, we don't bother with that for f_pos_lock.

> And calling mutex_init() is fine precisely because pipes do use it. It
> would be really ugly do ensure that mutex_init() isn't called for pipes.
> But I'll add a comment.

I'm not sure I understand what do you mean by "calling mutex_init() is fine
precisely because pipes do use it." Perhaps you meant "don't use it"?
Otherwise after looking at it for a while I agree the cure would be likely
worse than the disease so a comment is as good as it gets I guess.

								Honza


-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH RFC 18/20] fs: add f_pipe

[Al Viro]

On Fri, Aug 30, 2024 at 03:04:59PM +0200, Christian Brauner wrote:
> Only regular files with FMODE_ATOMIC_POS and directories need
> f_pos_lock. Place a new f_pipe member in a union with f_pos_lock
> that they can use and make them stop abusing f_version in follow-up
> patches.

Not sure I like that - having lseek(2) use a separate primitive
instead of fdget_pos(), grabbing ->f_pos_lock for _everything_ that
has FMODE_LSEEK, directory or no directory, would simplify quite
a few things.  OTOH, that will affect only the explanation of validity -
pipes do *not* have FMODE_LSEEK, so it becomes "fdget_pos() and
fdget_seek() are the only things that might want ->f_pos_lock, and
neither touch it for pipes - fdget_pos() because FMODE_ATOMIC_POS
is not there and fdget_seek() because FMODE_LSEEK isn't".

Oh, well...

[PATCH RFC 19/20] pipe: use f_pipe

[Christian Brauner]

Pipes use f_version to defer poll notifications until a write has been
observed. Since multiple file's refer to the same struct pipe_inode_info
in their ->private_data moving it into their isn't feasible since we
would need to introduce an additional pointer indirection.

However, since pipes don't require f_pos_lock we placed a new f_pipe
member into a union with f_pos_lock that pipes can use. This is similar
to what we already do for struct inode where we have additional fields
per file type. This will allow us to fully remove f_version in the next
step.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/pipe.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index 7dff2aa50a6d..b8f1943c57b9 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -686,7 +686,7 @@ pipe_poll(struct file *filp, poll_table *wait)
 	if (filp->f_mode & FMODE_READ) {
 		if (!pipe_empty(head, tail))
 			mask |= EPOLLIN | EPOLLRDNORM;
-		if (!pipe->writers && filp->f_version != pipe->w_counter)
+		if (!pipe->writers && filp->f_pipe != pipe->w_counter)
 			mask |= EPOLLHUP;
 	}
 
@@ -1108,7 +1108,7 @@ static int fifo_open(struct inode *inode, struct file *filp)
 	bool is_pipe = inode->i_sb->s_magic == PIPEFS_MAGIC;
 	int ret;
 
-	filp->f_version = 0;
+	filp->f_pipe = 0;
 
 	spin_lock(&inode->i_lock);
 	if (inode->i_pipe) {
@@ -1155,7 +1155,7 @@ static int fifo_open(struct inode *inode, struct file *filp)
 			if ((filp->f_flags & O_NONBLOCK)) {
 				/* suppress EPOLLHUP until we have
 				 * seen a writer */
-				filp->f_version = pipe->w_counter;
+				filp->f_pipe = pipe->w_counter;
 			} else {
 				if (wait_for_partner(pipe, &pipe->w_counter))
 					goto err_rd;

-- 
2.45.2

Re: [PATCH RFC 19/20] pipe: use f_pipe

[Jan Kara]

On Fri 30-08-24 15:05:00, Christian Brauner wrote:
> Pipes use f_version to defer poll notifications until a write has been
> observed. Since multiple file's refer to the same struct pipe_inode_info
> in their ->private_data moving it into their isn't feasible since we
> would need to introduce an additional pointer indirection.
> 
> However, since pipes don't require f_pos_lock we placed a new f_pipe
> member into a union with f_pos_lock that pipes can use. This is similar
> to what we already do for struct inode where we have additional fields
> per file type. This will allow us to fully remove f_version in the next
> step.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/pipe.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/pipe.c b/fs/pipe.c
> index 7dff2aa50a6d..b8f1943c57b9 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -686,7 +686,7 @@ pipe_poll(struct file *filp, poll_table *wait)
>  	if (filp->f_mode & FMODE_READ) {
>  		if (!pipe_empty(head, tail))
>  			mask |= EPOLLIN | EPOLLRDNORM;
> -		if (!pipe->writers && filp->f_version != pipe->w_counter)
> +		if (!pipe->writers && filp->f_pipe != pipe->w_counter)
>  			mask |= EPOLLHUP;
>  	}
>  
> @@ -1108,7 +1108,7 @@ static int fifo_open(struct inode *inode, struct file *filp)
>  	bool is_pipe = inode->i_sb->s_magic == PIPEFS_MAGIC;
>  	int ret;
>  
> -	filp->f_version = 0;
> +	filp->f_pipe = 0;
>  
>  	spin_lock(&inode->i_lock);
>  	if (inode->i_pipe) {
> @@ -1155,7 +1155,7 @@ static int fifo_open(struct inode *inode, struct file *filp)
>  			if ((filp->f_flags & O_NONBLOCK)) {
>  				/* suppress EPOLLHUP until we have
>  				 * seen a writer */
> -				filp->f_version = pipe->w_counter;
> +				filp->f_pipe = pipe->w_counter;
>  			} else {
>  				if (wait_for_partner(pipe, &pipe->w_counter))
>  					goto err_rd;
> 
> -- 
> 2.45.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RFC 20/20] fs: remove f_version

[Christian Brauner]

Now that detecting concurrent seeks is done by the filesystems that
require it we can remove f_version and free up 8 bytes for future
extensions.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/read_write.c    | 9 ++++-----
 include/linux/fs.h | 4 +---
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index 47f7b4e32a53..981146f50568 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -62,7 +62,8 @@ static loff_t vfs_setpos_cookie(struct file *file, loff_t offset,
 
 	if (offset != file->f_pos) {
 		file->f_pos = offset;
-		*cookie = 0;
+		if (cookie)
+			*cookie = 0;
 	}
 	return offset;
 }
@@ -81,7 +82,7 @@ static loff_t vfs_setpos_cookie(struct file *file, loff_t offset,
  */
 loff_t vfs_setpos(struct file *file, loff_t offset, loff_t maxsize)
 {
-	return vfs_setpos_cookie(file, offset, maxsize, &file->f_version);
+	return vfs_setpos_cookie(file, offset, maxsize, NULL);
 }
 EXPORT_SYMBOL(vfs_setpos);
 
@@ -362,10 +363,8 @@ loff_t default_llseek(struct file *file, loff_t offset, int whence)
 	}
 	retval = -EINVAL;
 	if (offset >= 0 || unsigned_offsets(file)) {
-		if (offset != file->f_pos) {
+		if (offset != file->f_pos)
 			file->f_pos = offset;
-			file->f_version = 0;
-		}
 		retval = offset;
 	}
 out:
diff --git a/include/linux/fs.h b/include/linux/fs.h
index ca4925008244..7e11ce172140 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1003,7 +1003,6 @@ static inline int ra_has_index(struct file_ra_state *ra, pgoff_t index)
  * @f_pos_lock: lock protecting file position
  * @f_pipe: specific to pipes
  * @f_pos: file position
- * @f_version: file version
  * @f_security: LSM security context of this file
  * @f_owner: file owner
  * @f_wb_err: writeback error
@@ -1034,11 +1033,10 @@ struct file {
 		u64			f_pipe;
 	};
 	loff_t				f_pos;
-	u64				f_version;
-	/* --- cacheline 2 boundary (128 bytes) --- */
 #ifdef CONFIG_SECURITY
 	void				*f_security;
 #endif
+	/* --- cacheline 2 boundary (128 bytes) --- */
 	struct fown_struct		*f_owner;
 	errseq_t			f_wb_err;
 	errseq_t			f_sb_err;

-- 
2.45.2

Re: [PATCH RFC 20/20] fs: remove f_version

[Jan Kara]

On Fri 30-08-24 15:05:01, Christian Brauner wrote:
> Now that detecting concurrent seeks is done by the filesystems that
> require it we can remove f_version and free up 8 bytes for future
> extensions.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/read_write.c    | 9 ++++-----
>  include/linux/fs.h | 4 +---
>  2 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 47f7b4e32a53..981146f50568 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -62,7 +62,8 @@ static loff_t vfs_setpos_cookie(struct file *file, loff_t offset,
>  
>  	if (offset != file->f_pos) {
>  		file->f_pos = offset;
> -		*cookie = 0;
> +		if (cookie)
> +			*cookie = 0;
>  	}
>  	return offset;
>  }
> @@ -81,7 +82,7 @@ static loff_t vfs_setpos_cookie(struct file *file, loff_t offset,
>   */
>  loff_t vfs_setpos(struct file *file, loff_t offset, loff_t maxsize)
>  {
> -	return vfs_setpos_cookie(file, offset, maxsize, &file->f_version);
> +	return vfs_setpos_cookie(file, offset, maxsize, NULL);
>  }
>  EXPORT_SYMBOL(vfs_setpos);
>  
> @@ -362,10 +363,8 @@ loff_t default_llseek(struct file *file, loff_t offset, int whence)
>  	}
>  	retval = -EINVAL;
>  	if (offset >= 0 || unsigned_offsets(file)) {
> -		if (offset != file->f_pos) {
> +		if (offset != file->f_pos)
>  			file->f_pos = offset;
> -			file->f_version = 0;
> -		}
>  		retval = offset;
>  	}
>  out:
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index ca4925008244..7e11ce172140 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1003,7 +1003,6 @@ static inline int ra_has_index(struct file_ra_state *ra, pgoff_t index)
>   * @f_pos_lock: lock protecting file position
>   * @f_pipe: specific to pipes
>   * @f_pos: file position
> - * @f_version: file version
>   * @f_security: LSM security context of this file
>   * @f_owner: file owner
>   * @f_wb_err: writeback error
> @@ -1034,11 +1033,10 @@ struct file {
>  		u64			f_pipe;
>  	};
>  	loff_t				f_pos;
> -	u64				f_version;
> -	/* --- cacheline 2 boundary (128 bytes) --- */
>  #ifdef CONFIG_SECURITY
>  	void				*f_security;
>  #endif
> +	/* --- cacheline 2 boundary (128 bytes) --- */
>  	struct fown_struct		*f_owner;
>  	errseq_t			f_wb_err;
>  	errseq_t			f_sb_err;
> 
> -- 
> 2.45.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH RFC 00/20] file: remove f_version

[Jeff Layton]

On Fri, 2024-08-30 at 15:04 +0200, Christian Brauner wrote:
> Probably not for this merge window anymore but let me get it out now:
> 
> The f_version member in struct file isn't particularly well-defined. It
> is mainly used as a cookie to detect concurrent seeks when iterating
> directories. But it is also abused by some subsystems for completely
> unrelated things.
> 
> It is mostly a directory specific thing that doesn't really need to live
> in struct file and with its wonky semantics it really lacks a specific
> function.
> 
> For pipes, f_version is (ab)used to defer poll notifications until a
> write has happened. And struct pipe_inode_info is used by multiple
> struct files in their ->private_data so there's no chance of pushing
> that down into file->private_data without introducing another pointer
> indirection.
> 
> But this should be a solvable problem. Only regular files with
> FMODE_ATOMIC_POS and directories require f_pos_lock. Pipes and other
> files don't. So this adds a union into struct file encompassing
> f_pos_lock and a pipe specific f_pipe member that pipes can use. This
> union of course can be extended to other file types and is similar to
> what we do in struct inode already.
> 
> I hope I got the details right. It at least holds up in xfstests and LTP
> and my hand-rolled getdents-seek stressor.
>   
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
> Christian Brauner (20):
>       file: remove pointless comment
>       adi: remove unused f_version
>       ceph: remove unused f_version
>       s390: remove unused f_version
>       fs: add vfs_setpos_cookie()
>       fs: add must_set_pos()
>       fs: use must_set_pos()
>       fs: add generic_llseek_cookie()
>       affs: store cookie in private data
>       ext2: store cookie in private data
>       ext4: store cookie in private data
>       input: remove f_version abuse
>       ocfs2: store cookie in private data
>       proc: store cookie in private data
>       udf: store cookie in private data
>       ufs: store cookie in private data
>       ubifs: store cookie in private data
>       fs: add f_pipe
>       pipe: use f_pipe
>       fs: remove f_version
> 
>  drivers/char/adi.c             |   1 -
>  drivers/input/input.c          |   8 +-
>  drivers/s390/char/hmcdrv_dev.c |   3 -
>  fs/affs/dir.c                  |  44 +++++++++--
>  fs/ceph/dir.c                  |   1 -
>  fs/ext2/dir.c                  |  28 ++++++-
>  fs/ext4/dir.c                  |  50 ++++++------
>  fs/ext4/ext4.h                 |   2 +
>  fs/ext4/inline.c               |   7 +-
>  fs/file_table.c                |   1 -
>  fs/ocfs2/dir.c                 |   3 +-
>  fs/ocfs2/file.c                |  11 ++-
>  fs/ocfs2/file.h                |   1 +
>  fs/pipe.c                      |   6 +-
>  fs/proc/base.c                 |  18 +++--
>  fs/read_write.c                | 169 +++++++++++++++++++++++++++++++----------
>  fs/ubifs/dir.c                 |  65 +++++++++++-----
>  fs/udf/dir.c                   |  28 ++++++-
>  fs/ufs/dir.c                   |  28 ++++++-
>  include/linux/fs.h             |  14 +++-
>  20 files changed, 366 insertions(+), 122 deletions(-)
> ---
> base-commit: 79868ff5ce9156228f9aef351d8bf76fb8540230
> change-id: 20240829-vfs-file-f_version-66321e3dafeb
> 

I like it! Fairly straightforward, and it's good to see f_version
(which has always been poorly defined) go away.

Reviewed-by: Jeff Layton <jlayton@kernel.org>