[PATCH] mm/mempolicy: Fix exception handling in shared_policy_replace()

[PATCH] mm/mempolicy: Fix exception handling in shared_policy_replace()

[Markus Elfring]

Date: Thu, 23 Mar 2023 18:18:59 +0100

The label “err_out” was used to jump to another pointer check despite of
the detail in the implementation of the function “shared_policy_replace”
that it was determined already that a corresponding variable contained a
null pointer because of a failed call of the function “kmem_cache_alloc”.

1. Use more appropriate labels instead.

2. The implementation of the function “mpol_put” contains a pointer check
   for its single input parameter.
   Thus delete a redundant check in the caller.


This issue was detected by using the Coccinelle software.

Fixes: 42288fe366c4f1ce7522bc9f27d0bc2a81c55264 ("mm: mempolicy: Convert shared_policy mutex to spinlock")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 mm/mempolicy.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index a256a241fd1d..fb0485688dcb 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -2736,13 +2736,12 @@ static int shared_policy_replace(struct shared_policy *sp, unsigned long start,
         sp_insert(sp, new);
     write_unlock(&sp->lock);
     ret = 0;
+put_mpol:
+    mpol_put(mpol_new);
 
-err_out:
-    if (mpol_new)
-        mpol_put(mpol_new);
     if (n_new)
         kmem_cache_free(sn_cache, n_new);
-
+exit:
     return ret;
 
 alloc_new:
@@ -2750,10 +2749,10 @@ static int shared_policy_replace(struct shared_policy *sp, unsigned long start,
     ret = -ENOMEM;
     n_new = kmem_cache_alloc(sn_cache, GFP_KERNEL);
     if (!n_new)
-        goto err_out;
+        goto exit;
     mpol_new = kmem_cache_alloc(policy_cache, GFP_KERNEL);
     if (!mpol_new)
-        goto err_out;
+        goto put_mpol;
     atomic_set(&mpol_new->refcnt, 1);
     goto restart;
 }
--
2.40.0

Re: [PATCH] mm/mempolicy: Fix exception handling in shared_policy_replace()

[Vlastimil Babka]

Your patch doesn't apply, seems like it uses spaces instead of tabs. Also I
can't use 'b4' to download it as there are multiple different patches using
the same message-id:

https://lore.kernel.org/all/6e9ca062-939b-af96-c8ff-56ad485d6e79@web.de/

Re: subject, I don't see a bug that this would fix. You could say it's
"cleanup" and this function could use one, but for a cleanup it's not
improving the situation much.

On 3/23/23 18:30, Markus Elfring wrote:
> Date: Thu, 23 Mar 2023 18:18:59 +0100
> 
> The label “err_out” was used to jump to another pointer check despite of
> the detail in the implementation of the function “shared_policy_replace”
> that it was determined already that a corresponding variable contained a
> null pointer because of a failed call of the function “kmem_cache_alloc”.
> 
> 1. Use more appropriate labels instead.
> 
> 2. The implementation of the function “mpol_put” contains a pointer check
>    for its single input parameter.
>    Thus delete a redundant check in the caller.
> 
> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: 42288fe366c4f1ce7522bc9f27d0bc2a81c55264 ("mm: mempolicy: Convert shared_policy mutex to spinlock")

Again this is not a fix.

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  mm/mempolicy.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index a256a241fd1d..fb0485688dcb 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -2736,13 +2736,12 @@ static int shared_policy_replace(struct shared_policy *sp, unsigned long start,
>          sp_insert(sp, new);
>      write_unlock(&sp->lock);
>      ret = 0;
> +put_mpol:
> +    mpol_put(mpol_new);
>  
> -err_out:
> -    if (mpol_new)
> -        mpol_put(mpol_new);
>      if (n_new)
>          kmem_cache_free(sn_cache, n_new);
> -
> +exit:
>      return ret;
>  
>  alloc_new:
> @@ -2750,10 +2749,10 @@ static int shared_policy_replace(struct shared_policy *sp, unsigned long start,
>      ret = -ENOMEM;
>      n_new = kmem_cache_alloc(sn_cache, GFP_KERNEL);
>      if (!n_new)
> -        goto err_out;
> +        goto exit;

Just "return ret" and no need for exit label?

>      mpol_new = kmem_cache_alloc(policy_cache, GFP_KERNEL);
>      if (!mpol_new)
> -        goto err_out;
> +        goto put_mpol;

We are doing this because mpol_new == NULL, so we know there's no reason to
do mpol_put(), we could jump to the freeing of n_new.

>      atomic_set(&mpol_new->refcnt, 1);
>      goto restart;
>  }
> --
> 2.40.0
> 
> 
> 

Re: [PATCH] mm/mempolicy: Fix exception handling in shared_policy_replace()

[Markus Elfring]

> Your patch doesn't apply, seems like it uses spaces instead of tabs.

I am sorry for this glitch.

Andrew Morton picked my change suggestion up yesterday.
https://git.kernel.org/pub/scm/linux/kernel/git/akpm/25-new.git/tree/patches/mm-mempolicy-fix-exception-handling-in-shared_policy_replace.patch


> https://lore.kernel.org/all/6e9ca062-939b-af96-c8ff-56ad485d6e79@web.de/
>
> Re: subject, I don't see a bug that this would fix. You could say it's
> "cleanup" and this function could use one, but for a cleanup it's not
> improving the situation much.

I find a few details improvable also for the mentioned function implementation.


>> The label “err_out” was used to jump to another pointer check despite of
>> the detail in the implementation of the function “shared_policy_replace”
>> that it was determined already that a corresponding variable contained a
>> null pointer because of a failed call of the function “kmem_cache_alloc”.
>>
>> 1. Use more appropriate labels instead.
>>
>> 2. The implementation of the function “mpol_put” contains a pointer check
>>    for its single input parameter.
>>    Thus delete a redundant check in the caller.
>>
>>
>> This issue was detected by using the Coccinelle software.
>>
>> Fixes: 42288fe366c4f1ce7522bc9f27d0bc2a81c55264 ("mm: mempolicy: Convert shared_policy mutex to spinlock")
>
> Again this is not a fix.

Do you find the change description helpful?

Regards,
Markus

[PATCH] selftests: cgroup: Fix exception handling in test_memcg_oom_group_score_events()

[Markus Elfring]

Date: Sat, 25 Mar 2023 19:11:13 +0100

The label “cleanup” was used to jump to another pointer check despite of
the detail in the implementation of the function
“test_memcg_oom_group_score_events” that it was determined already
that a corresponding variable contained a null pointer.

1. Thus return directly after a call of the function “cg_name” failed.

2. Use an additional label.

3. Delete a questionable check.


This issue was detected by using the Coccinelle software.

Fixes: a987785dcd6c8ae2915460582aebd6481c81eb67 ("Add tests for memory.oom.group")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 tools/testing/selftests/cgroup/test_memcontrol.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
index f4f7c0aef702..afcd1752413e 100644
--- a/tools/testing/selftests/cgroup/test_memcontrol.c
+++ b/tools/testing/selftests/cgroup/test_memcontrol.c
@@ -1242,12 +1242,11 @@ static int test_memcg_oom_group_score_events(const char *root)
 	int safe_pid;

 	memcg = cg_name(root, "memcg_test_0");
-
 	if (!memcg)
-		goto cleanup;
+		return ret;

 	if (cg_create(memcg))
-		goto cleanup;
+		goto free_cg;

 	if (cg_write(memcg, "memory.max", "50M"))
 		goto cleanup;
@@ -1275,8 +1274,8 @@ static int test_memcg_oom_group_score_events(const char *root)
 	ret = KSFT_PASS;

 cleanup:
-	if (memcg)
-		cg_destroy(memcg);
+	cg_destroy(memcg);
+free_cg:
 	free(memcg);

 	return ret;
--
2.40.0

Re: [PATCH] selftests: cgroup: Fix exception handling in test_memcg_oom_group_score_events()

[Lorenzo Stoakes]

On Sat, Mar 25, 2023 at 07:30:21PM +0100, Markus Elfring wrote:
> Date: Sat, 25 Mar 2023 19:11:13 +0100
>
> The label “cleanup” was used to jump to another pointer check despite of
> the detail in the implementation of the function
> “test_memcg_oom_group_score_events” that it was determined already
> that a corresponding variable contained a null pointer.

This is poorly writte and confusing. Something like 'avoid unnecessary null
check/cg_destroy() invocation' would be far clearer.

>
> 1. Thus return directly after a call of the function “cg_name” failed.
>

This feels superfluious.

> 2. Use an additional label.

This also feels superfluious.

>
> 3. Delete a questionable check.

This seems superfluois and frankly, rude. It's not questionable, it's
readable, you should try to avoid language like 'questionable' when the
purpose of the check is obvious.

>
>
> This issue was detected by using the Coccinelle software.
>
> Fixes: a987785dcd6c8ae2915460582aebd6481c81eb67 ("Add tests for memory.oom.group")

Fixes what in the what now? This is not a bug fix, it's a 'questionable'
refactoring.

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  tools/testing/selftests/cgroup/test_memcontrol.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
> index f4f7c0aef702..afcd1752413e 100644
> --- a/tools/testing/selftests/cgroup/test_memcontrol.c
> +++ b/tools/testing/selftests/cgroup/test_memcontrol.c
> @@ -1242,12 +1242,11 @@ static int test_memcg_oom_group_score_events(const char *root)
>  	int safe_pid;
>
>  	memcg = cg_name(root, "memcg_test_0");
> -
>  	if (!memcg)
> -		goto cleanup;
> +		return ret;
>
>  	if (cg_create(memcg))
> -		goto cleanup;
> +		goto free_cg;
>
>  	if (cg_write(memcg, "memory.max", "50M"))
>  		goto cleanup;
> @@ -1275,8 +1274,8 @@ static int test_memcg_oom_group_score_events(const char *root)
>  	ret = KSFT_PASS;
>
>  cleanup:
> -	if (memcg)
> -		cg_destroy(memcg);
> +	cg_destroy(memcg);
> +free_cg:
>  	free(memcg);
>
>  	return ret;
> --
> 2.40.0
>
>

I dislike this patch, it adds complexity for no discernible purpose and
actively makes the code _less_ readable and in a self-test of all places (!)

Not all pedantic Coccinelle results are actionable. Remember that it's
humans who are reading the code.

Your email client/scripting is still somehow broken, I couldn't get b4 to
pull it correctly and it seems to have a duplicate message ID. You really
need to take a look at that (git send-email should do this fine for
example).

Please try to filter the output of Coccinelle and instead of spamming
thousands of pointless patches that add no value, try to choose those that
do add value.

My advice overall would be to just stop.

Re: [PATCH] selftests: cgroup: Fix exception handling in test_memcg_oom_group_score_events()

[Markus Elfring]

>> The label “cleanup” was used to jump to another pointer check despite of
>> the detail in the implementation of the function
>> “test_memcg_oom_group_score_events” that it was determined already
>> that a corresponding variable contained a null pointer.
>
> This is poorly writte and confusing.

A special source code combination was detected.


Reconsidering repeated pointer checks with SmPL
https://lore.kernel.org/cocci/f9303bdc-b1a7-be5e-56c6-dfa8232b8b55@web.de/
https://sympa.inria.fr/sympa/arc/cocci/2023-03/msg00017.html



> Something like 'avoid unnecessary null check/cg_destroy() invocation'
> would be far clearer.

I (or another interested contributor) can integrate such information into
a subsequent patch if the change acceptance will grow accordingly.


>> 1. Thus return directly after a call of the function “cg_name” failed.
>
> This feels superfluious.

Under which circumstances would you care more for the advice
“If there is no cleanup needed then just return directly.”
from the section “Centralized exiting of functions”?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.3-rc3#n532


>> 2. Use an additional label.
>
> This also feels superfluious.

The marking of special source code positions is probably required for
the presented design goal.


>> 3. Delete a questionable check.
>
> This seems superfluois and frankly, rude. It's not questionable,

Can you find the combination of code fragments like “if (!memcg)”
and “if (memcg)” suspicious?


> it's readable, you should try to avoid language like 'questionable' when the
> purpose of the check is obvious.

I tend to prefer an other known design variant at this place.


>> This issue was detected by using the Coccinelle software.
>>
>> Fixes: a987785dcd6c8ae2915460582aebd6481c81eb67 ("Add tests for memory.oom.group")
>
> Fixes what in the what now?

1. Check repetition (which can be undesirable)

2. Can a cg_destroy() call ever work as expected if a cg_create() call failed?


>> +++ b/tools/testing/selftests/cgroup/test_memcontrol.c
>> @@ -1242,12 +1242,11 @@ static int test_memcg_oom_group_score_events(const char *root)
>>  	int safe_pid;
>>
>>  	memcg = cg_name(root, "memcg_test_0");
>> -
>>  	if (!memcg)
>> -		goto cleanup;
>> +		return ret;
>>
>>  	if (cg_create(memcg))
>> -		goto cleanup;
>> +		goto free_cg;
>>
>>  	if (cg_write(memcg, "memory.max", "50M"))
>>  		goto cleanup;
>> @@ -1275,8 +1274,8 @@ static int test_memcg_oom_group_score_events(const char *root)
>>  	ret = KSFT_PASS;
>>
>>  cleanup:
>> -	if (memcg)
>> -		cg_destroy(memcg);
>> +	cg_destroy(memcg);
>> +free_cg:
>>  	free(memcg);
>>
>>  	return ret;
>> --
…
> I dislike this patch, it adds complexity for no discernible purpose and
> actively makes the code _less_ readable and in a self-test of all places (!)

It seems that there is a conflict to resolve also according to another
information source.
https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources#MEM12C.Considerusingagotochainwhenleavingafunctiononerrorwhenusingandreleasingresources-CompliantSolution%28POSIX,GotoChain%29


Regards,
Markus

Re: [PATCH] selftests: cgroup: Fix exception handling in test_memcg_oom_group_score_events()

[David Vernet]

On Sun, Mar 26, 2023 at 10:15:31AM +0200, Markus Elfring wrote:

[...]

> >>
> >> Fixes: a987785dcd6c8ae2915460582aebd6481c81eb67 ("Add tests for memory.oom.group")
> >
> > Fixes what in the what now?
> 
> 1. Check repetition (which can be undesirable)
> 
> 2. Can a cg_destroy() call ever work as expected if a cg_create() call failed?

Perhaps next time you can answer your own question by spending 30
seconds actually reading the code you're "fixing":

int cg_destroy(const char *cgroup)
{
        int ret;

retry:
        ret = rmdir(cgroup);
        if (ret && errno == EBUSY) {
                cg_killall(cgroup);
                usleep(100);
                goto retry;
        }

        if (ret && errno == ENOENT) <<< that case is explicitly handled here
                ret = 0;

        return ret;
}

Re: [PATCH] selftests: cgroup: Fix exception handling in test_memcg_oom_group_score_events()

[Markus Elfring]

>> 2. Can a cg_destroy() call ever work as expected if a cg_create() call failed?
>
> Perhaps next time you can answer your own question by spending 30
> seconds actually reading the code you're "fixing":
>
> int cg_destroy(const char *cgroup)
> {
…
>         ret = rmdir(cgroup);
…
>         if (ret && errno == ENOENT) <<< that case is explicitly handled here
>                 ret = 0;
>
>         return ret;
> }

Is it interesting somehow that a non-existing directory (which would occasionally
not be found) is tolerated so far?
https://elixir.bootlin.com/linux/v6.3-rc3/source/tools/testing/selftests/cgroup/cgroup_util.c#L285

Should such a function call be avoided because of a failed cg_create() call?

Regards,
Markus

Re: [PATCH] selftests: cgroup: Fix exception handling in test_memcg_oom_group_score_events()

[David Vernet]

On Mon, Mar 27, 2023 at 07:56:03AM +0200, Markus Elfring wrote:
> >> 2. Can a cg_destroy() call ever work as expected if a cg_create() call failed?
> >
> > Perhaps next time you can answer your own question by spending 30
> > seconds actually reading the code you're "fixing":
> >
> > int cg_destroy(const char *cgroup)
> > {
> …
> >         ret = rmdir(cgroup);
> …
> >         if (ret && errno == ENOENT) <<< that case is explicitly handled here
> >                 ret = 0;
> >
> >         return ret;
> > }
> 
> Is it interesting somehow that a non-existing directory (which would occasionally
> not be found) is tolerated so far?
> https://elixir.bootlin.com/linux/v6.3-rc3/source/tools/testing/selftests/cgroup/cgroup_util.c#L285
> 
> Should such a function call be avoided because of a failed cg_create() call?

The point is that (a) you were wrong that this is fixing anything, and
(b) this patch is functionally useless. Sure, we could move some goto's
around and subjectively improve "something". Why?  What's the point?
It's highly debatable that what you're doing is even an improvement, and
I'm not interested in wasting time pontificating about the merits of a
trivial "fix" for a test cleanup function that isn't even broken.

Several people have already either advised or directly asked you to stop
sending these patches. I'm not sure why you're choosing to ignore them,
but I'll throw my hat in the ring regardless and do the same. Please
stop sending these fake cleanup patches.