[PATCH] sysctl: Reduce dput(child) calls in proc_sys_fill_cache()

[PATCH] sysctl: Reduce dput(child) calls in proc_sys_fill_cache()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 26 Sep 2024 10:10:33 +0200

A dput(child) call was immediately used after an error pointer check
for a d_splice_alias() call in this function implementation.
Thus call such a function instead directly before the check.

This issue was transformed by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 fs/proc/proc_sysctl.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index d11ebc055ce0..97547de58218 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -698,11 +698,11 @@ static bool proc_sys_fill_cache(struct file *file,
 			res = d_splice_alias(inode, child);
 			d_lookup_done(child);
 			if (unlikely(res)) {
-				if (IS_ERR(res)) {
-					dput(child);
-					return false;
-				}
 				dput(child);
+
+				if (IS_ERR(res))
+					return false;
+
 				child = res;
 			}
 		}
--
2.46.1

Re: [PATCH] sysctl: Reduce dput(child) calls in proc_sys_fill_cache()

[Joel Granados]

On Thu, Sep 26, 2024 at 10:20:34AM +0200, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 26 Sep 2024 10:10:33 +0200
> 
> A dput(child) call was immediately used after an error pointer check
> for a d_splice_alias() call in this function implementation.
> Thus call such a function instead directly before the check.
This message reads funny, please re-write for your v2. Here is how I would write
it.

"
Replace two dput(child) calls with one that occurs immediately before the IS_ERR
evaluation. This is ok because dput gets called regardless of the value returned
by IS_ERR(res).
"

> 
> This issue was transformed by using the Coccinelle software.
How long is the coccinelle script? If it is a reasonable size, can you please
append it to the commit message. If in doubt of what "reasonable" means, just
share it to the list before doing your V2.

> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  fs/proc/proc_sysctl.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index d11ebc055ce0..97547de58218 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -698,11 +698,11 @@ static bool proc_sys_fill_cache(struct file *file,
>  			res = d_splice_alias(inode, child);
>  			d_lookup_done(child);
>  			if (unlikely(res)) {
> -				if (IS_ERR(res)) {
> -					dput(child);
> -					return false;
> -				}
>  				dput(child);
> +
> +				if (IS_ERR(res))
> +					return false;
> +
>  				child = res;
>  			}
>  		}
> --
> 2.46.1
> 

-- 

Joel Granados

Re: sysctl: Reduce dput(child) calls in proc_sys_fill_cache()

[Markus Elfring]

>> A dput(child) call was immediately used after an error pointer check
>> for a d_splice_alias() call in this function implementation.
>> Thus call such a function instead directly before the check.
> This message reads funny, please re-write for your v2. Here is how I would write
> it.
>
> "
> Replace two dput(child) calls with one that occurs immediately before the IS_ERR
> evaluation. This is ok because dput gets called regardless of the value returned
> by IS_ERR(res).
> "

Do you prefer the mentioned macro name over the wording “error pointer check”?


>> This issue was transformed by using the Coccinelle software.
> How long is the coccinelle script? …

A related script for the semantic patch language was presented already according to
the clarification approach “Generalising a transformation with SmPL?”.
https://lore.kernel.org/kernel-janitors/300b5d1a-ab88-4548-91d2-0792bc15e15e@web.de/
https://lkml.org/lkml/2024/9/14/464
https://sympa.inria.fr/sympa/arc/cocci/2024-09/msg00004.html

Will further development ideas evolve accordingly?

Regards,
Markus

Re: Re: sysctl: Reduce dput(child) calls in proc_sys_fill_cache()

[Joel Granados]

On Wed, Oct 23, 2024 at 02:10:57PM +0200, Markus Elfring wrote:
> >> A dput(child) call was immediately used after an error pointer check
> >> for a d_splice_alias() call in this function implementation.
> >> Thus call such a function instead directly before the check.
> > This message reads funny, please re-write for your v2. Here is how I would write
> > it.
> >
> > "
> > Replace two dput(child) calls with one that occurs immediately before the IS_ERR
> > evaluation. This is ok because dput gets called regardless of the value returned
> > by IS_ERR(res).
> > "
> 
> Do you prefer the mentioned macro name over the wording “error pointer check”?
yes.

> 
> 
> >> This issue was transformed by using the Coccinelle software.
> > How long is the coccinelle script? …
> 
> A related script for the semantic patch language was presented already according to
> the clarification approach “Generalising a transformation with SmPL?”.
> https://lore.kernel.org/kernel-janitors/300b5d1a-ab88-4548-91d2-0792bc15e15e@web.de/
> https://lkml.org/lkml/2024/9/14/464
> https://sympa.inria.fr/sympa/arc/cocci/2024-09/msg00004.html
There where several scripts in these links but non of them where too
long. Can you please append the one you used for this patch to the
commit message.

Thx

-- 

Joel Granados

[PATCH v2] sysctl: Reduce dput(child) calls in proc_sys_fill_cache()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 23 Oct 2024 16:54:59 +0200

Replace two dput(child) calls with one that occurs immediately before
the IS_ERR evaluation. This transformation can be performed because
dput() gets called regardless of the value returned by IS_ERR(res).

This issue was transformed by using a script for the
semantic patch language like the following.
<SmPL>
@extended_adjustment@
expression e, f != { mutex_unlock }, x, y;
@@
+f(e);
 if (...)
 {
 <+... when != \( e = x \| y(..., &e, ...) \)
-   f(e);
 ...+>
 }
