Re: [GIT PULL] sigqueue cache fix

[Linus Torvalds]

[ Adding Christian and Oleg to participants ]

On Thu, Jun 24, 2021 at 9:29 AM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> So I think the sigqueue cache is still potentially quite buggy, and I
> think the bug is hidden by the READ/WRITE_ONCE games that are
> misleading and not actually valid.

Guys, I haven't heard any reaction to this. Any comments?

Because the more I look at it, the stranger it looks.

In particular: the code in sigqueue_cache_or_free() is a simple

        if (!READ_ONCE(current->sigqueue_cache))
                WRITE_ONCE(current->sigqueue_cache, q);

and it is documented to be safe because "current" is obviously single-threaded.

Except that documented "obviously" is not so obvious at all. Yes,
"current" is single-threaded, but only in task context. You can still
have interrupts etc that see that same "current" that happen
concurrently.

So it's not at all obviously safe. It *may* be safe, but it worries me.

It worries me _particularly_ with exactly this commit 399f8dd9a866
("signal: Prevent sigqueue caching after task got released").

Why? Because the alleged path is release_task() -> __exit_signal() ->
exit_task_sigqueue_cache(). And by the time "release_task()" runs,
that task it releases shouldn't be running. So how can release_task()
race with this logic in sigqueue_cache_or_free()?

IOW how can that change in commit 399f8dd9a866 _possibly_ fix
anything? That would seem to be a serious problem if it's actually the
case..

So I think

 (a) sigqueue_cache_or_free() is fine only if no signal is ever
released from interrupt/bh context.

 (b) commit 399f8dd9a866 looks dodgy to me - could we really ever do
"release_task(current)" without it being a huge bug?

Anyway, trying to really distill the logic of the sigqueue_cache, I've
come up with

 - sigqueue_cache_or_free() only does something if saw NULL (and will
turn it non-NULL)

 - __sigqueue_alloc() only does something if it saw a non-NULL value
(and will turn it NULL)

so they can't race with each other, because their initial values are disjoint.

So then we have

 (a) sigqueue_cache_or_free() allegedly cannot race with itself
because of "current".

 (b) __sigqueue_alloc() allegedly cannot race with itself because of
sighand->siglock

Now (b) I will actually believe, because __sigqueue_alloc() has only
two callers, and the first one actually has

        assert_spin_locked(&t->sighand->siglock);

and the second one passes SIGQUEUE_PREALLOC as sigqueue_flags, and
that will force it to not touch sigqueue_cache at all.

So I think __sigqueue_alloc() is ok.

Which makes me really suspect that (a) is the problem here.

Looking at what calls __sigqueue_free() -> sigqueue_cache_or_free(), we have:

 - flush_sigqueue

 - flush_itimer_signals() -> __flush_itimer_signals()

 - dequeue_signal() -> __dequeue_signal -> collect_signal()

 - get_signal() -> dequeue_synchronous_signal() (and dequeue_signal())

 - send_signal() -> __send_signal() -> prepare_signal() -> flush_sigqueue_mask()

 - kill_pid_usb_asyncio() -> __send_signal() -> ..

 - do_notify_parent() -> __send_signal() -> ..

 - send_sigqueue() -> prepare_signal() -> flush_sigqueue_mask()

 - kernel_sigaction() -> flush_sigqueue_mask()

 - sigqueue_free()

so there's a lot of things that can get into sigqueue_cache_or_free(),
and it's worth noting that that path does *NOT* serialize on the
sighand->siglock, but expressly purely on "current" being
single-threaded (and 'current' has nothing to do with sighand->siglock
anyway, the sighand lock is for the target of the signal, not the
source of it).

At at least that send_signal() path is very much called from
interrupts (ie timers etc).

Hmm?

Ok, I may have confused myself looking at all this, but it does all
make me think this is dodgy.

                  Linus