linux-kernel - Re: [f2fs-dev] [PATCH] f2fs: handle error cases of memory donation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <b68762b0-b003-4a0b-9f7e-09c1b7bd14eb@kernel.org>
Date: Wed, 23 Apr 2025 16:49:55 +0800
From: Chao Yu <chao@...nel.org>
To: Daeho Jeong <daeho43@...il.com>, linux-kernel@...r.kernel.org,
 linux-f2fs-devel@...ts.sourceforge.net, kernel-team@...roid.com
Cc: chao@...nel.org, Daeho Jeong <daehojeong@...gle.com>
Subject: Re: [f2fs-dev] [PATCH] f2fs: handle error cases of memory donation

On 4/23/25 03:11, Daeho Jeong wrote:
> From: Daeho Jeong <daehojeong@...gle.com>
> 
> In cases of removing memory donation, we need to handle some error cases
> like ENOENT and EACCES (indicating the range already has been donated).
> 
> Signed-off-by: Daeho Jeong <daehojeong@...gle.com>
> ---
>  fs/f2fs/f2fs.h     |  1 +
>  fs/f2fs/file.c     | 21 ++++++++++++++-------
>  fs/f2fs/shrinker.c |  5 +++++
>  3 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index f1576dc6ec67..e4b39550f380 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -821,6 +821,7 @@ enum {
>  	FI_ATOMIC_DIRTIED,	/* indicate atomic file is dirtied */
>  	FI_ATOMIC_REPLACE,	/* indicate atomic replace */
>  	FI_OPENED_FILE,		/* indicate file has been opened */
> +	FI_PAGE_DONATED,	/* indicate pages of file has been donated */
>  	FI_MAX,			/* max flag, never be used */
>  };
>  
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index abbcbb5865a3..0807f8e97492 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -2464,19 +2464,20 @@ static int f2fs_ioc_shutdown(struct file *filp, unsigned long arg)
>  	return ret;
>  }
>  
> -static void f2fs_keep_noreuse_range(struct inode *inode,
> +static int f2fs_keep_noreuse_range(struct inode *inode,
>  				loff_t offset, loff_t len)
>  {
>  	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>  	u64 max_bytes = F2FS_BLK_TO_BYTES(max_file_blocks(inode));
>  	u64 start, end;
> +	int ret = 0;
>  
>  	if (!S_ISREG(inode->i_mode))
> -		return;
> +		return 0;
>  
>  	if (offset >= max_bytes || len > max_bytes ||
>  	    (offset + len) > max_bytes)
> -		return;
> +		return 0;
>  
>  	start = offset >> PAGE_SHIFT;
>  	end = DIV_ROUND_UP(offset + len, PAGE_SIZE);
> @@ -2484,7 +2485,7 @@ static void f2fs_keep_noreuse_range(struct inode *inode,
>  	inode_lock(inode);
>  	if (f2fs_is_atomic_file(inode)) {
>  		inode_unlock(inode);
> -		return;
> +		return 0;
>  	}
>  
>  	spin_lock(&sbi->inode_lock[DONATE_INODE]);
> @@ -2493,7 +2494,10 @@ static void f2fs_keep_noreuse_range(struct inode *inode,
>  		if (!list_empty(&F2FS_I(inode)->gdonate_list)) {
>  			list_del_init(&F2FS_I(inode)->gdonate_list);
>  			sbi->donate_files--;
> -		}
> +			if (is_inode_flag_set(inode, FI_PAGE_DONATED))
> +				ret = -EACCES;

If caller has no enough permission, it will return EACCES as well, to avoid
conflict, what do you think of returning EALREADY? or use a wrapper of this
macro like EFSCORRUPTED?

Thanks,

> +		} else
> +			ret = -ENOENT;
>  	} else {
>  		if (list_empty(&F2FS_I(inode)->gdonate_list)) {
>  			list_add_tail(&F2FS_I(inode)->gdonate_list,
> @@ -2505,9 +2509,12 @@ static void f2fs_keep_noreuse_range(struct inode *inode,
>  		}
>  		F2FS_I(inode)->donate_start = start;
>  		F2FS_I(inode)->donate_end = end - 1;
> +		clear_inode_flag(inode, FI_PAGE_DONATED);
>  	}
>  	spin_unlock(&sbi->inode_lock[DONATE_INODE]);
>  	inode_unlock(inode);
> +
> +	return ret;
>  }
>  
>  static int f2fs_ioc_fitrim(struct file *filp, unsigned long arg)
> @@ -5236,8 +5243,8 @@ static int f2fs_file_fadvise(struct file *filp, loff_t offset, loff_t len,
>  	     f2fs_compressed_file(inode)))
>  		f2fs_invalidate_compress_pages(F2FS_I_SB(inode), inode->i_ino);
>  	else if (advice == POSIX_FADV_NOREUSE)
> -		f2fs_keep_noreuse_range(inode, offset, len);
> -	return 0;
> +		err = f2fs_keep_noreuse_range(inode, offset, len);
> +	return err;
>  }
>  
>  #ifdef CONFIG_COMPAT
> diff --git a/fs/f2fs/shrinker.c b/fs/f2fs/shrinker.c
> index 9c8d3aee89af..1fa6619db40f 100644
> --- a/fs/f2fs/shrinker.c
> +++ b/fs/f2fs/shrinker.c
> @@ -186,8 +186,13 @@ static unsigned int do_reclaim_caches(struct f2fs_sb_info *sbi,
>  
>  		len = fi->donate_end - fi->donate_start + 1;
>  		npages = npages < len ? 0 : npages - len;
> +
> +		inode_lock(inode);
>  		invalidate_inode_pages2_range(inode->i_mapping,
>  					fi->donate_start, fi->donate_end);
> +		set_inode_flag(inode, FI_PAGE_DONATED);
> +		inode_unlock(inode);
> +
>  		iput(inode);
>  		cond_resched();
>  	}