Re: [PATCH 07/10] ovl: implement overlayfs' own ->write_inode operation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ovl: missing read barrier in ovl_upper_unfinished()
--

commit f54ebddb1b6549b6ed1156d9609ee0d8a34e7718
Author: Chengguang Xu <charliecgxu@tencent.com>

ovl: implement overlayfs' own ->write_inode operation

This commit implements overlayfs's own ->write_inode() operation to sync
dirty data and metadata of the upper inode. It also keeps the overlay
inode marked dirty via ovl_upper_unfinished() while the upper inode is open
for writing or still has dirty or writeback pages.

> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index d21e7cbb70c28..0aec7059e9da7 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c

[ ... ]

> @@ -313,12 +315,62 @@ static int ovl_statfs(struct dentry *dentry, struct kstatfs *buf)
>  	return err;
>  }
>  
> +/*
> + * Could there be anything left to write or to wait for on @upper?
> + *
> + * Whoever has it open for writing may have it mapped, and stores through a
> + * mapping go unnoticed.  The mapping holds the file open.
> + *
> + * Writeback which was only started, by us or by the flusher of the upper
> + * filesystem, shows in the page cache tags until it is done.  The dirty flags
> + * of the inode do not, they are cleared at the start.
> + *
> + * Metadata is not looked at.  The upper filesystem logs it, see
> + * ovl_tracks_dirty(), and ovl_sync_fs() has the log committed.
> + *
> + * Nothing orders these reads.  The one order which matters is to not see
> + * i_writecount at zero and miss the tag of a page which was dirtied through
> + * the mapping.  The tag is set when the mapping goes away at the latest, the
> + * count drops when the file is released after that, with the locks of
> + * unmapping and of the final fput in between.
> + */
> +static bool ovl_upper_unfinished(struct inode *upper)
> +{
> +	struct address_space *mapping = upper->i_mapping;
> +
> +	return inode_is_open_for_write(upper) ||
> +	       mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) ||
> +	       mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK) ||
> +	       (inode_state_read_once(upper) & I_SYNC);
> +}

[Severity: Medium]
Can ovl_upper_unfinished() still observe upper->i_writecount == 0 while
missing PAGECACHE_TAG_DIRTY without a read barrier on the reader side?

The comment above ovl_upper_unfinished() notes that the critical ordering is
not to see i_writecount at zero and miss the dirty tag of a page dirtied
through a shared writable mapping, relying on the locks between unmapping
and the final fput on the writer side.

However, in ovl_upper_unfinished():
- inode_is_open_for_write(upper) performs a relaxed
  atomic_read(&upper->i_writecount)
- mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) performs a plain load of
  mapping->i_pages.xa_flags via xa_marked()

Consider the following race when background writeback runs
ovl_write_inode() -> write_inode_now(upper, false) while upper is initially
clean:

CPU 0                                 CPU 1
ovl_write_inode()
  write_inode_now(upper, false)
  ovl_upper_unfinished(upper)
                                      munmap() dirty shared mapping
                                        sets PAGECACHE_TAG_DIRTY in xa_flags
                                      close() -> __fput() -> put_file_access()
                                        atomic_dec(&upper->i_writecount)
    inode_is_open_for_write(upper)
    mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)

Because there is no smp_rmb() or acquire barrier between reading
upper->i_writecount and reading mapping->i_pages.xa_flags on CPU 0, can the
compiler or a weakly-ordered CPU reorder the xa_flags load before the
i_writecount load, causing ovl_upper_unfinished() to return false and leave
the overlay inode clean while upper still has dirty pages that a subsequent
syncfs() will miss?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-b4-ovl-containerized-sync-v1-0-802e512379a6@toxicpanda.com?part=7