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

[Wenjie Qi]

  Yes. This patch also extends the existing s_umount protection to
  ckpt_thread_ioprio and critical_task_priority, which is a separate
  remount-vs-sysfs race issue.

  I'll split it:
  - keep only the NULL f2fs_issue_ckpt fix in this patch
  - send the s_umount protection for ckpt_thread_ioprio and
    critical_task_priority as a separate patch

On Mon, Aug 3, 2026 at 3:53 PM Chao Yu <chao@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx
> > Signed-off-by: Wenjie Qi <qiwenjie@xxxxxxxxxx>
> > ---
> > 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;
>