On Sun, Aug 17, 2025 at 09:50:25AM -0700, Linus Torvalds wrote:
> On Sun, 17 Aug 2025 at 09:36, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> >
> > That function should never be (and never is) called with irqs
> > disabled - we have an explicit mutex_lock() in there, if nothing else.
> > Which makes spin_lock_irqsave() use in there pointless - we do need to
> > disable irqs for ->siglock, but that should be spin_lock_irq().
>
> I think we basically did the irqsave/restore version as the default
> when not wanting to think about the context.
>
> Your patch looks fine, but I doubt it's measurable outside of "it
> makes the code a few bytes smaller".
>
> So ACK on it, but I'm not convinced it's worth spending time actively
> _looking_ for these kinds of things.
It's obviously not a hot path of any sort; the only cost is head-scratching
of later readers. OTOH, seeing that nobody had looked at it that one in
what, 28 years...
For another head-scratcher in that area: I started to look at __rcu sparse
noise, and ->sighand->siglock accesses had been quite a part of that.
current->sighand is obviously stable and should need no rcu_dereference(),
right? So what the hell is
rcu_read_lock();
sighand = rcu_dereference(current->sighand);
spin_lock_irqsave(&sighand->siglock, flags);
recalc_sigpending();
spin_unlock_irqrestore(&sighand->siglock, flags);
rcu_read_unlock();
in net/sunrpc/svc.c:svc_unregister() about? Is there something subtle I'm
missing there? AFAICS, that came from 00a87e5d1d67 "SUNRPC: Address
RCU warning in net/sunrpc/svc.c" and commit message in there contains
no explanation beyond "sparse is complaining, let's make it STFU"...
Looking for ->sighand stores gives this:
fs/exec.c:1070: rcu_assign_pointer(me->sighand, newsighand);
store to current->sighand, called from begin_new_exec(), with
'me' set to current.
kernel/exit.c:215: tsk->sighand = NULL;
__exit_signal(), from release_task(), and if it's ever done
asynchronously to the current thread, we are really fucked (note that
we do *not* check that sighand is non-NULL in that snippet).
kernel/fork.c:1608: RCU_INIT_POINTER(tsk->sighand, sig);
copy_sighand(), from copy_process(), done to the child task
that is nowhere near running at that point.
So nothing weird has happened and that code looks very much like
a pointless head-scratcher. Sure, the cost in execution time is
not going to be measurable, but the cost for readers is non-trivial...
FWIW, I suspect that the right way would be something like
static inline struct sighand_struct *current_sighand(void)
{
return unrcu_pointer(current->sighand);
}
if unrcu_pointer is idiomatic in such case, plus conversion of open-coded
instances to it...
