Re: [PATCH v2 4/4] mm/mempolicy: Fix duplicate node addition in sysfs for weighted interleave

[Rakie Kim]

On Wed, 12 Mar 2025 08:04:39 -0700 Joshua Hahn <joshua.hahnjy@xxxxxxxxx> wrote:

Hi Joshua
Thank you for your response regarding this patch.

> Hi Rakie, thank your new revision!
> 
> I think this new ordering of the series makes more sense, since the bug exists
> even before your patch is applied! IMHO, it might also make sense to take
> patch 1 out of this series, and send it separately (and make patches 2-4
> their own series). 
> 
> I have a nit and a few thoughts about this patch and the way we order locking
> and allocating:
> 
> >  static void sysfs_wi_release(struct kobject *wi_kobj)
> > @@ -3464,35 +3470,54 @@ static const struct kobj_type wi_ktype = {
> >  
> >  static int sysfs_wi_node_add(int nid)
> >  {
> > -	struct iw_node_attr *node_attr;
> > +	int ret = 0;
> >  	char *name;
> >  
> > -	node_attr = kzalloc(sizeof(*node_attr), GFP_KERNEL);
> > -	if (!node_attr)
> > -		return -ENOMEM;
> > +	if (nid < 0 || nid >= nr_node_ids) {
> > +		pr_err("Invalid node id: %d\n", nid);
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +
> > +	mutex_lock(&ngrp->kobj_lock);
> > +	if (!ngrp->nattrs[nid]) {
> > +		ngrp->nattrs[nid] = kzalloc(sizeof(struct iw_node_attr), GFP_KERNEL);
> 
> I am unsure if kzallocing with the mutex_lock held is best practice. Even though
> two threads won't reach this place simultaneously since *most* calls to this
> function are sequential, we should try to keep the code safe since future
> patches might overlook this, and later make non-sequential calls : -)
> 
> It also doesn't seem wise to directly assign the result of an allocation
> without checking for its success (as I explain below).
> 
> IMHO it is best to allocate first, then acquire the lock and check for
> existence, then assign with the lock still held. We can also reduce this code
> section into a single if statement for clarity (but if you think it looks
> cleaner with the if-else, please keep it!)
> 
> struct iw_node_attr *node_attr;
> 
> ...
> 
> node_attr = kzalloc(sizeof(*node_attr), GFP_KERNEL);
> if (!node_attr) {
> 	ret = -ENOMEM;
> 	goto out;
> }
> 
> mutex_lock(&ngrp->kobj_lock);
> if (ngrp->nattrs[nid]) {
> 	mutex_unlock(&ngrp->kobj_lock);
> 	kfree(node_attr);
> 	pr_info("Node [%d] already exists\n");
> 	goto out;
> }
> ngrp->attrs[nid] = node_attr;
> mutex_unlock(&ngrp->kobj_lock):
> 

Your suggestion makes sense, and I will update this part accordingly
to reflect your feedback.

> 
> > +	} else {
> > +		mutex_unlock(&ngrp->kobj_lock);
> > +		pr_info("Node [%d] is already existed\n", nid);
> 
> NIT: To keep consistency with other parts of the kernel, maybe this can be
> rephrased to "Node [%d] already exists\n"

I also agree that modifying the wording would improve clarity.
I will make the necessary adjustments in the next version.

> 
> > +		goto out;
> > +	}
> > +	mutex_unlock(&ngrp->kobj_lock);
> > +
> > +	if (!ngrp->nattrs[nid]) {
> > +		ret = -ENOMEM;
> > +		goto out;
> > +	}
> 
> If we make the changes above, we don't have to check for allocation success
> *after* already having locked & unlocked and making the unnecessary assignment.
> 
> >  
> >  	name = kasprintf(GFP_KERNEL, "node%d", nid);
> >  	if (!name) {
> > -		kfree(node_attr);
> > -		return -ENOMEM;
> > +		kfree(ngrp->nattrs[nid]);
> > +		ret = -ENOMEM;
> > +		goto out;
> >  	}
> 
> For the same reasons above, I think it makes sense to make this allocation
> together with the allocation of node_attr above, and free / return -ENOMEM
> as early as possible if we can.
> 
> [...snip...]
> 
> Thank you again for this patch! Please let me know what you think : -)
> Have a great day!
> Joshua

Thank you for your detailed and thoughtful review. I will incorporate
your suggestions and update the next version accordingly.

> 
> Sent using hkml (https://github.com/sjp38/hackermail)
>