| Message ID | 87pndm5y3l.fsf_-_@x220.int.ebiederm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Infrastructure to allow fixing exec deadlocks | expand |
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" <ebiederm@xmission.com> > --- > 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.
Bernd Edlinger <bernd.edlinger@hotmail.de> 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" <ebiederm@xmission.com> >> --- >> 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
On 3/9/20 6:34 PM, Eric W. Biederman wrote: > Bernd Edlinger <bernd.edlinger@hotmail.de> 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" <ebiederm@xmission.com> >>> --- >>> 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.
On 3/9/20 6:56 PM, Bernd Edlinger wrote: > On 3/9/20 6:34 PM, Eric W. Biederman wrote: >> Bernd Edlinger <bernd.edlinger@hotmail.de> 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" <ebiederm@xmission.com> >>>> --- >>>> 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 <bernd.edlinger@hotmail.de> Bernd.
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" <ebiederm@xmission.com> 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 <keescook@chromium.org> -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 >
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" <ebiederm@xmission.com> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
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:
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" <ebiederm@xmission.com> --- fs/exec.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)