Re: [PATCH] do_fork(): Rename 'stack_size' argument to reflect actual use
On 05/03/15 22:29, David Rientjes wrote:
On Thu, 5 Mar 2015, Alex Dowad wrote:
diff --git a/kernel/fork.c b/kernel/fork.c
index cf65139..b38a2ae 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1186,10 +1186,12 @@ init_task_pid(struct task_struct *task, enum
pid_type type, struct pid *pid)
* It copies the registers, and all the appropriate
* parts of the process environment (as per the clone
* flags). The actual kick-off is left to the caller.
+ *
+ * When copying a kernel thread, 'stack_start' is the function to run.
*/
static struct task_struct *copy_process(unsigned long clone_flags,
unsigned long stack_start,
- unsigned long stack_size,
+ unsigned long kthread_arg,
int __user *child_tidptr,
struct pid *pid,
int trace)
@@ -1401,7 +1403,7 @@ static struct task_struct *copy_process(unsigned
long clone_flags,
retval = copy_io(clone_flags, p);
if (retval)
goto bad_fork_cleanup_namespaces;
- retval = copy_thread(clone_flags, stack_start, stack_size, p);
+ retval = copy_thread(clone_flags, stack_start, kthread_arg, p);
if (retval)
goto bad_fork_cleanup_io;
@@ -1629,8 +1631,8 @@ struct task_struct *fork_idle(int cpu)
* it and waits for it to finish using the VM if required.
*/
long do_fork(unsigned long clone_flags,
- unsigned long stack_start,
- unsigned long stack_size,
+ unsigned long stack_start, /* or function for kthread to run */
+ unsigned long kthread_arg,
int __user *parent_tidptr,
int __user *child_tidptr)
{
Looks fine, but I'm not sure about commenting functional formals. Since
copy_process() and do_fork() can have formals with different meanings,
then why not just rename them "arg1" and "arg2" respectively and then
define in the comment above the function what the possible combinations
are?
The second argument is *only* ever used for one thing: an argument passed to a
kernel thread. That's why I would like to rename it to "kthread_arg". The
previous argument (currently named "stack_start") is indeed used for 2
different things: a new stack pointer for a user thread, or a function to be
executed by a kernel thread. Rather than "arg1", what would you think of
something like "sp_or_fn", or "usp_or_fn"?
I would recommend exactly "arg" since it can be used for multiple purposes
and if the formal could ever be used for a third purpose we don't want to
go through another re-naming patch to change it from sp_or_fn or
usp_or_fn.
If that's done, then the comment above the function could define what arg
can represent.
Do others concur with this idea? Personally, I feel the code will be
more readable/maintainable if the naming of args/variables/etc reflects
what they are actually used for.
(Case in point: on IA64, copy_thread() adds the kernel thread arg to the
user stack pointer. The kernel thread arg is always 0 when forking a
user process, so this "works", but it's certainly not what the author
intended. Good names make it harder to write buggy code!)
For readability, using the same arg for 2 different purposes is a bad
practice (though it might be good for keeping the object code small). I
hate to think that "arg" might be co-opted for another purpose again.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] do_fork(): Rename 'stack_size' argument to reflect actual use
On Thu, 5 Mar 2015, Alex Dowad wrote:
> > > diff --git a/kernel/fork.c b/kernel/fork.c
> > > index cf65139..b38a2ae 100644
> > > --- a/kernel/fork.c
> > > +++ b/kernel/fork.c
> > > @@ -1186,10 +1186,12 @@ init_task_pid(struct task_struct *task, enum
> > > pid_type type, struct pid *pid)
> > >* It copies the registers, and all the appropriate
> > >* parts of the process environment (as per the clone
> > >* flags). The actual kick-off is left to the caller.
> > > + *
> > > + * When copying a kernel thread, 'stack_start' is the function to run.
> > >*/
> > > static struct task_struct *copy_process(unsigned long clone_flags,
> > > unsigned long stack_start,
> > > - unsigned long stack_size,
> > > + unsigned long kthread_arg,
> > > int __user *child_tidptr,
> > > struct pid *pid,
> > > int trace)
> > > @@ -1401,7 +1403,7 @@ static struct task_struct *copy_process(unsigned
> > > long clone_flags,
> > > retval = copy_io(clone_flags, p);
> > > if (retval)
> > > goto bad_fork_cleanup_namespaces;
> > > - retval = copy_thread(clone_flags, stack_start, stack_size, p);
> > > + retval = copy_thread(clone_flags, stack_start, kthread_arg, p);
> > > if (retval)
> > > goto bad_fork_cleanup_io;
> > > @@ -1629,8 +1631,8 @@ struct task_struct *fork_idle(int cpu)
> > >* it and waits for it to finish using the VM if required.
> > >*/
> > > long do_fork(unsigned long clone_flags,
> > > - unsigned long stack_start,
> > > - unsigned long stack_size,
> > > + unsigned long stack_start, /* or function for kthread to run */
> > > + unsigned long kthread_arg,
> > > int __user *parent_tidptr,
> > > int __user *child_tidptr)
> > > {
> > Looks fine, but I'm not sure about commenting functional formals. Since
> > copy_process() and do_fork() can have formals with different meanings,
> > then why not just rename them "arg1" and "arg2" respectively and then
> > define in the comment above the function what the possible combinations
> > are?
>
> The second argument is *only* ever used for one thing: an argument passed to a
> kernel thread. That's why I would like to rename it to "kthread_arg". The
> previous argument (currently named "stack_start") is indeed used for 2
> different things: a new stack pointer for a user thread, or a function to be
> executed by a kernel thread. Rather than "arg1", what would you think of
> something like "sp_or_fn", or "usp_or_fn"?
>
I would recommend exactly "arg" since it can be used for multiple purposes
and if the formal could ever be used for a third purpose we don't want to
go through another re-naming patch to change it from sp_or_fn or
usp_or_fn.
If that's done, then the comment above the function could define what arg
can represent.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] do_fork(): Rename 'stack_size' argument to reflect actual use
On 05/03/15 01:07, David Rientjes wrote:
On Wed, 4 Mar 2015, Alex Dowad wrote:
The 'stack_size' argument is never used to pass a stack size. It's only used
when
forking a kernel thread, in which case it is an argument which should be passed
to the 'main' function which the kernel thread executes. Hence, rename it to
'kthread_arg'.
Signed-off-by: Alex Dowad
---
Hi,
Please have a look at this patch. If this is accepted, I have a series of
patches
ready for a similar cleanup to all the arch-specific implementations of
copy_thread()
(as suggested by Andrew Morton in a private e-mail).
Thank you,
Alex Dowad
kernel/fork.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/kernel/fork.c b/kernel/fork.c
index cf65139..b38a2ae 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1186,10 +1186,12 @@ init_task_pid(struct task_struct *task, enum pid_type
type, struct pid *pid)
* It copies the registers, and all the appropriate
* parts of the process environment (as per the clone
* flags). The actual kick-off is left to the caller.
+ *
+ * When copying a kernel thread, 'stack_start' is the function to run.
*/
static struct task_struct *copy_process(unsigned long clone_flags,
unsigned long stack_start,
- unsigned long stack_size,
+ unsigned long kthread_arg,
int __user *child_tidptr,
struct pid *pid,
int trace)
@@ -1401,7 +1403,7 @@ static struct task_struct *copy_process(unsigned long
clone_flags,
retval = copy_io(clone_flags, p);
if (retval)
goto bad_fork_cleanup_namespaces;
- retval = copy_thread(clone_flags, stack_start, stack_size, p);
+ retval = copy_thread(clone_flags, stack_start, kthread_arg, p);
if (retval)
goto bad_fork_cleanup_io;
@@ -1629,8 +1631,8 @@ struct task_struct *fork_idle(int cpu)
* it and waits for it to finish using the VM if required.
*/
long do_fork(unsigned long clone_flags,
- unsigned long stack_start,
- unsigned long stack_size,
+ unsigned long stack_start, /* or function for kthread to run */
+ unsigned long kthread_arg,
int __user *parent_tidptr,
int __user *child_tidptr)
{
Looks fine, but I'm not sure about commenting functional formals. Since
copy_process() and do_fork() can have formals with different meanings,
then why not just rename them "arg1" and "arg2" respectively and then
define in the comment above the function what the possible combinations
are?
The second argument is *only* ever used for one thing: an argument
passed to a kernel thread. That's why I would like to rename it to
"kthread_arg". The previous argument (currently named "stack_start") is
indeed used for 2 different things: a new stack pointer for a user
thread, or a function to be executed by a kernel thread. Rather than
"arg1", what would you think of something like "sp_or_fn", or "usp_or_fn"?
Thanks for your feedback!
@@ -1656,7 +1658,7 @@ long do_fork(unsigned long clone_flags,
trace = 0;
}
- p = copy_process(clone_flags, stack_start, stack_size,
+ p = copy_process(clone_flags, stack_start, kthread_arg,
child_tidptr, NULL, trace);
/*
* Do this prior waking up the new thread - the thread pointer
@@ -1740,7 +1742,7 @@ SYSCALL_DEFINE5(clone, unsigned long, newsp, unsigned
long, clone_flags,
int, tls_val)
#elif defined(CONFIG_CLONE_BACKWARDS3)
SYSCALL_DEFINE6(clone, unsigned long, clone_flags, unsigned long, newsp,
- int, stack_size,
+ int, ignored,
int __user *, parent_tidptr,
int __user *, child_tidptr,
int, tls_val)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] do_fork(): Rename 'stack_size' argument to reflect actual use
On Thu, 5 Mar 2015, Alex Dowad wrote:
diff --git a/kernel/fork.c b/kernel/fork.c
index cf65139..b38a2ae 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1186,10 +1186,12 @@ init_task_pid(struct task_struct *task, enum
pid_type type, struct pid *pid)
* It copies the registers, and all the appropriate
* parts of the process environment (as per the clone
* flags). The actual kick-off is left to the caller.
+ *
+ * When copying a kernel thread, 'stack_start' is the function to run.
*/
static struct task_struct *copy_process(unsigned long clone_flags,
unsigned long stack_start,
- unsigned long stack_size,
+ unsigned long kthread_arg,
int __user *child_tidptr,
struct pid *pid,
int trace)
@@ -1401,7 +1403,7 @@ static struct task_struct *copy_process(unsigned
long clone_flags,
retval = copy_io(clone_flags, p);
if (retval)
goto bad_fork_cleanup_namespaces;
- retval = copy_thread(clone_flags, stack_start, stack_size, p);
+ retval = copy_thread(clone_flags, stack_start, kthread_arg, p);
if (retval)
goto bad_fork_cleanup_io;
@@ -1629,8 +1631,8 @@ struct task_struct *fork_idle(int cpu)
* it and waits for it to finish using the VM if required.
*/
long do_fork(unsigned long clone_flags,
- unsigned long stack_start,
- unsigned long stack_size,
+ unsigned long stack_start, /* or function for kthread to run */
+ unsigned long kthread_arg,
int __user *parent_tidptr,
int __user *child_tidptr)
{
Looks fine, but I'm not sure about commenting functional formals. Since
copy_process() and do_fork() can have formals with different meanings,
then why not just rename them arg1 and arg2 respectively and then
define in the comment above the function what the possible combinations
are?
The second argument is *only* ever used for one thing: an argument passed to a
kernel thread. That's why I would like to rename it to kthread_arg. The
previous argument (currently named stack_start) is indeed used for 2
different things: a new stack pointer for a user thread, or a function to be
executed by a kernel thread. Rather than arg1, what would you think of
something like sp_or_fn, or usp_or_fn?
I would recommend exactly arg since it can be used for multiple purposes
and if the formal could ever be used for a third purpose we don't want to
go through another re-naming patch to change it from sp_or_fn or
usp_or_fn.
If that's done, then the comment above the function could define what arg
can represent.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] do_fork(): Rename 'stack_size' argument to reflect actual use
On 05/03/15 01:07, David Rientjes wrote:
On Wed, 4 Mar 2015, Alex Dowad wrote:
The 'stack_size' argument is never used to pass a stack size. It's only used
when
forking a kernel thread, in which case it is an argument which should be passed
to the 'main' function which the kernel thread executes. Hence, rename it to
'kthread_arg'.
Signed-off-by: Alex Dowad alexinbeij...@gmail.com
---
Hi,
Please have a look at this patch. If this is accepted, I have a series of
patches
ready for a similar cleanup to all the arch-specific implementations of
copy_thread()
(as suggested by Andrew Morton in a private e-mail).
Thank you,
Alex Dowad
kernel/fork.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/kernel/fork.c b/kernel/fork.c
index cf65139..b38a2ae 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1186,10 +1186,12 @@ init_task_pid(struct task_struct *task, enum pid_type
type, struct pid *pid)
* It copies the registers, and all the appropriate
* parts of the process environment (as per the clone
* flags). The actual kick-off is left to the caller.
+ *
+ * When copying a kernel thread, 'stack_start' is the function to run.
*/
static struct task_struct *copy_process(unsigned long clone_flags,
unsigned long stack_start,
- unsigned long stack_size,
+ unsigned long kthread_arg,
int __user *child_tidptr,
struct pid *pid,
int trace)
@@ -1401,7 +1403,7 @@ static struct task_struct *copy_process(unsigned long
clone_flags,
retval = copy_io(clone_flags, p);
if (retval)
goto bad_fork_cleanup_namespaces;
- retval = copy_thread(clone_flags, stack_start, stack_size, p);
+ retval = copy_thread(clone_flags, stack_start, kthread_arg, p);
if (retval)
goto bad_fork_cleanup_io;
@@ -1629,8 +1631,8 @@ struct task_struct *fork_idle(int cpu)
* it and waits for it to finish using the VM if required.
*/
long do_fork(unsigned long clone_flags,
- unsigned long stack_start,
- unsigned long stack_size,
+ unsigned long stack_start, /* or function for kthread to run */
+ unsigned long kthread_arg,
int __user *parent_tidptr,
int __user *child_tidptr)
{
Looks fine, but I'm not sure about commenting functional formals. Since
copy_process() and do_fork() can have formals with different meanings,
then why not just rename them arg1 and arg2 respectively and then
define in the comment above the function what the possible combinations
are?
The second argument is *only* ever used for one thing: an argument
passed to a kernel thread. That's why I would like to rename it to
kthread_arg. The previous argument (currently named stack_start) is
indeed used for 2 different things: a new stack pointer for a user
thread, or a function to be executed by a kernel thread. Rather than
arg1, what would you think of something like sp_or_fn, or usp_or_fn?
Thanks for your feedback!
@@ -1656,7 +1658,7 @@ long do_fork(unsigned long clone_flags,
trace = 0;
}
- p = copy_process(clone_flags, stack_start, stack_size,
+ p = copy_process(clone_flags, stack_start, kthread_arg,
child_tidptr, NULL, trace);
/*
* Do this prior waking up the new thread - the thread pointer
@@ -1740,7 +1742,7 @@ SYSCALL_DEFINE5(clone, unsigned long, newsp, unsigned
long, clone_flags,
int, tls_val)
#elif defined(CONFIG_CLONE_BACKWARDS3)
SYSCALL_DEFINE6(clone, unsigned long, clone_flags, unsigned long, newsp,
- int, stack_size,
+ int, ignored,
int __user *, parent_tidptr,
int __user *, child_tidptr,
int, tls_val)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] do_fork(): Rename 'stack_size' argument to reflect actual use
On 05/03/15 22:29, David Rientjes wrote:
On Thu, 5 Mar 2015, Alex Dowad wrote:
diff --git a/kernel/fork.c b/kernel/fork.c
index cf65139..b38a2ae 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1186,10 +1186,12 @@ init_task_pid(struct task_struct *task, enum
pid_type type, struct pid *pid)
* It copies the registers, and all the appropriate
* parts of the process environment (as per the clone
* flags). The actual kick-off is left to the caller.
+ *
+ * When copying a kernel thread, 'stack_start' is the function to run.
*/
static struct task_struct *copy_process(unsigned long clone_flags,
unsigned long stack_start,
- unsigned long stack_size,
+ unsigned long kthread_arg,
int __user *child_tidptr,
struct pid *pid,
int trace)
@@ -1401,7 +1403,7 @@ static struct task_struct *copy_process(unsigned
long clone_flags,
retval = copy_io(clone_flags, p);
if (retval)
goto bad_fork_cleanup_namespaces;
- retval = copy_thread(clone_flags, stack_start, stack_size, p);
+ retval = copy_thread(clone_flags, stack_start, kthread_arg, p);
if (retval)
goto bad_fork_cleanup_io;
@@ -1629,8 +1631,8 @@ struct task_struct *fork_idle(int cpu)
* it and waits for it to finish using the VM if required.
*/
long do_fork(unsigned long clone_flags,
- unsigned long stack_start,
- unsigned long stack_size,
+ unsigned long stack_start, /* or function for kthread to run */
+ unsigned long kthread_arg,
int __user *parent_tidptr,
int __user *child_tidptr)
{
Looks fine, but I'm not sure about commenting functional formals. Since
copy_process() and do_fork() can have formals with different meanings,
then why not just rename them arg1 and arg2 respectively and then
define in the comment above the function what the possible combinations
are?
The second argument is *only* ever used for one thing: an argument passed to a
kernel thread. That's why I would like to rename it to kthread_arg. The
previous argument (currently named stack_start) is indeed used for 2
different things: a new stack pointer for a user thread, or a function to be
executed by a kernel thread. Rather than arg1, what would you think of
something like sp_or_fn, or usp_or_fn?
I would recommend exactly arg since it can be used for multiple purposes
and if the formal could ever be used for a third purpose we don't want to
go through another re-naming patch to change it from sp_or_fn or
usp_or_fn.
If that's done, then the comment above the function could define what arg
can represent.
Do others concur with this idea? Personally, I feel the code will be
more readable/maintainable if the naming of args/variables/etc reflects
what they are actually used for.
(Case in point: on IA64, copy_thread() adds the kernel thread arg to the
user stack pointer. The kernel thread arg is always 0 when forking a
user process, so this works, but it's certainly not what the author
intended. Good names make it harder to write buggy code!)
For readability, using the same arg for 2 different purposes is a bad
practice (though it might be good for keeping the object code small). I
hate to think that arg might be co-opted for another purpose again.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] do_fork(): Rename 'stack_size' argument to reflect actual use
On Wed, 4 Mar 2015, Alex Dowad wrote:
> The 'stack_size' argument is never used to pass a stack size. It's only used
> when
> forking a kernel thread, in which case it is an argument which should be
> passed
> to the 'main' function which the kernel thread executes. Hence, rename it to
> 'kthread_arg'.
>
> Signed-off-by: Alex Dowad
> ---
>
> Hi,
>
> Please have a look at this patch. If this is accepted, I have a series of
> patches
> ready for a similar cleanup to all the arch-specific implementations of
> copy_thread()
> (as suggested by Andrew Morton in a private e-mail).
>
> Thank you,
> Alex Dowad
>
> kernel/fork.c | 14 --
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index cf65139..b38a2ae 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1186,10 +1186,12 @@ init_task_pid(struct task_struct *task, enum pid_type
> type, struct pid *pid)
> * It copies the registers, and all the appropriate
> * parts of the process environment (as per the clone
> * flags). The actual kick-off is left to the caller.
> + *
> + * When copying a kernel thread, 'stack_start' is the function to run.
> */
> static struct task_struct *copy_process(unsigned long clone_flags,
> unsigned long stack_start,
> - unsigned long stack_size,
> + unsigned long kthread_arg,
> int __user *child_tidptr,
> struct pid *pid,
> int trace)
> @@ -1401,7 +1403,7 @@ static struct task_struct *copy_process(unsigned long
> clone_flags,
> retval = copy_io(clone_flags, p);
> if (retval)
> goto bad_fork_cleanup_namespaces;
> - retval = copy_thread(clone_flags, stack_start, stack_size, p);
> + retval = copy_thread(clone_flags, stack_start, kthread_arg, p);
> if (retval)
> goto bad_fork_cleanup_io;
>
> @@ -1629,8 +1631,8 @@ struct task_struct *fork_idle(int cpu)
> * it and waits for it to finish using the VM if required.
> */
> long do_fork(unsigned long clone_flags,
> - unsigned long stack_start,
> - unsigned long stack_size,
> + unsigned long stack_start, /* or function for kthread to run */
> + unsigned long kthread_arg,
> int __user *parent_tidptr,
> int __user *child_tidptr)
> {
Looks fine, but I'm not sure about commenting functional formals. Since
copy_process() and do_fork() can have formals with different meanings,
then why not just rename them "arg1" and "arg2" respectively and then
define in the comment above the function what the possible combinations
are?
> @@ -1656,7 +1658,7 @@ long do_fork(unsigned long clone_flags,
> trace = 0;
> }
>
> - p = copy_process(clone_flags, stack_start, stack_size,
> + p = copy_process(clone_flags, stack_start, kthread_arg,
>child_tidptr, NULL, trace);
> /*
>* Do this prior waking up the new thread - the thread pointer
> @@ -1740,7 +1742,7 @@ SYSCALL_DEFINE5(clone, unsigned long, newsp, unsigned
> long, clone_flags,
>int, tls_val)
> #elif defined(CONFIG_CLONE_BACKWARDS3)
> SYSCALL_DEFINE6(clone, unsigned long, clone_flags, unsigned long, newsp,
> - int, stack_size,
> + int, ignored,
> int __user *, parent_tidptr,
> int __user *, child_tidptr,
> int, tls_val)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] do_fork(): Rename 'stack_size' argument to reflect actual use
On Wed, Mar 4, 2015 at 4:40 AM, Alex Dowad wrote:
> The 'stack_size' argument is never used to pass a stack size. It's only used
> when
> forking a kernel thread, in which case it is an argument which should be
> passed
> to the 'main' function which the kernel thread executes. Hence, rename it to
> 'kthread_arg'.
>
> Signed-off-by: Alex Dowad
This looks correct to me. Thanks for cleaning it up!
Reviewed-by: Kees Cook
-Kees
> ---
>
> Hi,
>
> Please have a look at this patch. If this is accepted, I have a series of
> patches
> ready for a similar cleanup to all the arch-specific implementations of
> copy_thread()
> (as suggested by Andrew Morton in a private e-mail).
>
> Thank you,
> Alex Dowad
>
> kernel/fork.c | 14 --
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index cf65139..b38a2ae 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1186,10 +1186,12 @@ init_task_pid(struct task_struct *task, enum pid_type
> type, struct pid *pid)
> * It copies the registers, and all the appropriate
> * parts of the process environment (as per the clone
> * flags). The actual kick-off is left to the caller.
> + *
> + * When copying a kernel thread, 'stack_start' is the function to run.
> */
> static struct task_struct *copy_process(unsigned long clone_flags,
> unsigned long stack_start,
> - unsigned long stack_size,
> + unsigned long kthread_arg,
> int __user *child_tidptr,
> struct pid *pid,
> int trace)
> @@ -1401,7 +1403,7 @@ static struct task_struct *copy_process(unsigned long
> clone_flags,
> retval = copy_io(clone_flags, p);
> if (retval)
> goto bad_fork_cleanup_namespaces;
> - retval = copy_thread(clone_flags, stack_start, stack_size, p);
> + retval = copy_thread(clone_flags, stack_start, kthread_arg, p);
> if (retval)
> goto bad_fork_cleanup_io;
>
> @@ -1629,8 +1631,8 @@ struct task_struct *fork_idle(int cpu)
> * it and waits for it to finish using the VM if required.
> */
> long do_fork(unsigned long clone_flags,
> - unsigned long stack_start,
> - unsigned long stack_size,
> + unsigned long stack_start, /* or function for kthread to run */
> + unsigned long kthread_arg,
> int __user *parent_tidptr,
> int __user *child_tidptr)
> {
> @@ -1656,7 +1658,7 @@ long do_fork(unsigned long clone_flags,
> trace = 0;
> }
>
> - p = copy_process(clone_flags, stack_start, stack_size,
> + p = copy_process(clone_flags, stack_start, kthread_arg,
> child_tidptr, NULL, trace);
> /*
> * Do this prior waking up the new thread - the thread pointer
> @@ -1740,7 +1742,7 @@ SYSCALL_DEFINE5(clone, unsigned long, newsp, unsigned
> long, clone_flags,
> int, tls_val)
> #elif defined(CONFIG_CLONE_BACKWARDS3)
> SYSCALL_DEFINE6(clone, unsigned long, clone_flags, unsigned long, newsp,
> - int, stack_size,
> + int, ignored,
> int __user *, parent_tidptr,
> int __user *, child_tidptr,
> int, tls_val)
> --
> 2.0.0.GIT
>
--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] do_fork(): Rename 'stack_size' argument to reflect actual use
The 'stack_size' argument is never used to pass a stack size. It's only used
when
forking a kernel thread, in which case it is an argument which should be passed
to the 'main' function which the kernel thread executes. Hence, rename it to
'kthread_arg'.
Signed-off-by: Alex Dowad
---
Hi,
Please have a look at this patch. If this is accepted, I have a series of
patches
ready for a similar cleanup to all the arch-specific implementations of
copy_thread()
(as suggested by Andrew Morton in a private e-mail).
Thank you,
Alex Dowad
kernel/fork.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/kernel/fork.c b/kernel/fork.c
index cf65139..b38a2ae 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1186,10 +1186,12 @@ init_task_pid(struct task_struct *task, enum pid_type
type, struct pid *pid)
* It copies the registers, and all the appropriate
* parts of the process environment (as per the clone
* flags). The actual kick-off is left to the caller.
+ *
+ * When copying a kernel thread, 'stack_start' is the function to run.
*/
static struct task_struct *copy_process(unsigned long clone_flags,
unsigned long stack_start,
- unsigned long stack_size,
+ unsigned long kthread_arg,
int __user *child_tidptr,
struct pid *pid,
int trace)
@@ -1401,7 +1403,7 @@ static struct task_struct *copy_process(unsigned long
clone_flags,
retval = copy_io(clone_flags, p);
if (retval)
goto bad_fork_cleanup_namespaces;
- retval = copy_thread(clone_flags, stack_start, stack_size, p);
+ retval = copy_thread(clone_flags, stack_start, kthread_arg, p);
if (retval)
goto bad_fork_cleanup_io;
@@ -1629,8 +1631,8 @@ struct task_struct *fork_idle(int cpu)
* it and waits for it to finish using the VM if required.
*/
long do_fork(unsigned long clone_flags,
- unsigned long stack_start,
- unsigned long stack_size,
+ unsigned long stack_start, /* or function for kthread to run */
+ unsigned long kthread_arg,
int __user *parent_tidptr,
int __user *child_tidptr)
{
@@ -1656,7 +1658,7 @@ long do_fork(unsigned long clone_flags,
trace = 0;
}
- p = copy_process(clone_flags, stack_start, stack_size,
+ p = copy_process(clone_flags, stack_start, kthread_arg,
child_tidptr, NULL, trace);
/*
* Do this prior waking up the new thread - the thread pointer
@@ -1740,7 +1742,7 @@ SYSCALL_DEFINE5(clone, unsigned long, newsp, unsigned
long, clone_flags,
int, tls_val)
#elif defined(CONFIG_CLONE_BACKWARDS3)
SYSCALL_DEFINE6(clone, unsigned long, clone_flags, unsigned long, newsp,
- int, stack_size,
+ int, ignored,
int __user *, parent_tidptr,
int __user *, child_tidptr,
int, tls_val)
--
2.0.0.GIT
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] do_fork(): Rename 'stack_size' argument to reflect actual use
On Wed, Mar 4, 2015 at 4:40 AM, Alex Dowad alexinbeij...@gmail.com wrote:
The 'stack_size' argument is never used to pass a stack size. It's only used
when
forking a kernel thread, in which case it is an argument which should be
passed
to the 'main' function which the kernel thread executes. Hence, rename it to
'kthread_arg'.
Signed-off-by: Alex Dowad alexinbeij...@gmail.com
This looks correct to me. Thanks for cleaning it up!
Reviewed-by: Kees Cook keesc...@chromium.org
-Kees
---
Hi,
Please have a look at this patch. If this is accepted, I have a series of
patches
ready for a similar cleanup to all the arch-specific implementations of
copy_thread()
(as suggested by Andrew Morton in a private e-mail).
Thank you,
Alex Dowad
kernel/fork.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/kernel/fork.c b/kernel/fork.c
index cf65139..b38a2ae 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1186,10 +1186,12 @@ init_task_pid(struct task_struct *task, enum pid_type
type, struct pid *pid)
* It copies the registers, and all the appropriate
* parts of the process environment (as per the clone
* flags). The actual kick-off is left to the caller.
+ *
+ * When copying a kernel thread, 'stack_start' is the function to run.
*/
static struct task_struct *copy_process(unsigned long clone_flags,
unsigned long stack_start,
- unsigned long stack_size,
+ unsigned long kthread_arg,
int __user *child_tidptr,
struct pid *pid,
int trace)
@@ -1401,7 +1403,7 @@ static struct task_struct *copy_process(unsigned long
clone_flags,
retval = copy_io(clone_flags, p);
if (retval)
goto bad_fork_cleanup_namespaces;
- retval = copy_thread(clone_flags, stack_start, stack_size, p);
+ retval = copy_thread(clone_flags, stack_start, kthread_arg, p);
if (retval)
goto bad_fork_cleanup_io;
@@ -1629,8 +1631,8 @@ struct task_struct *fork_idle(int cpu)
* it and waits for it to finish using the VM if required.
*/
long do_fork(unsigned long clone_flags,
- unsigned long stack_start,
- unsigned long stack_size,
+ unsigned long stack_start, /* or function for kthread to run */
+ unsigned long kthread_arg,
int __user *parent_tidptr,
int __user *child_tidptr)
{
@@ -1656,7 +1658,7 @@ long do_fork(unsigned long clone_flags,
trace = 0;
}
- p = copy_process(clone_flags, stack_start, stack_size,
+ p = copy_process(clone_flags, stack_start, kthread_arg,
child_tidptr, NULL, trace);
/*
* Do this prior waking up the new thread - the thread pointer
@@ -1740,7 +1742,7 @@ SYSCALL_DEFINE5(clone, unsigned long, newsp, unsigned
long, clone_flags,
int, tls_val)
#elif defined(CONFIG_CLONE_BACKWARDS3)
SYSCALL_DEFINE6(clone, unsigned long, clone_flags, unsigned long, newsp,
- int, stack_size,
+ int, ignored,
int __user *, parent_tidptr,
int __user *, child_tidptr,
int, tls_val)
--
2.0.0.GIT
--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] do_fork(): Rename 'stack_size' argument to reflect actual use
On Wed, 4 Mar 2015, Alex Dowad wrote:
The 'stack_size' argument is never used to pass a stack size. It's only used
when
forking a kernel thread, in which case it is an argument which should be
passed
to the 'main' function which the kernel thread executes. Hence, rename it to
'kthread_arg'.
Signed-off-by: Alex Dowad alexinbeij...@gmail.com
---
Hi,
Please have a look at this patch. If this is accepted, I have a series of
patches
ready for a similar cleanup to all the arch-specific implementations of
copy_thread()
(as suggested by Andrew Morton in a private e-mail).
Thank you,
Alex Dowad
kernel/fork.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/kernel/fork.c b/kernel/fork.c
index cf65139..b38a2ae 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1186,10 +1186,12 @@ init_task_pid(struct task_struct *task, enum pid_type
type, struct pid *pid)
* It copies the registers, and all the appropriate
* parts of the process environment (as per the clone
* flags). The actual kick-off is left to the caller.
+ *
+ * When copying a kernel thread, 'stack_start' is the function to run.
*/
static struct task_struct *copy_process(unsigned long clone_flags,
unsigned long stack_start,
- unsigned long stack_size,
+ unsigned long kthread_arg,
int __user *child_tidptr,
struct pid *pid,
int trace)
@@ -1401,7 +1403,7 @@ static struct task_struct *copy_process(unsigned long
clone_flags,
retval = copy_io(clone_flags, p);
if (retval)
goto bad_fork_cleanup_namespaces;
- retval = copy_thread(clone_flags, stack_start, stack_size, p);
+ retval = copy_thread(clone_flags, stack_start, kthread_arg, p);
if (retval)
goto bad_fork_cleanup_io;
@@ -1629,8 +1631,8 @@ struct task_struct *fork_idle(int cpu)
* it and waits for it to finish using the VM if required.
*/
long do_fork(unsigned long clone_flags,
- unsigned long stack_start,
- unsigned long stack_size,
+ unsigned long stack_start, /* or function for kthread to run */
+ unsigned long kthread_arg,
int __user *parent_tidptr,
int __user *child_tidptr)
{
Looks fine, but I'm not sure about commenting functional formals. Since
copy_process() and do_fork() can have formals with different meanings,
then why not just rename them arg1 and arg2 respectively and then
define in the comment above the function what the possible combinations
are?
@@ -1656,7 +1658,7 @@ long do_fork(unsigned long clone_flags,
trace = 0;
}
- p = copy_process(clone_flags, stack_start, stack_size,
+ p = copy_process(clone_flags, stack_start, kthread_arg,
child_tidptr, NULL, trace);
/*
* Do this prior waking up the new thread - the thread pointer
@@ -1740,7 +1742,7 @@ SYSCALL_DEFINE5(clone, unsigned long, newsp, unsigned
long, clone_flags,
int, tls_val)
#elif defined(CONFIG_CLONE_BACKWARDS3)
SYSCALL_DEFINE6(clone, unsigned long, clone_flags, unsigned long, newsp,
- int, stack_size,
+ int, ignored,
int __user *, parent_tidptr,
int __user *, child_tidptr,
int, tls_val)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] do_fork(): Rename 'stack_size' argument to reflect actual use
The 'stack_size' argument is never used to pass a stack size. It's only used
when
forking a kernel thread, in which case it is an argument which should be passed
to the 'main' function which the kernel thread executes. Hence, rename it to
'kthread_arg'.
Signed-off-by: Alex Dowad alexinbeij...@gmail.com
---
Hi,
Please have a look at this patch. If this is accepted, I have a series of
patches
ready for a similar cleanup to all the arch-specific implementations of
copy_thread()
(as suggested by Andrew Morton in a private e-mail).
Thank you,
Alex Dowad
kernel/fork.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/kernel/fork.c b/kernel/fork.c
index cf65139..b38a2ae 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1186,10 +1186,12 @@ init_task_pid(struct task_struct *task, enum pid_type
type, struct pid *pid)
* It copies the registers, and all the appropriate
* parts of the process environment (as per the clone
* flags). The actual kick-off is left to the caller.
+ *
+ * When copying a kernel thread, 'stack_start' is the function to run.
*/
static struct task_struct *copy_process(unsigned long clone_flags,
unsigned long stack_start,
- unsigned long stack_size,
+ unsigned long kthread_arg,
int __user *child_tidptr,
struct pid *pid,
int trace)
@@ -1401,7 +1403,7 @@ static struct task_struct *copy_process(unsigned long
clone_flags,
retval = copy_io(clone_flags, p);
if (retval)
goto bad_fork_cleanup_namespaces;
- retval = copy_thread(clone_flags, stack_start, stack_size, p);
+ retval = copy_thread(clone_flags, stack_start, kthread_arg, p);
if (retval)
goto bad_fork_cleanup_io;
@@ -1629,8 +1631,8 @@ struct task_struct *fork_idle(int cpu)
* it and waits for it to finish using the VM if required.
*/
long do_fork(unsigned long clone_flags,
- unsigned long stack_start,
- unsigned long stack_size,
+ unsigned long stack_start, /* or function for kthread to run */
+ unsigned long kthread_arg,
int __user *parent_tidptr,
int __user *child_tidptr)
{
@@ -1656,7 +1658,7 @@ long do_fork(unsigned long clone_flags,
trace = 0;
}
- p = copy_process(clone_flags, stack_start, stack_size,
+ p = copy_process(clone_flags, stack_start, kthread_arg,
child_tidptr, NULL, trace);
/*
* Do this prior waking up the new thread - the thread pointer
@@ -1740,7 +1742,7 @@ SYSCALL_DEFINE5(clone, unsigned long, newsp, unsigned
long, clone_flags,
int, tls_val)
#elif defined(CONFIG_CLONE_BACKWARDS3)
SYSCALL_DEFINE6(clone, unsigned long, clone_flags, unsigned long, newsp,
- int, stack_size,
+ int, ignored,
int __user *, parent_tidptr,
int __user *, child_tidptr,
int, tls_val)
--
2.0.0.GIT
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
