[PATCH v2 1/2] f2fs: avoid NULL checkpoint thread access in sysfs

[PATCH v2 1/2] f2fs: avoid NULL checkpoint thread access in sysfs

[Wenjie Qi]

checkpoint_merge can be enabled even when no checkpoint merge thread is
running. A read-only mount is one case: f2fs does not start
f2fs_issue_ckpt there, but ckpt_thread_ioprio is still writable through
sysfs.

The ckpt_thread_ioprio store path updates the saved ioprio value and,
when checkpoint_merge is enabled, calls set_task_ioprio() for the
checkpoint thread. If cprc->f2fs_issue_ckpt is NULL, that dereferences a
NULL task pointer.

Keep storing the requested ioprio, but apply it only when the
checkpoint thread exists.

Fixes: e65920661708 ("f2fs: add ckpt_thread_ioprio sysfs node")
Cc: stable@kernel.org
Signed-off-by: Wenjie Qi <qiwenjie@xiaomi.com>
---
 fs/f2fs/sysfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
index be92c05a5420..070f9807ae81 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)
-- 
2.43.0

[PATCH v2 2/2] f2fs: protect sysfs thread priority updates with s_umount

[Wenjie Qi]

The sysfs store path already takes s_umount for GC thread control
entries, so thread state is not changed while unmount or remount is
tearing the filesystem down.

ckpt_thread_ioprio and critical_task_priority also touch checkpoint or
GC thread scheduling state, but they are not covered by that
serialization. They can race with remount or teardown paths that are
stopping those threads.

Extend the existing s_umount protection to these thread-priority sysfs
entries as well.

Signed-off-by: Wenjie Qi <qiwenjie@xiaomi.com>
---
 fs/f2fs/sysfs.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
index 070f9807ae81..aa1621419932 100644
--- a/fs/f2fs/sysfs.c
+++ b/fs/f2fs/sysfs.c
@@ -1007,13 +1007,15 @@ 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 = !strcmp(a->attr.name, "ckpt_thread_ioprio") ||
+			!strcmp(a->attr.name, "critical_task_priority");
 
-	if (gc_entry) {
+	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 (gc_entry || thread_entry)
 		up_read(&sbi->sb->s_umount);
 
 	return ret;
-- 
2.43.0

Re: [PATCH v2 1/2] f2fs: avoid NULL checkpoint thread access in sysfs

[Chao Yu]

This patch should include below change? otherwise it's not a self-contained
fix.

@@ -1007,13 +1007,15 @@ 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 = !strcmp(a->attr.name, "ckpt_thread_ioprio");

-	if (gc_entry) {
+	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 (gc_entry || thread_entry)
  		up_read(&sbi->sb->s_umount);

On 8/3/26 17:23, Wenjie Qi wrote:
> checkpoint_merge can be enabled even when no checkpoint merge thread is
> running. A read-only mount is one case: f2fs does not start
> f2fs_issue_ckpt there, but ckpt_thread_ioprio is still writable through
> sysfs.
> 
> The ckpt_thread_ioprio store path updates the saved ioprio value and,
> when checkpoint_merge is enabled, calls set_task_ioprio() for the
> checkpoint thread. If cprc->f2fs_issue_ckpt is NULL, that dereferences a
> NULL task pointer.
> 
> Keep storing the requested ioprio, but apply it only when the
> checkpoint thread exists.
> 
> Fixes: e65920661708 ("f2fs: add ckpt_thread_ioprio sysfs node")
> Cc: stable@kernel.org
> Signed-off-by: Wenjie Qi <qiwenjie@xiaomi.com>
> ---
>   fs/f2fs/sysfs.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
> index be92c05a5420..070f9807ae81 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)