2019-04-30 09:12:24

by Petr Mladek

Subject: [PATCH v2 2/2] livepatch: Use static buffer for debugging messages under rq lock

klp_try_switch_task() is called under klp_mutex. The buffer for
debugging messages might be static.

Signed-off-by: Petr Mladek <[email protected]>
---
kernel/livepatch/transition.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 8e0274075e75..d66086fd6d75 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -304,11 +304,11 @@ static int klp_check_stack(struct task_struct *task, char *err_buf)
*/
static bool klp_try_switch_task(struct task_struct *task)
{
+ static char err_buf[STACK_ERR_BUF_SIZE];
struct rq *rq;
struct rq_flags flags;
int ret;
bool success = false;
- char err_buf[STACK_ERR_BUF_SIZE];

err_buf[0] = '\0';

@@ -351,7 +351,6 @@ static bool klp_try_switch_task(struct task_struct *task)
pr_debug("%s", err_buf);

return success;
-
}

/*
--
2.16.4

2019-04-30 11:33:57

by Miroslav Benes

Subject: Re: [PATCH v2 2/2] livepatch: Use static buffer for debugging messages under rq lock

On Tue, 30 Apr 2019, Petr Mladek wrote:

> klp_try_switch_task() is called under klp_mutex. The buffer for
> debugging messages might be static.
>
> Signed-off-by: Petr Mladek <[email protected]>

Acked-by: Miroslav Benes <[email protected]>

Miroslav

2019-04-30 12:33:09

by Kamalesh Babulal

Subject: Re: [PATCH v2 2/2] livepatch: Use static buffer for debugging messages under rq lock

On Tue, Apr 30, 2019 at 11:10:49AM +0200, Petr Mladek wrote:
> klp_try_switch_task() is called under klp_mutex. The buffer for
> debugging messages might be static.
>
> Signed-off-by: Petr Mladek <[email protected]>

Reviewed-by: Kamalesh Babulal <[email protected]>

2019-05-07 00:44:31

by Josh Poimboeuf

Subject: Re: [PATCH v2 2/2] livepatch: Use static buffer for debugging messages under rq lock

On Tue, Apr 30, 2019 at 11:10:49AM +0200, Petr Mladek wrote:
> klp_try_switch_task() is called under klp_mutex. The buffer for
> debugging messages might be static.

The patch description is missing a "why" (presumably to reduce stack
usage).

>
> Signed-off-by: Petr Mladek <[email protected]>
> ---
> kernel/livepatch/transition.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index 8e0274075e75..d66086fd6d75 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -304,11 +304,11 @@ static int klp_check_stack(struct task_struct *task, char *err_buf)
> */
> static bool klp_try_switch_task(struct task_struct *task)
> {
> + static char err_buf[STACK_ERR_BUF_SIZE];
> struct rq *rq;
> struct rq_flags flags;
> int ret;
> bool success = false;
> - char err_buf[STACK_ERR_BUF_SIZE];
>
> err_buf[0] = '\0';
>
> @@ -351,7 +351,6 @@ static bool klp_try_switch_task(struct task_struct *task)
> pr_debug("%s", err_buf);
>
> return success;
> -
> }
>
> /*
> --
> 2.16.4
>

--
Josh

2019-05-07 11:52:55

by Petr Mladek

Subject: Re: [PATCH v2 2/2] livepatch: Use static buffer for debugging messages under rq lock

On Mon 2019-05-06 19:43:19, Josh Poimboeuf wrote:
> On Tue, Apr 30, 2019 at 11:10:49AM +0200, Petr Mladek wrote:
> > klp_try_switch_task() is called under klp_mutex. The buffer for
> > debugging messages might be static.
>
> The patch description is missing a "why" (presumably to reduce stack
> usage).

Exactly. I thought that it was obvious. But I am infected by printk
code where line buffers are 1k and nobody wants them on the stack.

128bytes in klp_try_switch_task() context are acceptable but
it is still rather big buffer.

OK, what about the following commit message?

"klp_try_switch_task() is called under klp_mutex. The buffer for
debugging messages might be static to reduce stack usage."

Best Regards,
Petr

2019-05-07 13:21:49

by Josh Poimboeuf

Subject: Re: [PATCH v2 2/2] livepatch: Use static buffer for debugging messages under rq lock

On Tue, May 07, 2019 at 01:50:29PM +0200, Petr Mladek wrote:
> On Mon 2019-05-06 19:43:19, Josh Poimboeuf wrote:
> > On Tue, Apr 30, 2019 at 11:10:49AM +0200, Petr Mladek wrote:
> > > klp_try_switch_task() is called under klp_mutex. The buffer for
> > > debugging messages might be static.
> >
> > The patch description is missing a "why" (presumably to reduce stack
> > usage).
>
> Exactly. I thought that it was obvious. But I am infected by printk
> code where line buffers are 1k and nobody wants them on the stack.
>
> 128bytes in klp_try_switch_task() context are acceptable but
> it is still rather big buffer.
>
> OK, what about the following commit message?
>
> "klp_try_switch_task() is called under klp_mutex. The buffer for
> debugging messages might be static to reduce stack usage."

It's better to use imperative language. It would also be good to
reverse the order of the wording by starting with the problem.

Something like:

The err_buf array uses 128 bytes of stack space. Move it off the stack
by making it static. It's safe to use a shared buffer because
klp_try_switch_task() is called under klp_mutex.

--
Josh