[v1] mm/mempolicy: Fix error code in sysfs_wi_node_add()

[PATCH next] mm/mempolicy: Fix error code in sysfs_wi_node_add()

Posted by Dan Carpenter 7 months, 4 weeks ago

Return -EEXIST if the node already exists.  Don't return success.

Fixes: 1bf270ac1b0a ("mm/mempolicy: support memory hotplug in weighted interleave")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
Potentially returning success was intentional?  This is from static
analysis and I can't be totally sure.

 mm/mempolicy.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index f43951668c41..0538a994440a 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -3539,7 +3539,7 @@ static const struct kobj_type wi_ktype = {
 
 static int sysfs_wi_node_add(int nid)
 {
-	int ret = 0;
+	int ret;
 	char *name;
 	struct iw_node_attr *new_attr;
 
@@ -3569,6 +3569,7 @@ static int sysfs_wi_node_add(int nid)
 	if (wi_group->nattrs[nid]) {
 		mutex_unlock(&wi_group->kobj_lock);
 		pr_info("node%d already exists\n", nid);
+		ret = -EEXIST;
 		goto out;
 	}
 
-- 
2.47.2

Re: [PATCH next] mm/mempolicy: Fix error code in sysfs_wi_node_add()

Posted by Honggyu Kim 7 months, 2 weeks ago

Hi Dan,

On 4/23/2025 5:24 PM, Dan Carpenter wrote:
> Return -EEXIST if the node already exists.  Don't return success.
> 
> Fixes: 1bf270ac1b0a ("mm/mempolicy: support memory hotplug in weighted interleave")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> Potentially returning success was intentional?  This is from static
> analysis and I can't be totally sure.
> 
>   mm/mempolicy.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index f43951668c41..0538a994440a 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -3539,7 +3539,7 @@ static const struct kobj_type wi_ktype = {
>   
>   static int sysfs_wi_node_add(int nid)
>   {
> -	int ret = 0;
> +	int ret;
>   	char *name;
>   	struct iw_node_attr *new_attr;
>   
> @@ -3569,6 +3569,7 @@ static int sysfs_wi_node_add(int nid)
>   	if (wi_group->nattrs[nid]) {
>   		mutex_unlock(&wi_group->kobj_lock);
>   		pr_info("node%d already exists\n", nid);
> +		ret = -EEXIST;

Returning -EEXIST here looks good to me, but could you remove the above pr_info
as well?  I mean the following change is needed.

-		pr_info("node%d already exists\n", nid)
+		ret = -EEXIST;

We don't need the above pr_info here because we delegate a warning message to
its caller wi_node_notifier().

This can close another warning report below.
https://lore.kernel.org/all/202505020458.yLHRAaW9-lkp@intel.com

If you apply my suggestion then please add

	Reviewed-by: Honggyu Kim <honggyu.kim@sk.com>

Thanks,
Honggyu

>   		goto out;
>   	}
>

Re: [PATCH next] mm/mempolicy: Fix error code in sysfs_wi_node_add()

Posted by Dan Carpenter 7 months, 2 weeks ago

On Fri, May 02, 2025 at 03:46:21PM +0900, Honggyu Kim wrote:
> Hi Dan,
> 
> On 4/23/2025 5:24 PM, Dan Carpenter wrote:
> > Return -EEXIST if the node already exists.  Don't return success.
> > 
> > Fixes: 1bf270ac1b0a ("mm/mempolicy: support memory hotplug in weighted interleave")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> > Potentially returning success was intentional?  This is from static
> > analysis and I can't be totally sure.
> > 
> >   mm/mempolicy.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> > index f43951668c41..0538a994440a 100644
> > --- a/mm/mempolicy.c
> > +++ b/mm/mempolicy.c
> > @@ -3539,7 +3539,7 @@ static const struct kobj_type wi_ktype = {
> >   static int sysfs_wi_node_add(int nid)
> >   {
> > -	int ret = 0;
> > +	int ret;
> >   	char *name;
> >   	struct iw_node_attr *new_attr;
> > @@ -3569,6 +3569,7 @@ static int sysfs_wi_node_add(int nid)
> >   	if (wi_group->nattrs[nid]) {
> >   		mutex_unlock(&wi_group->kobj_lock);
> >   		pr_info("node%d already exists\n", nid);
> > +		ret = -EEXIST;
> 
> Returning -EEXIST here looks good to me, but could you remove the above pr_info
> as well?  I mean the following change is needed.
> 
> -		pr_info("node%d already exists\n", nid)
> +		ret = -EEXIST;
> 
> We don't need the above pr_info here because we delegate a warning message to
> its caller wi_node_notifier().
> 
> This can close another warning report below.
> https://lore.kernel.org/all/202505020458.yLHRAaW9-lkp@intel.com
> 
> If you apply my suggestion then please add
> 
> 	Reviewed-by: Honggyu Kim <honggyu.kim@sk.com>
> 

Rakie Kim was pretty confident that returning 0 was intentional.  Btw,
Smatch considers it intentional if the "ret = 0;" is within 5
lines of the goto.  Or we could add a comment, which wouldn't silence
the warning but it would help people reading the code.

regards,
dan carpenter

Re: [PATCH next] mm/mempolicy: Fix error code in sysfs_wi_node_add()

Posted by Rakie Kim 7 months, 2 weeks ago

On Fri, 2 May 2025 10:10:33 +0300 Dan Carpenter <dan.carpenter@linaro.org> wrote:
> On Fri, May 02, 2025 at 03:46:21PM +0900, Honggyu Kim wrote:
> > Hi Dan,
> > 
> > On 4/23/2025 5:24 PM, Dan Carpenter wrote:
> > > Return -EEXIST if the node already exists.  Don't return success.
> > > 
> > > Fixes: 1bf270ac1b0a ("mm/mempolicy: support memory hotplug in weighted interleave")
> > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > ---
> > > Potentially returning success was intentional?  This is from static
> > > analysis and I can't be totally sure.
> > > 
> > >   mm/mempolicy.c | 3 ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> > > index f43951668c41..0538a994440a 100644
> > > --- a/mm/mempolicy.c
> > > +++ b/mm/mempolicy.c
> > > @@ -3539,7 +3539,7 @@ static const struct kobj_type wi_ktype = {
> > >   static int sysfs_wi_node_add(int nid)
> > >   {
> > > -	int ret = 0;
> > > +	int ret;
> > >   	char *name;
> > >   	struct iw_node_attr *new_attr;
> > > @@ -3569,6 +3569,7 @@ static int sysfs_wi_node_add(int nid)
> > >   	if (wi_group->nattrs[nid]) {
> > >   		mutex_unlock(&wi_group->kobj_lock);
> > >   		pr_info("node%d already exists\n", nid);
> > > +		ret = -EEXIST;
> > 
> > Returning -EEXIST here looks good to me, but could you remove the above pr_info
> > as well?  I mean the following change is needed.
> > 
> > -		pr_info("node%d already exists\n", nid)
> > +		ret = -EEXIST;
> > 
> > We don't need the above pr_info here because we delegate a warning message to
> > its caller wi_node_notifier().
> > 
> > This can close another warning report below.
> > https://lore.kernel.org/all/202505020458.yLHRAaW9-lkp@intel.com
> > 
> > If you apply my suggestion then please add
> > 
> > 	Reviewed-by: Honggyu Kim <honggyu.kim@sk.com>
> > 
> 
> Rakie Kim was pretty confident that returning 0 was intentional.  Btw,
> Smatch considers it intentional if the "ret = 0;" is within 5
> lines of the goto.  Or we could add a comment, which wouldn't silence
> the warning but it would help people reading the code.
> 

Hi Dan,

Thank you for taking the time to review this code and point out the issue.
I believe there may have been some confusion related to the behavior in
previous versions.

In the latest revision, the `wi_node_notifier()` function that calls
`sysfs_wi_node_add()` has been updated to always return `NOTIFY_OK`,
regardless of the return value from `sysfs_wi_node_add()`. This ensures that
no memory hotplug event will be blocked by our notifier logic.

static int wi_node_notifier(struct notifier_block *nb,
                            unsigned long action, void *data)
{
	...
	switch (action) {
	case MEM_ONLINE:
		err = sysfs_wi_node_add(nid);
		if (err)
			pr_err("failed to add sysfs for node%d during hotplug: %d\n",
			       nid, err);
		break;
	...
	return NOTIFY_OK;
}

Given that, it is appropriate for `sysfs_wi_node_add()` to return `-EEXIST`
when the node already exists. Since the error message is already logged by
`wi_node_notifier()`, I agree with Honggyu's suggestion to remove the
redundant `pr_info()` statement as well:

-		pr_info("node%d already exists\n", nid);
+		ret = -EEXIST;

Once again, thank you very much for your review and for helping us improve
the code.

Reviewed-by: Rakie Kim <rakie.kim@sk.com>

Rakie

> regards,
> dan carpenter
> 
>

Re: [PATCH next] mm/mempolicy: Fix error code in sysfs_wi_node_add()

Posted by Gregory Price 7 months, 4 weeks ago

On Wed, Apr 23, 2025 at 11:24:58AM +0300, Dan Carpenter wrote:
> Return -EEXIST if the node already exists.  Don't return success.
> 
> Fixes: 1bf270ac1b0a ("mm/mempolicy: support memory hotplug in weighted interleave")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> Potentially returning success was intentional?  This is from static
> analysis and I can't be totally sure.

I think this was intentional to allow hotplug callbacks to continue
executing.  I will let the SK folks who wrote the patch confirm/deny.

If it is intentional, then we need to add a comment here to explain.

~Gregory

> 
>  mm/mempolicy.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index f43951668c41..0538a994440a 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -3539,7 +3539,7 @@ static const struct kobj_type wi_ktype = {
>  
>  static int sysfs_wi_node_add(int nid)
>  {
> -	int ret = 0;
> +	int ret;
>  	char *name;
>  	struct iw_node_attr *new_attr;
>  
> @@ -3569,6 +3569,7 @@ static int sysfs_wi_node_add(int nid)
>  	if (wi_group->nattrs[nid]) {
>  		mutex_unlock(&wi_group->kobj_lock);
>  		pr_info("node%d already exists\n", nid);
> +		ret = -EEXIST;
>  		goto out;
>  	}
>  
> -- 
> 2.47.2
>

Re: [PATCH next] mm/mempolicy: Fix error code in sysfs_wi_node_add()

Posted by Rakie Kim 7 months, 4 weeks ago

On Wed, 23 Apr 2025 12:33:50 -0400 Gregory Price <gourry@gourry.net> wrote:
> On Wed, Apr 23, 2025 at 11:24:58AM +0300, Dan Carpenter wrote:
> > Return -EEXIST if the node already exists.  Don't return success.
> > 
> > Fixes: 1bf270ac1b0a ("mm/mempolicy: support memory hotplug in weighted interleave")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> > Potentially returning success was intentional?  This is from static
> > analysis and I can't be totally sure.
> 
> I think this was intentional to allow hotplug callbacks to continue
> executing.  I will let the SK folks who wrote the patch confirm/deny.
> 
> If it is intentional, then we need to add a comment here to explain.
> 
> ~Gregory
> 
> > 
> >  mm/mempolicy.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> > index f43951668c41..0538a994440a 100644
> > --- a/mm/mempolicy.c
> > +++ b/mm/mempolicy.c
> > @@ -3539,7 +3539,7 @@ static const struct kobj_type wi_ktype = {
> >  
> >  static int sysfs_wi_node_add(int nid)
> >  {
> > -	int ret = 0;
> > +	int ret;
> >  	char *name;
> >  	struct iw_node_attr *new_attr;
> >  
> > @@ -3569,6 +3569,7 @@ static int sysfs_wi_node_add(int nid)
> >  	if (wi_group->nattrs[nid]) {
> >  		mutex_unlock(&wi_group->kobj_lock);
> >  		pr_info("node%d already exists\n", nid);
> > +		ret = -EEXIST;
> >  		goto out;
> >  	}
> >  
> > -- 
> > 2.47.2
> > 
> 

Hi Dan,

Thank you very much for analyzing the issue in this code and for
sharing a detailed patch to address it. Your review is greatly
appreciated.

However, the current behavior of returning success instead of an
-EEXIST or other error code was intentional. I would like to explain
the rationale for this choice and would appreciate your further
thoughts.

The condition:

	if (wi_group->nattrs[nid]) {
		mutex_unlock(&wi_group->kobj_lock);
		pr_info("node%d already exists\n", nid);
		goto out;
	}

is triggered in the following two cases:

1. If `sysfs_wi_node_delete()` fails:
   - This function only performs `sysfs_remove_file()` and frees
     memory, and these operations do not fail in a way that would leave
     the system in an inconsistent state.

2. If `sysfs_wi_node_add()` is invoked multiple times for the same node:
   - While repeated additions for the same node would indicate a
     potential issue in logic, simply skipping the redundant addition
     does not cause a functional problem. The original sysfs entry
     remains valid and continues to work as expected.

Therefore, I chose to return success in this case to avoid interrupting
the flow unnecessarily.

Also, as you pointed out, even if we returned -EEXIST here, it would not
change the runtime behavior. This is because `sysfs_wi_node_add()` is
called from the following memory notifier:

	static int wi_node_notifier(struct notifier_block *nb,
				     unsigned long action, void *data)
	{
		...
		switch (action) {
		case MEM_ONLINE:
			err = sysfs_wi_node_add(nid);
			if (err)
				pr_err("failed to add sysfs for node%d during hotplug: %d\n",
				       nid, err);
			break;
		...
		}
		return NOTIFY_OK;
	}

As discussed in prior reviews (including suggestions by David
Hildenbrand), returning NOTIFY_BAD on failure can interfere with other
notifier chains due to NOTIFY_STOP_MASK behavior. Hence, we always
return NOTIFY_OK to preserve consistent hotplug handling across
subsystems.
I would sincerely appreciate it if you could share any further thoughts
or concerns you may have regarding this decision.

Rakie

Re: [PATCH next] mm/mempolicy: Fix error code in sysfs_wi_node_add()

Posted by Joshua Hahn 7 months, 4 weeks ago

On Wed, 23 Apr 2025 11:24:58 +0300 Dan Carpenter <dan.carpenter@linaro.org> wrote:

Hi Dan,

This makes sense to me! I think that the only way the node can already exist
is if an offlining node didn't properly clean up its sysfs entry, which is
a bug, of course. With that said, I don't think the previous state would have
caused any functional problems, since the same node offlining and onlining
should share the same sysfs entry anyways (unless I'm overlooking something
important...)

This fix will help when the cleanup does fail though, and I think that will
help us assess whether a failed cleanup does indeed cause other problems.

Thank you for spotting this & fixing it! I hope you have a great day : -)

Reviewed-by: Joshua Hahn <joshua.hahnjy@gmail.com>

> Return -EEXIST if the node already exists.  Don't return success.
> 
> Fixes: 1bf270ac1b0a ("mm/mempolicy: support memory hotplug in weighted interleave")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> Potentially returning success was intentional?  This is from static
> analysis and I can't be totally sure.
> 
>  mm/mempolicy.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index f43951668c41..0538a994440a 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -3539,7 +3539,7 @@ static const struct kobj_type wi_ktype = {
>  
>  static int sysfs_wi_node_add(int nid)
>  {
> -	int ret = 0;
> +	int ret;
>  	char *name;
>  	struct iw_node_attr *new_attr;
>  
> @@ -3569,6 +3569,7 @@ static int sysfs_wi_node_add(int nid)
>  	if (wi_group->nattrs[nid]) {
>  		mutex_unlock(&wi_group->kobj_lock);
>  		pr_info("node%d already exists\n", nid);
> +		ret = -EEXIST;
>  		goto out;
>  	}
>  
> -- 
> 2.47.2