[v2,1/5] exec: Only compute current once in flush_old_exec

Commit Message

Eric W. Biederman March 8, 2020, 9:35 p.m. UTC

Make it clear that current only needs to be computed once in
flush_old_exec.  This may have some efficiency improvements and it
makes the code easier to change.

Signed-off-by: "Eric W. Biederman" <[e-mail]>
---
 fs/exec.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Bernd Edlinger March 9, 2020, 1:56 p.m. UTC | #1

On 3/8/20 10:35 PM, Eric W. Biederman wrote:
> 
> Make it clear that current only needs to be computed once in
> flush_old_exec.  This may have some efficiency improvements and it
> makes the code easier to change.
> 
> Signed-off-by: "Eric W. Biederman" <[e-mail]>
> ---
>  fs/exec.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index db17be51b112..c3f34791f2f0 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1260,13 +1260,14 @@ void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
>   */
>  int flush_old_exec(struct linux_binprm * bprm)
>  {
> +	struct task_struct *me = current;
>  	int retval;
>  
>  	/*
>  	 * Make sure we have a private signal table and that
>  	 * we are unassociated from the previous thread group.
>  	 */
> -	retval = de_thread(current);
> +	retval = de_thread(me);
>  	if (retval)
>  		goto out;
>  
> @@ -1294,10 +1295,10 @@ int flush_old_exec(struct linux_binprm * bprm)
>  	bprm->mm = NULL;
>  
>  	set_fs(USER_DS);
> -	current->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD |
> +	me->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD |
>  					PF_NOFREEZE | PF_NO_SETAFFINITY);

I wonder if this line should be aligned with the previous?


Bernd.

Eric W. Biederman March 9, 2020, 5:34 p.m. UTC | #2

Bernd Edlinger <[e-mail]> writes:

> On 3/8/20 10:35 PM, Eric W. Biederman wrote:
>> 
>> Make it clear that current only needs to be computed once in
>> flush_old_exec.  This may have some efficiency improvements and it
>> makes the code easier to change.
>> 
>> Signed-off-by: "Eric W. Biederman" <[e-mail]>
>> ---
>>  fs/exec.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>> 
>> diff --git a/fs/exec.c b/fs/exec.c
>> index db17be51b112..c3f34791f2f0 100644
>> --- a/fs/exec.c
>> +++ b/fs/exec.c
>> @@ -1260,13 +1260,14 @@ void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
>>   */
>>  int flush_old_exec(struct linux_binprm * bprm)
>>  {
>> +	struct task_struct *me = current;
>>  	int retval;
>>  
>>  	/*
>>  	 * Make sure we have a private signal table and that
>>  	 * we are unassociated from the previous thread group.
>>  	 */
>> -	retval = de_thread(current);
>> +	retval = de_thread(me);
>>  	if (retval)
>>  		goto out;
>>  
>> @@ -1294,10 +1295,10 @@ int flush_old_exec(struct linux_binprm * bprm)
>>  	bprm->mm = NULL;
>>  
>>  	set_fs(USER_DS);
>> -	current->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD |
>> +	me->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD |
>>  					PF_NOFREEZE | PF_NO_SETAFFINITY);
>
> I wonder if this line should be aligned with the previous?

In this case I don't think so.  The style used for second line is indent
with tabs as much as possible to the right.  I haven't changed that.

Further mixing a change in indentation style with just a variable rename
will make the patch confusing to read because two things have to be
verified at the same time.

So while I see why you ask I think this bit needs to stay as is.

Eric

Bernd Edlinger March 9, 2020, 5:56 p.m. UTC | #3

