linux-kernel - Re: [PATCH 2/2] getrusage: use sig->stats

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]

Date: Sun, 21 Jan 2024 13:07:54 +0100
From: Oleg Nesterov <oleg@...hat.com>
To: Andrew Morton <akpm@...ux-foundation.org>
Cc: Dylan Hatch <dylanbhatch@...gle.com>, Kees Cook <keescook@...omium.org>,
	Frederic Weisbecker <frederic@...nel.org>,
	"Joel Fernandes (Google)" <joel@...lfernandes.org>,
	Ard Biesheuvel <ardb@...nel.org>,
	"Matthew Wilcox (Oracle)" <willy@...radead.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Vincent Whitchurch <vincent.whitchurch@...s.com>,
	Dmitry Vyukov <dvyukov@...gle.com>,
	Luis Chamberlain <mcgrof@...nel.org>,
	Mike Christie <michael.christie@...cle.com>,
	David Hildenbrand <david@...hat.com>,
	Catalin Marinas <catalin.marinas@....com>,
	Stefan Roesch <shr@...kernel.io>, Joey Gouly <joey.gouly@....com>,
	Josh Triplett <josh@...htriplett.org>, Helge Deller <deller@....de>,
	Ondrej Mosnacek <omosnace@...hat.com>,
	Florent Revest <revest@...omium.org>,
	Miguel Ojeda <ojeda@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] getrusage: use sig->stats_lock

On 01/20, Andrew Morton wrote:
>
> On Fri, 19 Jan 2024 19:27:49 -0800 Dylan Hatch <dylanbhatch@...gle.com> wrote:
>
> >
> > I applied these to a 5.10 kernel, and my repro (calling getrusage(RUSAGE_SELF)
> > from 200K threads) is no longer triggering a hard lockup.
>
> Thanks, but...
>
> The changelogs don't actually describe any hard lockup.  [1/2] does
> mention "the deadlock" but that's all the info we have.

Sorry for confusion... 1/2 tries to explain that this change is not
strictly necessary for 2/2, it is safe to call thread_group_cputime()
with sig->stats_lock held for writing even if thread_group_cputime()
takes the same lock, because in this case thread_group_cputime() can't
enter the slow mode.

> So could we please have a suitable description of the bug which these are
> addressing?  And a Reported-by:, a Closes: and a Fixes would be great too.

Yes sorry I forgot to add Reported-by. So I'll try to update the changelog
and add Reported-and-tested-by.

But the problem is known and old. I think do_io_accounting() had the same
problem until 1df4bd83cdfdbd0 ("do_io_accounting: use sig->stats_lock").
and do_task_stat() ...

getrusage() takes siglock and does for_each_thread() twice. If NR_THREADS
call sys_getrusage() in an endless loop on NR_CPUS, lock_task_sighand()
can trigger a hard lockup because it spins with irqs disabled waiting
for other NR_CPUS-1 which need the same siglock. So the time it spins
with irqs disabled is O(NR_CPUS * NR_THREADS).

With this patch all the threads can run lockless in parallel in the
likely case.

Dylan, do you have a better description? Can you share your repro?
although I think that something simple like

	#define NT BIG_NUMBER

	pthread_barrier_t barr;

	void *thread(void *arg)
	{
		struct rusage ru;

		pthread_barrier_wait(&barr);
		for (;;)
			getrusage(RUSAGE_SELF, &ru);
		return NULL;
	}

	int main(void)
	{
		pthread_barrier_init(&barr, NULL, NT);

		for (int n = 0; n < NT-1; ++n) {
			pthread_t pt;
			pthread_create(&pt, NULL, thread, NULL);
		}
		thread(NULL);

		return 0;
	}

should work if you have a machine with a lot of memory/cpus.

Oleg.