linux-kernel - Re: [PATCH v3 1/2] hung_task: Introduce helper for hung task warning

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] [day] [month] [year] [list]

Message-ID: <q6ldjtrsll45ovvbuorzxjr2wr7hamf6maedlmqbefagqczmoo@epcog7bvhc5s>
Date: Sun, 21 Dec 2025 16:52:47 -0500
From: Aaron Tomlin <atomlin@...mlin.com>
To: Petr Mladek <pmladek@...e.com>
Cc: akpm@...ux-foundation.org, lance.yang@...ux.dev, mhiramat@...nel.org, 
	gregkh@...uxfoundation.org, sean@...e.io, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/2] hung_task: Introduce helper for hung task warning

On Wed, Dec 17, 2025 at 10:39:07AM +0100, Petr Mladek wrote:
> It makes sense to make the computation separately.

Hi Petr,

Thank you for the detailed review and the constructive feedback.

> > +	const char *coredump_msg = "      Blocked by coredump.";
> 
> Honestly, I do not see any advantage in storing the string into a
> variable and passing it via %s. It does not help with readability
> because the reader has to lookup the variable definition. And it
> is less effective code (not a big deal but...).
> 
> Instead, I would add "\n". It will allow to show the message
> immediately on consoles. Without the trailing "\n", the message
> is not finalized because it might still get extended by pr_cont().
> 
> > +	const char *disable_msg =
> > +		"\"echo 0 > /proc/sys/kernel/hung_task_timeout_secs\""
> > +		" disables this message.";
> 
> Same here. I would avoid the %s and add the trailing '\n'.
> 
> Also the message should be on a single line. It makes it easier
> to find it via "git grep". ./scripts/checkpatch.pl even complain
> about it:
> 
> WARNING: quoted string split across lines
> #37: FILE: kernel/hung_task.c:239:
> +		"\"echo 0 > /proc/sys/kernel/hung_task_timeout_secs\""
> +		" disables this message.";
> 
> 
> > +	pr_err("INFO: task %s:%d blocked for more than %ld seconds.\n",
> > +	       t->comm, t->pid, blocked_secs);
> > +	pr_err("      %s %s %.*s\n",
> > +	       print_tainted(), init_utsname()->release,
> > +	       (int)strcspn(init_utsname()->version, " "),
> > +	       init_utsname()->version);
> > +	if (t->flags & PF_POSTCOREDUMP)
> > +		pr_err("%s\n", coredump_msg);
> > +	pr_err("%s\n", disable_msg);
> > +}
> 
> Otherwise, I like the change.

I entirely agree with your observations. Upon reflection, using local
variables for these strings was indeed an unnecessary abstraction that
hindered rather than helped readability.


Kind regards,
-- 
Aaron Tomlin