linux-kernel - Re: [PATCH v4 2/6] perf: Enqueue SIGTRAP always via task

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: <Z1DxqJlGM_I8irVQ@pavilion.home>
Date: Thu, 5 Dec 2024 01:19:52 +0100
From: Frederic Weisbecker <frederic@...nel.org>
To: Oleg Nesterov <oleg@...hat.com>
Cc: Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	"Lai, Yi" <yi1.lai@...ux.intel.com>,
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
	Adrian Hunter <adrian.hunter@...el.com>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Daniel Bristot de Oliveira <bristot@...nel.org>,
	Ian Rogers <irogers@...gle.com>, Ingo Molnar <mingo@...hat.com>,
	Jiri Olsa <jolsa@...nel.org>, Kan Liang <kan.liang@...ux.intel.com>,
	Marco Elver <elver@...gle.com>, Mark Rutland <mark.rutland@....com>,
	Namhyung Kim <namhyung@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Arnaldo Carvalho de Melo <acme@...hat.com>, yi1.lai@...el.com,
	syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH v4 2/6] perf: Enqueue SIGTRAP always via task_work.

Le Wed, Dec 04, 2024 at 02:48:27PM +0100, Oleg Nesterov a écrit :
> On 11/11, Sebastian Andrzej Siewior wrote:
> Not that bad, but I personally dislike this patch for other reasons.
> But lets forget it for the moment.
> 
> The numbers in
> 
> 	PATCH] task_work: remove fifo ordering guarantee
> 	https://lore.kernel.org/all/1440816150.8932.123.camel@edumazet-glaptop2.roam.corp.google.com/
> 
> didn't look too bad too, yet they convinced Linus and other reviewers.
> 
> I still think that fifo makes much more sense. The main (only?) offender
> is fput(), so perhaps we can do something like
> https://lore.kernel.org/all/20150907134924.GA24254@redhat.com/
> but when I look at this change now I see it is racy.
> 
> Stupid question. What if we revert this "task_work: remove fifo ordering guarantee"
> patch above? Can this help?
> 
> I don't understand this code and the problem. But when I (try to) read the
> previous discussion on lore.kernel.org it seems that perf_pending_task_sync()
> fails to cancel event->pending_task because it is called from task_work_run()
> and then rcuwait_wait_event() obviously hangs.
> 
> Your patch can only help if task_work_add(current, &event->pending_task) was
> called before fput()->task_work_add(task, &file->f_task_work(), right?

Right, IIUC if &event->pending_task was added after then perf_pending_task()
would be called before perf_release() and we wouldn't have the problem.

> So perhaps, if we restore the fifo ordering, we can rely on the fact that
> current should call perf_pending_task() before it calls perf_release/free_event ?

Hmm but a perf event can still fire between the task_work_add() on fput and the
actual call to task_work_run() that will run the queue. So &event->pending_task
can be set either before or after. And then whether fifo or lifo, that would
still be buggy. Or am I missing something?

Looking at task_work, it seems that most enqueues happen to the current task.
AFAICT, only io_uring() does remote enqueue. Would it make sense to have a light
version of task_work that is only ever used by current? This would be a very
simple flavour with easy queue and cancellation without locking/atomics/RmW
operations. We would just need to be extra careful about NMIs. And cancellation
on the current queue would be more deterministic...

Of course we would then lose the advantage of a solution that works for both
remote and current enqueue...

Thanks.