Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1532422 > unrolled thread

[PATCH] printk.c: removed unnecessary code

Back to article view | Back to linux.kernel

Contents

  [PATCH] printk.c: removed unnecessary code Sebastian Duda <sebastian.duda@fau.de> - 2016-11-29 16:30 +0100
    Re: [PATCH] printk.c: removed unnecessary code Michal Hocko <mhocko@kernel.org> - 2016-11-30 10:20 +0100
      Re: [PATCH] printk.c: removed unnecessary code Petr Mladek <pmladek@suse.com> - 2016-11-30 11:30 +0100
    Re: [lkp] [printk.c]  ea0639c4d5: BUG:recent_printk_recursion Petr Mladek <pmladek@suse.com> - 2016-12-02 11:10 +0100

#1532422 — [PATCH] printk.c: removed unnecessary code

snprintf((char *) ?, 0, ...); always returns Zero and doesn't change the data.
Thus the execution of
	snprintf(NULL, 0, "[%5lu.000000] ", (unsigned long)ts);
has no effect on program.
The substitution with 0 increases the readability of the code.

Signed-off-by: Sebastian Duda <sebastian.duda@fau.de>
Signed-off-by: Tobias Baumeister <tobias.baumeister@fau.de>
---
 kernel/printk/printk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 5028f4f..fe3fec1 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1186,7 +1186,7 @@ static size_t print_time(u64 ts, char *buf)
 	rem_nsec = do_div(ts, 1000000000);
 
 	if (!buf)
-		return snprintf(NULL, 0, "[%5lu.000000] ", (unsigned long)ts);
+		return 0;
 
 	return sprintf(buf, "[%5lu.%06lu] ",
 		       (unsigned long)ts, rem_nsec / 1000);
-- 
2.7.4

[toc] | [next] | [standalone]

#1533096

[Resending with the full CC list as my email client has clobbered it in
the previous attempt for some reason]

On Tue 29-11-16 16:19:01, Sebastian Duda wrote:
> snprintf((char *) ?, 0, ...); always returns Zero and doesn't change the data.
> Thus the execution of
> 	snprintf(NULL, 0, "[%5lu.000000] ", (unsigned long)ts);
> has no effect on program.
> The substitution with 0 increases the readability of the code.

Are you sure this is correct. As per vsnprintf documentation:
"
 * The return value is the number of characters which would
 * be generated for the given input, excluding the trailing
 * '\0', as per ISO C99.
"

this should just work as 35dac27cedd1 ("printk: fix incorrect length
from print_time() when seconds > 99999") intended.

I haven't checked the implementation though so I might be wrong here.

> 
> Signed-off-by: Sebastian Duda <sebastian.duda@fau.de>
> Signed-off-by: Tobias Baumeister <tobias.baumeister@fau.de>
> ---
>  kernel/printk/printk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 5028f4f..fe3fec1 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1186,7 +1186,7 @@ static size_t print_time(u64 ts, char *buf)
>  	rem_nsec = do_div(ts, 1000000000);
>  
>  	if (!buf)
> -		return snprintf(NULL, 0, "[%5lu.000000] ", (unsigned long)ts);
> +		return 0;
>  
>  	return sprintf(buf, "[%5lu.%06lu] ",
>  		       (unsigned long)ts, rem_nsec / 1000);
> -- 
> 2.7.4

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1533193

On Wed 2016-11-30 10:14:28, Michal Hocko wrote:
> [Resending with the full CC list as my email client has clobbered it in
> the previous attempt for some reason]
> 
> On Tue 29-11-16 16:19:01, Sebastian Duda wrote:
> > snprintf((char *) ?, 0, ...); always returns Zero and doesn't change the data.
> > Thus the execution of
> > 	snprintf(NULL, 0, "[%5lu.000000] ", (unsigned long)ts);
> > has no effect on program.
> > The substitution with 0 increases the readability of the code.
> 
> Are you sure this is correct. As per vsnprintf documentation:
> "
>  * The return value is the number of characters which would
>  * be generated for the given input, excluding the trailing
>  * '\0', as per ISO C99.
> "
> 
> this should just work as 35dac27cedd1 ("printk: fix incorrect length
> from print_time() when seconds > 99999") intended.
> 
> I haven't checked the implementation though so I might be wrong here.

print_time() actually is not used by printk() directly. Therefore
the above reasoning does not apply.

They key here is to look when the buf might be NULL. It is when
we try to get an idea how much space will be needed for the message
when it is printed to some output device, e.g. console or syslog.
For example, there is the following chain of calls:

  syslog_print_all()
     msg_print_text(msg, prev, true, NULL, 0)
       print_prefix(msg, syslog, NULL)
         len += print_time(msg->ts_nsec, buf ? buf + len : NULL)

It means that we really need to know the length of the printed
string even when we do not write it to a buffer for the moment.

By other words, the patch would break the functionality.

> > 
> > Signed-off-by: Sebastian Duda <sebastian.duda@fau.de>
> > Signed-off-by: Tobias Baumeister <tobias.baumeister@fau.de>

Nacked-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1534784 — Re: [lkp] [printk.c] ea0639c4d5: BUG:recent_printk_recursion

On Fri 2016-12-02 01:12:32, kernel test robot wrote:
> 
> FYI, we noticed the following commit:
> 
> commit: ea0639c4d5c700ad63c0ace6a7a17877aac5b4c2 ("printk.c: removed unnecessary code")
> url: https://github.com/0day-ci/linux/commits/Sebastian-Duda/printk-c-removed-unnecessary-code/20161130-225006
> 
> 
> in testcase: trinity
> with following parameters:
> 
> 	runtime: 300s
> 
> test-description: Trinity is a linux system call fuzz tester.
> test-url: http://codemonkey.org.uk/projects/trinity/
> 
> 
> on test machine: qemu-system-i386 -enable-kvm -m 256M
> 
> caused below changes:
> 
> 
> +-------------------------------------------------------+------------+------------+
> |                                                       | ded6e842cf | ea0639c4d5 |
> +-------------------------------------------------------+------------+------------+
> | boot_successes                                        | 19         | 8          |
> | boot_failures                                         | 3          | 49         |

I am not surprised. The patch breaks counting of the space
needed to print the message. It is likely to cause buffer
overflows. I have already nacked it, see
https://lkml.kernel.org/r/20161130102231.GE24060@pathway.suse.cz

Anyway, thanks for testing.

Best Regards,
Petr

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web