Re: [PATCH 0/4] Stop TIF_NOTIFY_SIGNAL from interrupting work that can't be restarted

Hi Christian,

I am still travelling without my work laptop, can't read the code.
I'll try to take a look on Friday.

IIRC, zap_pid_ns() can use the new helper too...

Oleg.

On 08/18, Christian Brauner wrote:
>
> Ok, so I was looking into things and as usual got
> side-tracked so here we are. Oleg, save me please.
> 
> IF_NOTIFY_SIGNAL is used to kick a task in uninterruptible sleep to
> return to userspace and run task work and then go back to sleep. This
> mechanism works well but breaks coredumps. dump_interrupted() only
> allows fatal signals to interrupt a coredump and the whole regular write
> path going to actual filesystems is impervious to TIF_NOTIFY_SIGNAL as
> well.
> 
> The core is that you can have quite deep callchains that end up calling
> signal_pending() in both the pipe and the socket codepaths so it's like
> we can just pass a flag through somehow.
> 
> For coredumps its very annoying because it means io_uring is - depending
> on how much outstanding work you have - incompatible with generating
> non-truncated coredumps. A process with too many file backed mappings
> and io_uring requests in flight ends up losing most of the coredump.
> 
> While zap_threads() has cleared TIF_SIGPENDING for a long time, just
> clearing TIF_NOTIFY_SIGNAL isn't going to work because the next
> completion will just set it again.
> 
> The fun part also is that io_uring isn't actually the only case:
> 
> (1) io_uring
> 
> (2) klp_send_signals()
> 
> (3) bpf_task_work_schedule_signal()
> 
> (4) landlock's tsync
> 
> And technically, kthread_stop() and the printk kunit test set the bit
> raw. So no simple way of just fixing this in one subsystem.
> 
> So, a fix for this issue has the following constraints:
> 
> (i) The places where a write is aborted are deep callchains that we
>     can't reasonably parameterize. For example, anon_pipe_write(),
>     unix_stream_sendmsg(), unix_stream_read_generic(),
>     sk_stream_wait_memory(), or a bare wait_event_interruptible() in
>     wait_for_dump_helpers(). All of them are shared with regular
>     syscalls that must stay interruptible.
> 
>     IOW, the state has to be per-task and ambient.
> 
>     An LLM would call this "load bearing"...
> 
> (ii) There are multiple ways TIF_NOTIFY_SIGNAL can get raised and they
>      can get set from irq context against any task. As said above we
>      have at least io_uring paths (poll task_work, msg_ring, tctx exit,
>      io-wq via __set_notify_signal()), bpf_task_work_schedule_signal(),
>      klp_send_signals(), landlock tsync, plus kthread_stop() and the
>      printk kunit stuff that set the bit raw.
> 
>      So fixing this up in the individual subsystems is doomed to fail or
>      require constant audits in case some new variant shows up.
> 
> (iii) The coredump task is exiting and can't restart the work.
> 
> So here's some stuff that was considered but I think it not really
> feasible:
> 
> (a) Check for task_is_coredumping(). That will end up forcing
>     task_work_add() users to know about coredumps. This seems like a
>     layering violation.
> 
>     I have a patch for this as well but it's ugly. It races with the
>     dump staarting unless the bit setter takes a lock in the io_uring
>     completion. I know someone that will disagree with this approach. ;)
> 
> (b) Oleg's old suggestion iirc. Just clear the bit at coredump entry.
>     That doesn't work because io_uring poll completions just raise it
>     again from irq context. So you also need synchronization with the
>     setter of which there are quite a few.
> 
> (c) Let the setting task defer setting the bit if the task is flagged
>     and then raise it again at exit.
> 
> (d) Take TIF_NOTIFY_SIGNAL out of singal_pending() and make it opt-in at
>     specific points. That breaks io_uring quite badly and forces a
>     tree-wide audit.
> 
> So the amount of patches for this issue over the years is impressive. So
> let me add one to the pile for the lolz.
> 
> Add PF_NO_NOTIFY_SIGNAL and helpers to raise/restore it. This is the
> same approach as memalloc_nofs_save(). signal_pending() will not report
> a fake pending signal via TIF_NOTIFY_SIGNAL if inside a
> PF_NO_NOTIFY_SIGNAL critical section. Obviously you can't
> fork()/clone3() in such a section.
> 
> Fix coredumps and smb. Fwiw, I think there's a few other potential users
> of the helpers that are left out of this series.
> 
> |         |    notify_signal_pipe     |   notify_signal_socket    |
> |---------|---------------------------|---------------------------|
> | fix     | ok                        | ok                        |
> |---------|---------------------------|---------------------------|
> | unfixed | 357324 of ≥ 5111808 bytes | 467336 of ≥ 5111808 bytes |
> 
> Signed-off-by: Christian Brauner (Amutable) <[email protected]>
> ---
> Christian Brauner (4):
>       signal: allow taks to temporarily block TIF_NOTIFY_SIGNAL
>       coredump: prevent TIF_NOTIFY_SIGNAL from interrupting coredumps
>       selftests/coredump: test that TIF_NOTIFY_SIGNAL doesn't truncate a coredump
>       smb: prevent TIF_NOTIFY_SIGNAL from interrupting
> 
>  fs/coredump.c                                      |   4 +
>  fs/smb/client/transport.c                          |  13 +-
>  include/linux/sched.h                              |   2 +-
>  include/linux/sched/signal.h                       |  21 +-
>  tools/testing/selftests/coredump/Makefile          |   7 +-
>  .../selftests/coredump/coredump_notify_signal.h    |  29 ++
>  .../coredump/coredump_notify_signal_helper.c       |  46 +++
>  .../coredump/coredump_notify_signal_test.c         | 245 ++++++++++++++++
>  tools/testing/selftests/coredump/coredump_test.h   |   1 +
>  .../selftests/coredump/coredump_test_helpers.c     | 318 +++++++++++++++++++++
>  10 files changed, 674 insertions(+), 12 deletions(-)
> ---
> base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f
> change-id: 20260817-work-tif_notify_signal-6ab080d33693
>