-f(e);
</SmPL>

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---

V2:
* This update suggestion was rebased on source files of the software
  “Linux next-20241023”.

* The change description was adjusted according to the wording preferences
  by Joel Granados.

* An SmPL script example was appended.


 fs/proc/proc_sysctl.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 9b9dfc450cb3..b277a1ca392e 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -698,11 +698,11 @@ static bool proc_sys_fill_cache(struct file *file,
 			res = d_splice_alias(inode, child);
 			d_lookup_done(child);
 			if (unlikely(res)) {
-				if (IS_ERR(res)) {
-					dput(child);
-					return false;
-				}
 				dput(child);
+
+				if (IS_ERR(res))
+					return false;
+
 				child = res;
 			}
 		}
--
2.47.0

Re: [PATCH v2] sysctl: Reduce dput(child) calls in proc_sys_fill_cache()

[Joel Granados]

On Wed, Oct 23, 2024 at 05:27:11PM +0200, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 23 Oct 2024 16:54:59 +0200
> 
> Replace two dput(child) calls with one that occurs immediately before
> the IS_ERR evaluation. This transformation can be performed because
> dput() gets called regardless of the value returned by IS_ERR(res).
> 
> This issue was transformed by using a script for the
> semantic patch language like the following.
> <SmPL>
Aren't you missing a "virtual patch" here?

Is there another way to run it besides this command:
make coccicheck MODE=patch SPFLAGS="--in-place --include-headers --smpl-spacing --jobs=16" COCCI=SCRIPT

Best
> @extended_adjustment@
> expression e, f != { mutex_unlock }, x, y;
> @@
> +f(e);
>  if (...)
>  {
>  <+... when != \( e = x \| y(..., &e, ...) \)
> -   f(e);
>  ...+>
>  }
> -f(e);
> </SmPL>
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> 
> V2:
> * This update suggestion was rebased on source files of the software
>   “Linux next-20241023”.
> 
> * The change description was adjusted according to the wording preferences
>   by Joel Granados.
> 
> * An SmPL script example was appended.
> 
> 
>  fs/proc/proc_sysctl.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index 9b9dfc450cb3..b277a1ca392e 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -698,11 +698,11 @@ static bool proc_sys_fill_cache(struct file *file,
>  			res = d_splice_alias(inode, child);
>  			d_lookup_done(child);
>  			if (unlikely(res)) {
> -				if (IS_ERR(res)) {
> -					dput(child);
> -					return false;
> -				}
>  				dput(child);
> +
> +				if (IS_ERR(res))
> +					return false;
> +
>  				child = res;
>  			}
>  		}
> --
> 2.47.0
> 

-- 

Joel Granados

Re: [v2] sysctl: Reduce dput(child) calls in proc_sys_fill_cache()

[Markus Elfring]

…
>> This issue was transformed by using a script for the
>> semantic patch language like the following.
>> <SmPL>
> Aren't you missing a "virtual patch" here?

Yes.

I presented an SmPL script example which would not be directly used
for the tool “coccicheck”.


> Is there another way to run it besides this command:

Another command example:
time spatch -timeout 23 -j4 --chunksize 1 --no-loops -dir . …/Projekte/Coccinelle/janitor/avoid_duplicate_function_call2.cocci > …/Projekte/Bau/Linux/scripts/Coccinelle/tuning1/next/20240913/avoid_duplicate_function_call2-no_loops.diff 2> …/Projekte/Bau/Linux/scripts/Coccinelle/tuning1/next/20240913/avoid_duplicate_function_call2-no_loops-errors.txt


Regards,
Markus

Re: [PATCH v2] sysctl: Reduce dput(child) calls in proc_sys_fill_cache()

[Joel Granados]

On Wed, Oct 23, 2024 at 05:27:11PM +0200, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 23 Oct 2024 16:54:59 +0200

Small comment: Remember to send out a new patch-set email in order to
make it easier for tools like b4.

> 
> Replace two dput(child) calls with one that occurs immediately before
> the IS_ERR evaluation. This transformation can be performed because
> dput() gets called regardless of the value returned by IS_ERR(res).
> 
> This issue was transformed by using a script for the
> semantic patch language like the following.
> <SmPL>
> @extended_adjustment@
> expression e, f != { mutex_unlock }, x, y;
> @@
> +f(e);
>  if (...)
>  {
>  <+... when != \( e = x \| y(..., &e, ...) \)
> -   f(e);
>  ...+>
>  }
> -f(e);
> </SmPL>
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> 
> V2:
> * This update suggestion was rebased on source files of the software
>   “Linux next-20241023”.
> 
> * The change description was adjusted according to the wording preferences
>   by Joel Granados.
> 
> * An SmPL script example was appended.
> 
> 
>  fs/proc/proc_sysctl.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index 9b9dfc450cb3..b277a1ca392e 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -698,11 +698,11 @@ static bool proc_sys_fill_cache(struct file *file,
>  			res = d_splice_alias(inode, child);
>  			d_lookup_done(child);
>  			if (unlikely(res)) {
> -				if (IS_ERR(res)) {
> -					dput(child);
> -					return false;
> -				}
>  				dput(child);
> +
> +				if (IS_ERR(res))
> +					return false;
> +
>  				child = res;
>  			}
>  		}
> --
> 2.47.0
> 

This looks good to me. I'll push it to sysctl-next.
Thx for the patch

Reviewed-by: Joel Granados <joel.granados@kernel.org>

Best

-- 

Joel Granados