linux-kernel - Re: [PATCH 01/13] kernel/irq/proc: use seq_put_decimal_ull

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMuHMdUGt-8onJqKbai9Uc+Vb-YHXKx_ggMqyURJp5btuf1v_A@mail.gmail.com>
Date: Wed, 20 Nov 2024 10:00:38 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: David Wang <00107082@....com>
Cc: tglx@...utronix.de, linux-kernel@...r.kernel.org, 
	linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH 01/13] kernel/irq/proc: use seq_put_decimal_ull_width()
 for decimal values

Hi David,

On Wed, Nov 20, 2024 at 3:08 AM David Wang <00107082@....com> wrote:
> At 2024-11-20 09:37:04, "David Wang" <00107082@....com> wrote:
> >At 2024-11-20 03:55:30, "Geert Uytterhoeven" <geert@...ux-m68k.org> wrote:
> >>On Sat, 9 Nov 2024, David Wang wrote:
> >>> seq_printf() is costy, on a system with m interrupts and n CPUs, there
> >>> would be m*n decimal values yield via seq_printf() when reading
> >>> /proc/interrupts, the cost parsing format strings grows with number of
> >>> CPU. Profiling on a x86 8-core system indicates seq_printf() takes ~47%
> >>> samples of show_interrupts(), and replace seq_printf() with
> >>> seq_put_decimal_ull_width() could have near 30% performance gain.
> >>>
> >>> The improvement has pratical significance, considering many monitoring
> >>> tools would read /proc/interrupts periodically.
> >>>
> >>> Signed-off-by: David Wang <00107082@....com>
> >>
> >>Thanks for your patch, which is now commit f9ed1f7c2e26fcd1
> >>("genirq/proc: Use seq_put_decimal_ull_width() for decimal values")
> >>in irqchip/irq/core.
> >>
> >>This removes a space after the last CPU column, causing the values in
> >>this column to be concatenated to the values in the next column.
> >>
> >>E.g. on Koelsch (R-Car M-W), the output changes from:
> >>
> >>             CPU0       CPU1
> >>      27:       1871       2017 GIC-0  27 Level     arch_timer
> >>      29:        646          0 GIC-0 205 Level     e60b0000.i2c
> >>      30:          0          0 GIC-0 174 Level     ffca0000.timer
> >>      31:          0          0 GIC-0  36 Level     e6050000.gpio
> >>      32:          0          0 GIC-0  37 Level     e6051000.gpio
> >>      [...]
> >>
> >>to
> >>
> >>             CPU0       CPU1
> >>      27:       1966       1900GIC-0  27 Level     arch_timer
> >>      29:        580          0GIC-0 205 Level     e60b0000.i2c
> >>      30:          0          0GIC-0 174 Level     ffca0000.timer
> >>      31:          0          0GIC-0  36 Level     e6050000.gpio
> >>      32:          0          0GIC-0  37 Level     e6051000.gpio
> >>      [...]
> >>
> >>making the output hard to read, and probably breaking scripts that parse
> >>its contents.
> >
> >Thanks for reporting this, I was considering the spaces and checked it on my system,
> >I thought "all" descriptions have leading spaces and it's ok to remove the extra one.
> >But I did not check all the "irq_print_chip" codes, now when
> >checking the code, there are many GPIO drivers' implementations with no leading spaces.
> >(The behavior is not consistent cross  driver implementations though...)
>
> Several drivers use dev_name as format string for seq_printf,  would this raise security concerns?
>
>        drivers/gpio/gpio-xgs-iproc.c:   seq_printf(p, dev_name(chip->dev));
>         drivers/gpio/gpio-mlxbf2.c:     seq_printf(p, dev_name(gs->dev));
>         drivers/gpio/gpio-omap.c:       seq_printf(p, dev_name(bank->dev));
>         drivers/gpio/gpio-hlwd.c:       seq_printf(p, dev_name(hlwd->dev));
>         drivers/gpio/gpio-aspeed.c:     seq_printf(p, dev_name(gpio->dev));
>         drivers/gpio/gpio-pca953x.c:    seq_printf(p, dev_name(gc->parent));
>         drivers/gpio/gpio-tegra186.c:   seq_printf(p, dev_name(gc->parent));
>         drivers/gpio/gpio-tegra.c:      seq_printf(s, dev_name(chip->parent));
>         drivers/gpio/gpio-ep93xx.c:     seq_printf(p, dev_name(gc->parent));
>         drivers/gpio/gpio-aspeed-sgpio.c:       seq_printf(p, dev_name(gpio->dev));
>         drivers/gpio/gpio-pl061.c:      seq_printf(p, dev_name(gc->parent));
>         drivers/gpio/gpio-visconti.c:   seq_printf(p, dev_name(priv->dev));

In theory, yes. But I guess it's hard to sneak a percent sign in these
device names.

But given the above, all of them should probably be updated to print
an initial space?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds