Re: [PATCH] signal: Use list_del_init_careful() in flush_sigqueue()

[Bradley Morgan]

Hi Hyunwoo,

> That second half does not hold for the old group leader in a non-leader
> exec(). de_thread() calls exchange_tids() before release_task(leader), so
> the struct pid held by a SIGEV_THREAD_ID timer created against the
> leader's
> tid now points to the thread which called execve(). pid_task() returns
> that
> thread and lock_task_sighand() on it succeeds.

This is the part to be sure of, and it checks out. In de_thread(),
exchange_tids() runs before release_task(leader), so the timer's struct pid
resolves to the exec'ing thread instead of going stale. The "pid_task()
returns NULL" assumption from fb3bbcfe344e really does break here. Nasty
one.

> posixtimer_send_sigqueue() checks whether the sigqueue is already queued
> with a plain list_empty(), which only reads ->next. list_del_init() is
> not
> atomic and INIT_LIST_HEAD() stores ->next before ->prev, so the check can
> pass in between.

Right. INIT_LIST_HEAD() does the ->next store first, so there is a window
where the reader sees the entry as unqueued while the flush still has its
->prev store left, and that store then lands on top of the requeue.

> Use list_del_init_careful(), which stores ->next last. A list_empty()
> which
> sees the entry unqueued is then guaranteed that the flush will not store
> into the entry any more.

This reads wrong at first glance, since list_del_init_careful() is
documented
to pair with list_empty_careful(), and you leave the reader as plain
list_empty(). But it is correct, and your changelog is the reason: ->next
becomes the last store, and list_empty() gates on ->next, so seeing it
pointing at itself means the flush is fully done with the entry and nothing
can land after. That is exactly the guarantee needed here, no acquire
required
on the read.

Only nit, and optional: that reasoning is the whole patch but it lives in
the
changelog. The comment in the code just says it pairs with the
list_empty().
If someone later decides the careful/plain mix looks like a mistake and
reverts
the del back to list_del_init(), the bug comes back. One clause pinning
"must
store ->next last" to the code would stop that. Feel free to bikeshed.

> Fixes: fb3bbcfe344e ("exit: change the release_task() paths to call flush_sigqueue() lockless")
> Cc: stable@xxxxxxxxxxxxxxx

Both right, it landed in 6.14.

Real bug, well decoded, minimal fix.

Reviewed-by: Bradley Morgan <include@xxxxxxxxx>

Thanks!