Re: [RFC PATCH 1/9] mm/damon/sysfs: kobject_del() target, context and kdamond dirs

[SeongJae Park <sj@kernel.org>]

On Tue, 16 Jun 2026 15:25:52 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Incomplete fix: Missing kobject_del() in sysfs directory creation error paths leads to persistent -EEXIST collisions.
> --
> 
> commit 1c0329e17860c383f4ab90a119f917db75ecacee
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/sysfs: kobject_del() target, context and kdamond dirs
> 
> On CONFIG_DEBUG_KOBJECT_RELEASE enabled kernel, lack of kobject_del()
> could cause directories creation failures due to the name conflicts.
> Fix those issues for target, context and kdamond directories by adding
> kobject_del() calls.
> 
> Fixes: c951cd3b8901 ("mm/damon: implement a minimal stub for sysfs-based DAMON interface")
> Cc: <stable@vger.kernel.org> # 5.18.x
> 
> > diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
> > index 2e95e3bac774d..d93f7919c3ca1 100644
> > --- a/mm/damon/sysfs.c
> > +++ b/mm/damon/sysfs.c
> > @@ -333,6 +333,7 @@ static void damon_sysfs_targets_rm_dirs(struct damon_sysfs_targets *targets)
> >  
> >  	for (i = 0; i < targets->nr; i++) {
> >  		damon_sysfs_target_rm_dirs(targets_arr[i]);
> > +		kobject_del(&targets_arr[i]->kobj);
> 
> [Severity: High]
> This isn't a regression introduced by this patch, but while you are fixing
> the -EEXIST name collisions in the rm_dirs() paths, are the same
> kobject_del() calls also needed in the error paths of the corresponding
> add_dirs() functions?
> 
> For example, in damon_sysfs_targets_add_dirs():
> 
>     mm/damon/sysfs.c:damon_sysfs_targets_add_dirs() {
>         ...
>             err = kobject_init_and_add(&target->kobj, ...);
>             if (err)
>                 goto out;
> 
>             err = damon_sysfs_target_add_dirs(target);
>             if (err)
>                 goto out;
>         ...
>     out:
>         damon_sysfs_targets_rm_dirs(targets);
>         kobject_put(&target->kobj);
>         return err;
>     }
> 
> If damon_sysfs_target_add_dirs() fails, kobject_put() is called without
> kobject_del(). Does this result in a lingering directory and a persistent
> -EEXIST if the user retries?
> 
> Similarly, in mm/damon/sysfs-schemes.c:damos_sysfs_populate_region_dir():
> 
>     mm/damon/sysfs-schemes.c:damos_sysfs_populate_region_dir() {
>         ...
>         if (kobject_init_and_add(&region->kobj,
>                     &damon_sysfs_scheme_region_ktype,
>                     &sysfs_regions->kobj, "%d",
>                     sysfs_regions->nr_regions))
>             goto out;
>         if (damos_sysfs_region_add_dirs(region, ctx, r))
>             goto out;
>         ...
>         sysfs_regions->nr_regions++;
>         return;
> 
>     out:
>         kobject_put(&region->kobj);
>     }
> 
> If this fails, it returns without incrementing sysfs_regions->nr_regions.
> Could the next region processed in the loop then attempt to initialize with
> the exact same name string, immediately hitting -EEXIST and breaking the
> entire region population loop?

Good finding.  I will fix this in the next revision, like below:

'''
--- a/mm/damon/sysfs.c
+++ b/mm/damon/sysfs.c
@@ -374,13 +374,15 @@ static int damon_sysfs_targets_add_dirs(struct damon_sysfs_targets *targets,

                err = damon_sysfs_target_add_dirs(target);
                if (err)
-                       goto out;
+                       goto del_out;

                targets_arr[i] = target;
                targets->nr++;
        }
        return 0;

+del_out:
+       kobject_del(&target->kobj);
 out:
        damon_sysfs_targets_rm_dirs(targets);
        kobject_put(&target->kobj);
'''


Thanks,
SJ

[...]