jbd2_inode fields are updated under journal->j_list_lock, but some
paths read them without holding the lock (e.g. fast commit
helpers and the ordered truncate fast path).

Use READ_ONCE() for these lockless reads to correct the
concurrency assumptions.

Suggested-by: Jan Kara <jack@suse.com>
Signed-off-by: Li Chen <me@linux.beauty>
---
 fs/jbd2/commit.c      | 39 ++++++++++++++++++++++++++++++++-------
 fs/jbd2/transaction.c |  2 +-
 2 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 7203d2d2624d..3347d75da2f8 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -180,7 +180,13 @@ static int journal_wait_on_commit_record(journal_t *journal,
 /* Send all the data buffers related to an inode */
 int jbd2_submit_inode_data(journal_t *journal, struct jbd2_inode *jinode)
 {
-	if (!jinode || !(jinode->i_flags & JI_WRITE_DATA))
+	unsigned long flags;
+
+	if (!jinode)
+		return 0;
+
+	flags = READ_ONCE(jinode->i_flags);
+	if (!(flags & JI_WRITE_DATA))
 		return 0;
 
 	trace_jbd2_submit_inode_data(jinode->i_vfs_inode);
@@ -191,12 +197,30 @@ EXPORT_SYMBOL(jbd2_submit_inode_data);
 
 int jbd2_wait_inode_data(journal_t *journal, struct jbd2_inode *jinode)
 {
-	if (!jinode || !(jinode->i_flags & JI_WAIT_DATA) ||
-		!jinode->i_vfs_inode || !jinode->i_vfs_inode->i_mapping)
+	struct address_space *mapping;
+	struct inode *inode;
+	unsigned long flags;
+	loff_t start, end;
+
+	if (!jinode)
+		return 0;
+
+	flags = READ_ONCE(jinode->i_flags);
+	if (!(flags & JI_WAIT_DATA))
+		return 0;
+
+	inode = READ_ONCE(jinode->i_vfs_inode);
+	if (!inode)
+		return 0;
+
+	mapping = inode->i_mapping;
+	start = READ_ONCE(jinode->i_dirty_start);
+	end = READ_ONCE(jinode->i_dirty_end);
+
+	if (!mapping)
 		return 0;
 	return filemap_fdatawait_range_keep_errors(
-		jinode->i_vfs_inode->i_mapping, jinode->i_dirty_start,
-		jinode->i_dirty_end);
+		mapping, start, end);
 }
 EXPORT_SYMBOL(jbd2_wait_inode_data);
 
@@ -240,10 +264,11 @@ static int journal_submit_data_buffers(journal_t *journal,
 int jbd2_journal_finish_inode_data_buffers(struct jbd2_inode *jinode)
 {
 	struct address_space *mapping = jinode->i_vfs_inode->i_mapping;
+	loff_t start = READ_ONCE(jinode->i_dirty_start);
+	loff_t end = READ_ONCE(jinode->i_dirty_end);
 
 	return filemap_fdatawait_range_keep_errors(mapping,
-						   jinode->i_dirty_start,
-						   jinode->i_dirty_end);
+						   start, end);
 }
 
 /*
diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index dca4b5d8aaaa..302b2090eea7 100644
--- a/fs/jbd2/transaction.c
+++ b/fs/jbd2/transaction.c
@@ -2739,7 +2739,7 @@ int jbd2_journal_begin_ordered_truncate(journal_t *journal,
 	int ret = 0;
 
 	/* This is a quick check to avoid locking if not necessary */
-	if (!jinode->i_transaction)
+	if (!READ_ONCE(jinode->i_transaction))
 		goto out;
 	/* Locks are here just to force reading of recent values, it is
 	 * enough that the transaction was not committing before we started
-- 
2.52.0

On Fri 30-01-26 11:12:30, Li Chen wrote:
> jbd2_inode fields are updated under journal->j_list_lock, but some
> paths read them without holding the lock (e.g. fast commit
> helpers and the ordered truncate fast path).
> 
> Use READ_ONCE() for these lockless reads to correct the
> concurrency assumptions.
> 
> Suggested-by: Jan Kara <jack@suse.com>
> Signed-off-by: Li Chen <me@linux.beauty>

Just one nit below. With that fixed feel free to add:

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

> @@ -191,12 +197,30 @@ EXPORT_SYMBOL(jbd2_submit_inode_data);
>  
>  int jbd2_wait_inode_data(journal_t *journal, struct jbd2_inode *jinode)
>  {
> -	if (!jinode || !(jinode->i_flags & JI_WAIT_DATA) ||
> -		!jinode->i_vfs_inode || !jinode->i_vfs_inode->i_mapping)
> +	struct address_space *mapping;
> +	struct inode *inode;
> +	unsigned long flags;
> +	loff_t start, end;
> +
> +	if (!jinode)
> +		return 0;
> +
> +	flags = READ_ONCE(jinode->i_flags);
> +	if (!(flags & JI_WAIT_DATA))
> +		return 0;
> +
> +	inode = READ_ONCE(jinode->i_vfs_inode);

i_vfs_inode never changes so READ_ONCE is pointless here.

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

On Mon 02-02-26 17:40:45, Jan Kara wrote:
> On Fri 30-01-26 11:12:30, Li Chen wrote:
> > jbd2_inode fields are updated under journal->j_list_lock, but some
> > paths read them without holding the lock (e.g. fast commit
> > helpers and the ordered truncate fast path).
> > 
> > Use READ_ONCE() for these lockless reads to correct the
> > concurrency assumptions.
> > 
> > Suggested-by: Jan Kara <jack@suse.com>
> > Signed-off-by: Li Chen <me@linux.beauty>
> 
> Just one nit below. With that fixed feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> > @@ -191,12 +197,30 @@ EXPORT_SYMBOL(jbd2_submit_inode_data);
> >  
> >  int jbd2_wait_inode_data(journal_t *journal, struct jbd2_inode *jinode)
> >  {
> > -	if (!jinode || !(jinode->i_flags & JI_WAIT_DATA) ||
> > -		!jinode->i_vfs_inode || !jinode->i_vfs_inode->i_mapping)
> > +	struct address_space *mapping;
> > +	struct inode *inode;
> > +	unsigned long flags;
> > +	loff_t start, end;
> > +
> > +	if (!jinode)
> > +		return 0;
> > +
> > +	flags = READ_ONCE(jinode->i_flags);
> > +	if (!(flags & JI_WAIT_DATA))
> > +		return 0;
> > +
> > +	inode = READ_ONCE(jinode->i_vfs_inode);
> 
> i_vfs_inode never changes so READ_ONCE is pointless here.

One more note: I've realized that for this to work you also need to make
jbd2_journal_file_inode() use WRITE_ONCE() when updating i_dirty_start,
i_dirty_end and i_flags.

								Honza

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