On 3/9/20 6:34 PM, Eric W. Biederman wrote:
> Bernd Edlinger <[e-mail]> writes:
> 
>> On 3/8/20 10:35 PM, Eric W. Biederman wrote:
>>>
>>> Make it clear that current only needs to be computed once in
>>> flush_old_exec.  This may have some efficiency improvements and it
>>> makes the code easier to change.
>>>
>>> Signed-off-by: "Eric W. Biederman" <[e-mail]>
>>> ---
>>>  fs/exec.c | 9 +++++----
>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/exec.c b/fs/exec.c
>>> index db17be51b112..c3f34791f2f0 100644
>>> --- a/fs/exec.c
>>> +++ b/fs/exec.c
>>> @@ -1260,13 +1260,14 @@ void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
>>>   */
>>>  int flush_old_exec(struct linux_binprm * bprm)
>>>  {
>>> +	struct task_struct *me = current;
>>>  	int retval;
>>>  
>>>  	/*
>>>  	 * Make sure we have a private signal table and that
>>>  	 * we are unassociated from the previous thread group.
>>>  	 */
>>> -	retval = de_thread(current);
>>> +	retval = de_thread(me);
>>>  	if (retval)
>>>  		goto out;
>>>  
>>> @@ -1294,10 +1295,10 @@ int flush_old_exec(struct linux_binprm * bprm)
>>>  	bprm->mm = NULL;
>>>  
>>>  	set_fs(USER_DS);
>>> -	current->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD |
>>> +	me->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD |
>>>  					PF_NOFREEZE | PF_NO_SETAFFINITY);
>>
>> I wonder if this line should be aligned with the previous?
> 
> In this case I don't think so.  The style used for second line is indent
> with tabs as much as possible to the right.  I haven't changed that.
> 
> Further mixing a change in indentation style with just a variable rename
> will make the patch confusing to read because two things have to be
> verified at the same time.
> 
> So while I see why you ask I think this bit needs to stay as is.
> 

Ah, okay, I see.
Thanks for explaining this rule, I was not aware of it,
but I am still new here :)


Thanks
Bernd.

Bernd Edlinger March 9, 2020, 7:27 p.m. UTC | #4

On 3/9/20 6:56 PM, Bernd Edlinger wrote:
> On 3/9/20 6:34 PM, Eric W. Biederman wrote:
>> Bernd Edlinger <[e-mail]> writes:
>>
>>> On 3/8/20 10:35 PM, Eric W. Biederman wrote:
>>>>
>>>> Make it clear that current only needs to be computed once in
>>>> flush_old_exec.  This may have some efficiency improvements and it
>>>> makes the code easier to change.
>>>>
>>>> Signed-off-by: "Eric W. Biederman" <[e-mail]>
>>>> ---
>>>>  fs/exec.c | 9 +++++----
>>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/fs/exec.c b/fs/exec.c
>>>> index db17be51b112..c3f34791f2f0 100644
>>>> --- a/fs/exec.c
>>>> +++ b/fs/exec.c
>>>> @@ -1260,13 +1260,14 @@ void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
>>>>   */
>>>>  int flush_old_exec(struct linux_binprm * bprm)
>>>>  {
>>>> +	struct task_struct *me = current;
>>>>  	int retval;
>>>>  
>>>>  	/*
>>>>  	 * Make sure we have a private signal table and that
>>>>  	 * we are unassociated from the previous thread group.
>>>>  	 */
>>>> -	retval = de_thread(current);
>>>> +	retval = de_thread(me);
>>>>  	if (retval)
>>>>  		goto out;
>>>>  
>>>> @@ -1294,10 +1295,10 @@ int flush_old_exec(struct linux_binprm * bprm)
>>>>  	bprm->mm = NULL;
>>>>  
>>>>  	set_fs(USER_DS);
>>>> -	current->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD |
>>>> +	me->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD |
>>>>  					PF_NOFREEZE | PF_NO_SETAFFINITY);
>>>
>>> I wonder if this line should be aligned with the previous?
>>
>> In this case I don't think so.  The style used for second line is indent
>> with tabs as much as possible to the right.  I haven't changed that.
>>
>> Further mixing a change in indentation style with just a variable rename
>> will make the patch confusing to read because two things have to be
>> verified at the same time.
>>
>> So while I see why you ask I think this bit needs to stay as is.
>>
> 
> Ah, okay, I see.
> Thanks for explaining this rule, I was not aware of it,
> but I am still new here :)
> 

