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

[Joshua Hahn <joshua.hahnjy@gmail.com>]

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):


> +	} 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"

> +		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

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