[PATCH] erofs: hold real inodes for shared files

[PATCH] erofs: hold real inodes for shared files

[Gao Xiang]

EROFS page cache sharing uses shared inodes. However, a mmaped VMA
keeps the shared file even after the corresponding fd is closed, while
currently the real inode is never held by any reference associated
with the shared file.

As a result, the real inode could be evicted and removed from the
shared inode's list while the page fault can still invoke on the
shared inode's address space, which causes no candidate is
successfully acquired by erofs_real_inode().

Fix this by simply keeping the original inode in the shared backing
file's private data.

Also replace confusing "readinode"/"rf" names with "sf" (sharedfile)
to unify the terms as well as nullify realinode in erofs_real_inode()
to avoid returning an uninitialized pointer.

Fixes: 5ef3208e3be5 ("erofs: introduce the page cache share feature")
Reported-by: YoungWoo Rim <xsapiens75@gmail.com>
Closes: https://lore.kernel.org/r/atzSVuvqYKgmvDmO@MacBookPro
Cc: stable@vger.kernel.org # 7.0+
Signed-off-by: Gao Xiang (Docker) <xiang@kernel.org>
---
 fs/erofs/ishare.c | 43 ++++++++++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 13 deletions(-)

diff --git a/fs/erofs/ishare.c b/fs/erofs/ishare.c
index a1cf2aff8d32..61f48ceb6cb6 100644
--- a/fs/erofs/ishare.c
+++ b/fs/erofs/ishare.c
@@ -30,9 +30,18 @@ static int erofs_ishare_iget5_set(struct inode *inode, void *data)
 	return 0;
 }
 
+static int erofs_ishare_backing_release(struct inode *inode, struct file *file)
+{
+	iput(file->private_data);
+	file->private_data = NULL;
+	return 0;
+}
+
 bool erofs_ishare_fill_inode(struct inode *inode)
 {
-	static const struct file_operations empty_fops = {};
+	static const struct file_operations backing_fops = {
+		.release = erofs_ishare_backing_release,
+	};
 	struct erofs_sb_info *sbi = EROFS_SB(inode->i_sb);
 	const struct address_space_operations *aops;
 	struct erofs_inode *vi = EROFS_I(inode);
@@ -50,7 +59,7 @@ bool erofs_ishare_fill_inode(struct inode *inode)
 			  xxh32(fp.opaque, fp.size, 0),
 			  erofs_ishare_iget5_eq, erofs_ishare_iget5_set, &fp);
 	if (si && (inode_state_read_once(si) & I_NEW)) {
-		si->i_fop = &empty_fops;
+		si->i_fop = &backing_fops;
 		si->i_mapping->a_ops = aops;
 		si->i_mode = 0444 | S_IFREG;
 		si->i_size = inode->i_size;
@@ -100,16 +109,24 @@ static int erofs_ishare_file_open(struct inode *inode, struct file *file)
 		.mnt = erofs_ishare_mnt,
 		.dentry = EROFS_I(inode)->sharedentry,
 	};
-	struct file *rf;
+	struct file *sf;
 
 	if (file->f_flags & O_DIRECT)
 		return -EINVAL;
 
-	rf = backing_file_open(file, file->f_flags | O_NOATIME,
+	sf = backing_file_open(file, file->f_flags | O_NOATIME,
 			       &sharedpath, current_cred());
-	if (IS_ERR(rf))
-		return PTR_ERR(rf);
-	file->private_data = rf;
+	if (IS_ERR(sf))
+		return PTR_ERR(sf);
+	/*
+	 * A VMA only holds a reference to the shared file so that the original
+	 * file may be released: the shared file must therefore hold a reference
+	 * to one of the real inodes ("inode" passed here), and that reference
+	 * will be released by erofs_ishare_backing_release().
+	 */
+	ihold(inode);
+	sf->private_data = inode;
+	file->private_data = sf;
 	return 0;
 }
 
@@ -123,13 +140,13 @@ static int erofs_ishare_file_release(struct inode *inode, struct file *file)
 static ssize_t erofs_ishare_file_read_iter(struct kiocb *iocb,
 					   struct iov_iter *to)
 {
-	struct file *realfile = iocb->ki_filp->private_data;
+	struct file *sf = iocb->ki_filp->private_data;
 	struct kiocb dedup_iocb;
 	ssize_t nread;
 
 	if (!iov_iter_count(to))
 		return 0;
-	kiocb_clone(&dedup_iocb, iocb, realfile);
+	kiocb_clone(&dedup_iocb, iocb, sf);
 	nread = filemap_read(&dedup_iocb, to, 0);
 	iocb->ki_pos = dedup_iocb.ki_pos;
 	return nread;
@@ -137,12 +154,12 @@ static ssize_t erofs_ishare_file_read_iter(struct kiocb *iocb,
 
 static int erofs_ishare_mmap(struct file *file, struct vm_area_struct *vma)
 {
-	struct file *realfile = file->private_data;
+	struct file *sf = file->private_data;
 	int err;
 
-	vma_set_file(vma, realfile);
+	vma_set_file(vma, sf);
 
-	err = security_mmap_backing_file(vma, realfile, file);
+	err = security_mmap_backing_file(vma, sf, file);
 	if (err)
 		return err;
 
@@ -176,7 +193,7 @@ const struct file_operations erofs_ishare_fops = {
 struct inode *erofs_real_inode(struct inode *inode, bool *need_iput)
 {
 	struct erofs_inode *vi, *vi_share;
-	struct inode *realinode;
+	struct inode *realinode = NULL;
 
 	*need_iput = false;
 	if (inode->i_sb != erofs_ishare_mnt->mnt_sb)
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] erofs: hold real inodes for shared files

[Chao Yu]

On 10/25/26 03:20, Gao Xiang wrote:
> EROFS page cache sharing uses shared inodes. However, a mmaped VMA
> keeps the shared file even after the corresponding fd is closed, while
> currently the real inode is never held by any reference associated
> with the shared file.
> 
> As a result, the real inode could be evicted and removed from the
> shared inode's list while the page fault can still invoke on the
> shared inode's address space, which causes no candidate is
> successfully acquired by erofs_real_inode().
> 
> Fix this by simply keeping the original inode in the shared backing
> file's private data.
> 
> Also replace confusing "readinode"/"rf" names with "sf" (sharedfile)
> to unify the terms as well as nullify realinode in erofs_real_inode()
> to avoid returning an uninitialized pointer.
> 
> Fixes: 5ef3208e3be5 ("erofs: introduce the page cache share feature")
> Reported-by: YoungWoo Rim <xsapiens75@gmail.com>
> Closes: https://lore.kernel.org/r/atzSVuvqYKgmvDmO@MacBookPro
> Cc: stable@vger.kernel.org # 7.0+
> Signed-off-by: Gao Xiang (Docker) <xiang@kernel.org>

Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,