Reviewed-by: Bernd Edlinger <[e-mail]>


Bernd.

Kees Cook March 10, 2020, 8:17 p.m. UTC | #5

On Sun, Mar 08, 2020 at 04:35:26PM -0500, Eric W. Biederman wrote:
> 
> Make it clear that current only needs to be computed once in
> flush_old_exec.  This may have some efficiency improvements and it
> makes the code easier to change.
> 
> Signed-off-by: "Eric W. Biederman" <[e-mail]>

modulo my suggestion of adding more comments (it could even be kerndoc!)
that explicitly states that "me" should always be "current", yup, looks
good:

Reviewed-by: Kees Cook <[e-mail]>

-Kees

> ---
>  fs/exec.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index db17be51b112..c3f34791f2f0 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1260,13 +1260,14 @@ void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
>   */
>  int flush_old_exec(struct linux_binprm * bprm)
>  {
> +	struct task_struct *me = current;
>  	int retval;
>  
>  	/*
>  	 * Make sure we have a private signal table and that
>  	 * we are unassociated from the previous thread group.
>  	 */
> -	retval = de_thread(current);
> +	retval = de_thread(me);
>  	if (retval)
>  		goto out;
>  
> @@ -1294,10 +1295,10 @@ int flush_old_exec(struct linux_binprm * bprm)
>  	bprm->mm = NULL;
>  
>  	set_fs(USER_DS);
> -	current->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD |
> +	me->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD |
>  					PF_NOFREEZE | PF_NO_SETAFFINITY);
>  	flush_thread();
> -	current->personality &= ~bprm->per_clear;
> +	me->personality &= ~bprm->per_clear;
>  
>  	/*
>  	 * We have to apply CLOEXEC before we change whether the process is
> @@ -1305,7 +1306,7 @@ int flush_old_exec(struct linux_binprm * bprm)
>  	 * trying to access the should-be-closed file descriptors of a process
>  	 * undergoing exec(2).
>  	 */
> -	do_close_on_exec(current->files);
> +	do_close_on_exec(me->files);
>  	return 0;
>  
>  out:
> -- 
> 2.25.0
>

Christian Brauner March 10, 2020, 9:12 p.m. UTC | #6

On Sun, Mar 08, 2020 at 04:35:26PM -0500, Eric W. Biederman wrote:
> 
> Make it clear that current only needs to be computed once in
> flush_old_exec.  This may have some efficiency improvements and it
> makes the code easier to change.
> 
> Signed-off-by: "Eric W. Biederman" <[e-mail]>

Acked-by: Christian Brauner <[e-mail]>

Patch

diff --git a/fs/exec.c b/fs/exec.c
index db17be51b112..c3f34791f2f0 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1260,13 +1260,14 @@  void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
  */
 int flush_old_exec(struct linux_binprm * bprm)
 {
+	struct task_struct *me = current;
 	int retval;
 
 	/*
 	 * Make sure we have a private signal table and that
 	 * we are unassociated from the previous thread group.
 	 */
-	retval = de_thread(current);
+	retval = de_thread(me);
 	if (retval)
 		goto out;
 
@@ -1294,10 +1295,10 @@  int flush_old_exec(struct linux_binprm * bprm)
 	bprm->mm = NULL;
 
 	set_fs(USER_DS);
-	current->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD |
+	me->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD |
 					PF_NOFREEZE | PF_NO_SETAFFINITY);
 	flush_thread();
-	current->personality &= ~bprm->per_clear;
+	me->personality &= ~bprm->per_clear;
 
 	/*
 	 * We have to apply CLOEXEC before we change whether the process is
@@ -1305,7 +1306,7 @@  int flush_old_exec(struct linux_binprm * bprm)
 	 * trying to access the should-be-closed file descriptors of a process
 	 * undergoing exec(2).
 	 */
-	do_close_on_exec(current->files);
+	do_close_on_exec(me->files);
 	return 0;
 
 out: