Re: [f2fs-dev] [PATCH] f2fs: avoid NULL checkpoint thread access in sysfs

[Chao Yu via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net>]

On 7/24/26 17:05, Wenjie Qi wrote:
> checkpoint_merge is set by default, but the checkpoint merge thread is not
> created for read-only mounts. The ckpt_thread_ioprio sysfs store path uses
> the mount option to decide whether to update the task ioprio. On such
> mounts, writing the node passes a NULL task to set_task_ioprio().
> 
> Keep storing the requested ioprio, but apply it only when the checkpoint
> thread exists. Take s_umount for ckpt_thread_ioprio and
> critical_task_priority too, matching the existing protection for GC thread
> entries.
> 
> Fixes: e65920661708 ("f2fs: add ckpt_thread_ioprio sysfs node")
> Cc: stable@kernel.org
> Signed-off-by: Wenjie Qi <qiwenjie@xiaomi.com>
> ---
> Reproducer:
>    mount -o ro -t f2fs /dev/vdb /mnt/f2fs
>    echo be,4 > /sys/fs/f2fs/vdb/ckpt_thread_ioprio
> 
> Baseline dmesg:
>    Oops: general protection fault
>    KASAN: null-ptr-deref in range [0x00000000000007b0-0x00000000000007b7]
>    RIP: set_task_ioprio+0x8b/0x360
>    Call Trace:
>     f2fs_sbi_store+0x2af/0x2580
>     kernfs_fop_write_iter+0x360/0x620
>     vfs_write+0x5f8/0xf50
>     ksys_write+0xf9/0x1d0
>     do_syscall_64+0x5f/0x550
> 
>   fs/f2fs/sysfs.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
> index be92c05a5420..0294544d3d34 100644
> --- a/fs/f2fs/sysfs.c
> +++ b/fs/f2fs/sysfs.c
> @@ -557,7 +557,7 @@ static ssize_t __sbi_store(struct f2fs_attr *a,
>   			return -EINVAL;
>   
>   		cprc->ckpt_thread_ioprio = IOPRIO_PRIO_VALUE(class, level);
> -		if (test_opt(sbi, MERGE_CHECKPOINT)) {
> +		if (cprc->f2fs_issue_ckpt) {
>   			ret = set_task_ioprio(cprc->f2fs_issue_ckpt,
>   					cprc->ckpt_thread_ioprio);
>   			if (ret)
> @@ -1007,13 +1007,16 @@ static ssize_t f2fs_sbi_store(struct f2fs_attr *a,
>   	ssize_t ret;
>   	bool gc_entry = (!strcmp(a->attr.name, "gc_urgent") ||
>   					a->struct_type == GC_THREAD);
> +	bool thread_entry = gc_entry ||
> +			!strcmp(a->attr.name, "ckpt_thread_ioprio") ||
> +			!strcmp(a->attr.name, "critical_task_priority");

Oh, seems you fixed another race issue in this patch as well?

"mount -o remount,ro" vs "echo xx > critical_task_priority"

Can you fix this in a separated patch?

>   
> -	if (gc_entry) {
> +	if (thread_entry) {

bool thread_entry = !strcmp(a->attr.name, "ckpt_thread_ioprio") ||
			!strcmp(a->attr.name, "critical_task_priority");

if (gc_entry || thread_entry)

>   		if (!down_read_trylock(&sbi->sb->s_umount))
>   			return -EAGAIN;
>   	}
>   	ret = __sbi_store(a, sbi, buf, count);
> -	if (gc_entry)
> +	if (thread_entry)

if (gc_entry || thread_entry)

Thanks,

>   		up_read(&sbi->sb->s_umount);
>   
>   	return ret;



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel