Re: [PATCH v7 2/3] mm/mempolicy: Prepare weighted interleave sysfs for memory hotplug

[Rakie Kim <rakie.kim@sk.com>]

On Tue, 8 Apr 2025 20:54:48 -0700 Dan Williams <dan.j.williams@intel.com> wrote:
> Dan Williams wrote:
> > >  
> > > +struct sysfs_wi_group {
> > > +	struct kobject wi_kobj;
> > > +	struct iw_node_attr *nattrs[];
> > > +};
> > > +
> > > +static struct sysfs_wi_group *wi_group;
> > > +
> > >  static ssize_t node_show(struct kobject *kobj, struct kobj_attribute *attr,
> > >  			 char *buf)
> > >  {
> > > @@ -3461,27 +3468,24 @@ static ssize_t node_store(struct kobject *kobj, struct kobj_attribute *attr,
> > >  	return count;
> > >  }
> > >  
> > > -static struct iw_node_attr **node_attrs;
> > > -
> > > -static void sysfs_wi_node_release(struct iw_node_attr *node_attr,
> > > -				  struct kobject *parent)
> > > +static void sysfs_wi_node_delete(int nid)
> > >  {
> > > -	if (!node_attr)
> > > +	if (!wi_group->nattrs[nid])
> > >  		return;
> > > -	sysfs_remove_file(parent, &node_attr->kobj_attr.attr);
> > > -	kfree(node_attr->kobj_attr.attr.name);
> > > -	kfree(node_attr);
> > > +
> > > +	sysfs_remove_file(&wi_group->wi_kobj,
> > > +			  &wi_group->nattrs[nid]->kobj_attr.attr);
> > 
> > This still looks broken to me, but I think this is more a problem that
> > was present in the original code.
> > 
> > At this point @wi_group's reference count is zero because
> > sysfs_wi_release() has been called. However, it can only be zero if it has
> > properly transitioned through kobject_del() and final kobject_put(). It
> > follows that kobject_del() arranges for kobj->sd to be NULL. That means
> > that this *should* be hitting the WARN() in kernfs_remove_by_name_ns()
> > for the !parent case.
> > 
> > So, either you are not triggering that path, or testing that path, but
> > sys_remove_file() of the child attributes should be happening *before*
> > sysfs_wi_release().
> > 
> > Did I miss something?
> 
> I think the missing change is that sysfs_wi_node_add() failures need to
> be done with a sysfs_wi_node_delete() of the added attrs *before* the
> kobject_del() of @wi_group.

Hi Dan,

Thank you for pointing out this issue.

As you suggested, I believe the most appropriate way to handle this is
to incorporate your feedback into Patch 1 
(mm/mempolicy: Fix memory leaks in weighted interleave sysfs).

To ensure that sysfs_remove_file() is called before kobject_del(), I
have restructured the code as follows:

<Previously>
static void sysfs_wi_release(struct kobject *wi_kobj)
{
	int nid;

	for (nid = 0; nid < nr_node_ids; nid++)
		sysfs_wi_node_delete(node_attrs[nid], wi_kobj);
		-> ERROR: sysfs_remove_file called here
	kfree(node_attrs);
	kfree(wi_kobj);
}

<Now>
static void sysfs_wi_node_delete_all(struct kobject *wi_kobj)
{
	int nid;

	for (nid = 0; nid < nr_node_ids; nid++)
		sysfs_wi_node_delete(node_attrs[nid], wi_kobj);
		-> sysfs_remove_file called here
}

static void sysfs_wi_release(struct kobject *wi_kobj)
{
	kfree(node_attrs);
	kfree(wi_kobj);
}

In addition, I call sysfs_wi_node_delete_all() before kobject_del()
during error handling:

+err_cleanup_kobj:
+	sysfs_wi_node_delete_all(wi_kobj);
	kobject_del(wi_kobj);

I believe this resolves the issue you raised.

That said, I have a follow-up question. With this structure, when the
system is shutting down, sysfs_remove_file() will not be called. Based
on my review of other kernel subsystems, it seems that sysfs_remove_file()
is only called during module_exit() in driver code, and not in other
built-in subsystems.

Is this an acceptable practice? If you happen to know the expected
behavior in such cases, I would appreciate your insights.

Below is the full content of the updated Patch 1.
@@ -3463,8 +3463,8 @@ static ssize_t node_store(struct kobject *kobj, struct kobj_attribute *attr,
 
 static struct iw_node_attr **node_attrs;
 
-static void sysfs_wi_node_release(struct iw_node_attr *node_attr,
-                                 struct kobject *parent)
+static void sysfs_wi_node_delete(struct iw_node_attr *node_attr,
+                                struct kobject *parent)
 {
        if (!node_attr)
                return;
@@ -3473,13 +3473,16 @@ static void sysfs_wi_node_release(struct iw_node_attr *node_attr,
        kfree(node_attr);
 }
 
-static void sysfs_wi_release(struct kobject *wi_kobj)
+static void sysfs_wi_node_delete_all(struct kobject *wi_kobj)
 {
-       int i;
+       int nid;
 
-       for (i = 0; i < nr_node_ids; i++)
-               sysfs_wi_node_release(node_attrs[i], wi_kobj);
+       for (nid = 0; nid < nr_node_ids; nid++)
+               sysfs_wi_node_delete(node_attrs[nid], wi_kobj);
+}
 
+static void sysfs_wi_release(struct kobject *wi_kobj)
+{
        kfree(node_attrs);
        kfree(wi_kobj);
 }
@@ -3547,13 +3550,14 @@ static int add_weighted_interleave_group(struct kobject *root_kobj)
                err = add_weight_node(nid, wi_kobj);
                if (err) {
                        pr_err("failed to add sysfs [node%d]\n", nid);
-                       goto err_del_kobj;
+                       goto err_cleanup_kobj;
                }
        }
 
        return 0;
 
-err_del_kobj:
+err_cleanup_kobj:
+       sysfs_wi_node_delete_all(wi_kobj);
        kobject_del(wi_kobj);
 err_put_kobj:
        kobject_put(wi_kobj);

Thank you again for your helpful feedback.

Rakie