| 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
| ||
|
Message-ID: <m2wme1pm4k.fsf@posteo.net>
Date: Fri, 07 Feb 2025 19:53:31 +0000
From: Charalampos Mitrodimas <charmitro@...teo.net>
To: Alban Kurti <kurti@...icto.ai>
Cc: Andy Whitcroft <apw@...onical.com>, Joe Perches <joe@...ches.com>,
Dwaipayan Ray <dwaipayanray1@...il.com>, Lukas Bulwahn
<lukas.bulwahn@...il.com>, Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor
<alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo
<gary@...yguo.net>, Björn Roy Baron
<bjorn3_gh@...tonmail.com>, Benno
Lossin <benno.lossin@...ton.me>, Andreas Hindborg
<a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross
<tmgross@...ch.edu>, linux-kernel@...r.kernel.org,
rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v4] checkpatch: add warning for pr_* and dev_* macros
without a trailing newline
Alban Kurti <kurti@...icto.ai> writes:
> Add a new check in scripts/checkpatch.pl to detect usage of pr_(level)
> and dev_(level) macros (for both C and Rust) when the string literal
> does not end with '\n'. Missing trailing newlines can lead to incomplete
> log lines that do not appear properly in dmesg or in console output.
> To show an example of this working after applying the patch we can run
> the script on the commit that likely motivated this need/issue:
> ./scripts/checkpatch.pl --strict -g "f431c5c581fa1"
>
> Also, the patch is able to handle correctly if there is a printing call
> without a newline which then has a newline printed via pr_cont for
> both Rust and C alike. If there is no newline printed and the patch
> ends or there is another pr_* call before a newline with pr_cont is
> printed it will show a warning. Not implemented for dev_cont because it
> is not clear to me if that is used at all.
>
> One false warning that will be generated due to this change is in case
> we have a patch that modifies a `pr_* call without a newline` which has a
> pr_cont with a newline following it. In this case there will be a
> warning but because the patch does not include the following pr_cont it
> will warn there is nothing creating a newline. I have modified the
> warning to be softer due to this known problem.
>
> I have tested with comments, whitespace, differen orders of pr_* calls
> and pr_cont and the only case that I suspect to be a problem is the one
> outlined above.
This is now a more significant change, I belive we should document it
this under Documentation/dev-tools/checkpatch.rst. Where you can also
provide examples/use-cases.
>
> Suggested-by: Miguel Ojeda <ojeda@...nel.org>
> Closes: https://github.com/Rust-for-Linux/linux/issues/1140
> Signed-off-by: Alban Kurti <kurti@...icto.ai>
> ---
> Changes since v3:
> - Just reordered the checkpatch.pl code original addition as it did not work properly
> - Link to v3: https://lore.kernel.org/all/20250207-checkpatch-newline-v3-1-20d8774f16ea@invicto.ai/
> ---
> scripts/checkpatch.pl | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 96 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 9eed3683ad76caffbbb2418e5dbea7551d374406..0e7684d2f0cf30575640d7c4da9e51a13d91463b 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -77,6 +77,8 @@ my ${CONFIG_} = "CONFIG_";
>
> my %maybe_linker_symbol; # for externs in c exceptions, when seen in *vmlinux.lds.h
>
> +my $pending_log = undef;
> +
> sub help {
> my ($exitcode) = @_;
>
> @@ -3888,6 +3890,91 @@ sub process {
> }
> }
>
> +# check for pr_* and dev_* logs without a newline for C and Rust files to avoid missing log messages
> + my $pr_cont_pattern = qr{
> + \b
> + pr_cont!?
> + \s*
> + \(
> + \s*
> + "([^"]*)"
> + [^)]*
> + \)
> + }x;
> + my $log_macro_pattern = qr{
> + \b
> + (
> + pr_(?:emerg|alert|crit|err|warn|notice|info|debug)
> + | dev_(?:emerg|alert|crit|err|warn|notice|info|dbg)
> + )
> + (!?)
> + \s*
> + \(
> + \s*
> + "([^"]*)"
> + }x;
> +
> + if ($realfile =~ /\.(?:c|h|rs)$/) {
> + if ($rawline =~ /^\+/) {
> + my $cleanline = $rawline;
> + $cleanline =~ s/^[+\s]+//;
> + $cleanline =~ s/\r?$//;
> + $cleanline =~ s{/\*.*?\*/}{}g;
> + $cleanline =~ s{//.*}{}g;
> +
> + if ($pending_log) {
> + if ($cleanline =~ /$pr_cont_pattern/) {
> + my $cont_string_arg = $1;
> + if ($cont_string_arg =~ /\\n$/) {
> + $pending_log = undef;
> + }
> + } elsif ($cleanline =~ /$log_macro_pattern/) {
> + WARN($pending_log->{lang} . "_LOG_NO_NEWLINE",
> + "Possible usage of $pending_log->{macro_call} without a trailing newline.\n" .
> + $pending_log->{herecurr});
> +
> + $pending_log = undef;
> +
> + my $macro_call = $1;
> + my $maybe_excl = $2;
> + my $string_arg = $3;
> + $string_arg =~ s/\s+$//;
> +
> + if ($realfile =~ /\.rs$/ && $maybe_excl ne '!') {
> + return;
> + }
> +
> + if ($string_arg !~ /\\n$/ && $string_arg !~ /\n$/) {
> + $pending_log = {
> + macro_call => $macro_call,
> + herecurr => $herecurr,
> + lang => ($realfile =~ /\.rs$/) ? "Rust" : "C",
> + };
> + }
> + }
> + } else {
> + if ($cleanline =~ /$log_macro_pattern/) {
> + my $macro_call = $1;
> + my $maybe_excl = $2;
> + my $string_arg = $3;
> + $string_arg =~ s/\s+$//;
> +
> + if ($realfile =~ /\.rs$/ && $maybe_excl ne '!') {
> + return;
> + }
> +
> + if ($string_arg !~ /\\n$/ && $string_arg !~ /\n$/) {
> + $pending_log = {
> + macro_call => $macro_call,
> + herecurr => $herecurr,
> + lang => ($realfile =~ /\.rs$/) ? "Rust" : "C",
> + };
> + }
> + }
> + }
> + }
> + }
> +
> # check for .L prefix local symbols in .S files
> if ($realfile =~ /\.S$/ &&
> $line =~ /^\+\s*(?:[A-Z]+_)?SYM_[A-Z]+_(?:START|END)(?:_[A-Z_]+)?\s*\(\s*\.L/) {
> @@ -7678,6 +7765,15 @@ sub process {
> }
> }
>
> +# pending log means a pr_* without an ending newline has not
> +# been followed by a pr_cont call with a newline at the end
> + if ($pending_log) {
> + WARN($pending_log->{lang} . "_LOG_NO_NEWLINE",
> + "Usage of $pending_log->{macro_call} without a trailing newline.\n" .
> + $pending_log->{herecurr});
> + $pending_log = undef;
> + }
> +
> # If we have no input at all, then there is nothing to report on
> # so just keep quiet.
> if ($#rawlines == -1) {
>
> ---
> base-commit: ceff0757f5dafb5be5205988171809c877b1d3e3
> change-id: 20250207-checkpatch-newline2-f60275e30eb6
>
> Best regards,
Powered by blists - more mailing lists