linux-kernel - Re: [PATCH] spin_lock_irqsave() in autofs

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: <20250817174741.GX222315@ZenIV>
Date: Sun, 17 Aug 2025 18:47:41 +0100
From: Al Viro <viro@...iv.linux.org.uk>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Ian Kent <raven@...maw.net>, autofs@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] spin_lock_irqsave() in autofs_write() is bogus

On Sun, Aug 17, 2025 at 09:50:25AM -0700, Linus Torvalds wrote:
> On Sun, 17 Aug 2025 at 09:36, Al Viro <viro@...iv.linux.org.uk> 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...