Horton

Thread

Commits

  1. Fix double-release of spinlock

  2. Move cancel key generation to after forking the backend

  1. pgsql: Fix double-release of spinlock

    Heikki Linnakangas <heikki.linnakangas@iki.fi> — 2024-07-29T15:24:52Z

    Fix double-release of spinlock

Commit 9d9b9d46f3 added spinlocks to protect the fields in ProcSignal
flags, but in EmitProcSignalBarrier(), the spinlock was released
twice. With most spinlock implementations, releasing a lock that's not
held is not easy to notice, because most of the time it does nothing,
but if the spinlock was concurrently acquired by another process, it
could lead to more serious issues. Fortunately, with the
--disable-spinlocks emulation implementation, it caused more visible
failures.

In the passing, fix a type in comment and add an assertion that the
procNumber passed to SendProcSignal looks valid.

Discussion: https://www.postgresql.org/message-id/b8ce284c-18a2-4a79-afd3-1991a2e7d246@iki.fi

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/0393f542d72c6182271c392d9a83d0fc775113c7

Modified Files
--------------
src/backend/storage/ipc/procsignal.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

  2. Re: pgsql: Fix double-release of spinlock

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-29T15:31:56Z

    Heikki Linnakangas <heikki.linnakangas@iki.fi> writes:
> Commit 9d9b9d46f3 added spinlocks to protect the fields in ProcSignal
> flags, but in EmitProcSignalBarrier(), the spinlock was released
> twice. With most spinlock implementations, releasing a lock that's not
> held is not easy to notice, because most of the time it does nothing,
> but if the spinlock was concurrently acquired by another process, it
> could lead to more serious issues. Fortunately, with the
> --disable-spinlocks emulation implementation, it caused more visible
> failures.

There was some recent discussion about getting rid of
--disable-spinlocks on the grounds that nobody would use
hardware that lacked native spinlocks.  But now I wonder
if there is a testing/debugging reason to keep it.

			regards, tom lane

  3. Re: pgsql: Fix double-release of spinlock

    Andres Freund <andres@anarazel.de> — 2024-07-29T16:18:46Z

    Hi,

On 2024-07-29 11:31:56 -0400, Tom Lane wrote:
> Heikki Linnakangas <heikki.linnakangas@iki.fi> writes:
> > Commit 9d9b9d46f3 added spinlocks to protect the fields in ProcSignal
> > flags, but in EmitProcSignalBarrier(), the spinlock was released
> > twice. With most spinlock implementations, releasing a lock that's not
> > held is not easy to notice, because most of the time it does nothing,
> > but if the spinlock was concurrently acquired by another process, it
> > could lead to more serious issues. Fortunately, with the
> > --disable-spinlocks emulation implementation, it caused more visible
> > failures.
> 
> There was some recent discussion about getting rid of
> --disable-spinlocks on the grounds that nobody would use
> hardware that lacked native spinlocks.  But now I wonder
> if there is a testing/debugging reason to keep it.

Seems it'd be a lot more straightforward to just add an assertion to the
x86-64 spinlock implementation verifying that the spinlock isn't already free?

Greetings,

Andres Freund

  4. Re: pgsql: Fix double-release of spinlock

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-29T16:33:13Z

    Andres Freund <andres@anarazel.de> writes:
> On 2024-07-29 11:31:56 -0400, Tom Lane wrote:
>> There was some recent discussion about getting rid of
>> --disable-spinlocks on the grounds that nobody would use
>> hardware that lacked native spinlocks.  But now I wonder
>> if there is a testing/debugging reason to keep it.

> Seems it'd be a lot more straightforward to just add an assertion to the
> x86-64 spinlock implementation verifying that the spinlock isn't already free?

I dunno, is that the only extra check that the --disable-spinlocks
implementation is providing?

I'm kind of allergic to putting Asserts into spinlocked code segments,
mostly on the grounds that it violates the straight-line-code precept.
I suppose it's not really that bad for tests that you don't expect
to fail, but still ...

			regards, tom lane

  5. Re: pgsql: Fix double-release of spinlock

    Andres Freund <andres@anarazel.de> — 2024-07-29T16:40:26Z

    Hi,

On 2024-07-29 12:33:13 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2024-07-29 11:31:56 -0400, Tom Lane wrote:
> >> There was some recent discussion about getting rid of
> >> --disable-spinlocks on the grounds that nobody would use
> >> hardware that lacked native spinlocks.  But now I wonder
> >> if there is a testing/debugging reason to keep it.
> 
> > Seems it'd be a lot more straightforward to just add an assertion to the
> > x86-64 spinlock implementation verifying that the spinlock isn't already free?

FWIW, I quickly hacked that up, and it indeed quickly fails with 0393f542d72^
and passes with 0393f542d72.


> I dunno, is that the only extra check that the --disable-spinlocks
> implementation is providing?

I think it also provides the (valuable!) check that spinlocks were actually
initialized. But that again seems like something we'd be better off adding
more general infrastructure for - nobody runs --disable-spinlocks locally, we
shouldn't need to run this on the buildfarm to find problems like this.


> I'm kind of allergic to putting Asserts into spinlocked code segments,
> mostly on the grounds that it violates the straight-line-code precept.
> I suppose it's not really that bad for tests that you don't expect
> to fail, but still ...

I don't think the spinlock implementation itself is really affected by that
rule - after all, the --disable-spinlocks implementation actually consists out
of several layers of external function calls (including syscalls in some
cases!).

Greetings,

Andres Freund

  6. Re: pgsql: Fix double-release of spinlock

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-29T16:45:19Z

    Andres Freund <andres@anarazel.de> writes:
> On 2024-07-29 12:33:13 -0400, Tom Lane wrote:
>> I dunno, is that the only extra check that the --disable-spinlocks
>> implementation is providing?

> I think it also provides the (valuable!) check that spinlocks were actually
> initialized. But that again seems like something we'd be better off adding
> more general infrastructure for - nobody runs --disable-spinlocks locally, we
> shouldn't need to run this on the buildfarm to find problems like this.

Hmm, but how?  One of the things we gave up by nuking HPPA support
was that that platform's representation of an initialized, free
spinlock was not all-zeroes, so that it'd catch this type of problem.
I think all the remaining platforms do use zeroes, so it's hard to
see how anything short of valgrind would be likely to catch it.

			regards, tom lane

  7. Detect double-release of spinlock

    Andres Freund <andres@anarazel.de> — 2024-07-29T16:51:54Z

    Hi,

On 2024-07-29 09:40:26 -0700, Andres Freund wrote:
> On 2024-07-29 12:33:13 -0400, Tom Lane wrote:
> > Andres Freund <andres@anarazel.de> writes:
> > > On 2024-07-29 11:31:56 -0400, Tom Lane wrote:
> > >> There was some recent discussion about getting rid of
> > >> --disable-spinlocks on the grounds that nobody would use
> > >> hardware that lacked native spinlocks.  But now I wonder
> > >> if there is a testing/debugging reason to keep it.
> > 
> > > Seems it'd be a lot more straightforward to just add an assertion to the
> > > x86-64 spinlock implementation verifying that the spinlock isn't already free?
> 
> FWIW, I quickly hacked that up, and it indeed quickly fails with 0393f542d72^
> and passes with 0393f542d72.

Thought it'd be valuable to post a patch to go along with this, to
-hackers. The thread started at [1]

Other context from this discussion:
> > I dunno, is that the only extra check that the --disable-spinlocks
> > implementation is providing?
> 
> I think it also provides the (valuable!) check that spinlocks were actually
> initialized. But that again seems like something we'd be better off adding
> more general infrastructure for - nobody runs --disable-spinlocks locally, we
> shouldn't need to run this on the buildfarm to find problems like this.
> 
> 
> > I'm kind of allergic to putting Asserts into spinlocked code segments,
> > mostly on the grounds that it violates the straight-line-code precept.
> > I suppose it's not really that bad for tests that you don't expect
> > to fail, but still ...
> 
> I don't think the spinlock implementation itself is really affected by that
> rule - after all, the --disable-spinlocks implementation actually consists out
> of several layers of external function calls (including syscalls in some
> cases!).

Greetings,

Andres Freund

[1] https://www.postgresql.org/message-id/E1sYSF2-001lEB-D1%40gemulon.postgresql.org

  8. Re: Detect double-release of spinlock

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-07-29T17:07:56Z

    On 29/07/2024 19:51, Andres Freund wrote:
> On 2024-07-29 09:40:26 -0700, Andres Freund wrote:
>> On 2024-07-29 12:33:13 -0400, Tom Lane wrote:
>>> Andres Freund <andres@anarazel.de> writes:
>>>> On 2024-07-29 11:31:56 -0400, Tom Lane wrote:
>>>>> There was some recent discussion about getting rid of
>>>>> --disable-spinlocks on the grounds that nobody would use
>>>>> hardware that lacked native spinlocks.  But now I wonder
>>>>> if there is a testing/debugging reason to keep it.
>>>
>>>> Seems it'd be a lot more straightforward to just add an assertion to the
>>>> x86-64 spinlock implementation verifying that the spinlock isn't already free?
>>
>> FWIW, I quickly hacked that up, and it indeed quickly fails with 0393f542d72^
>> and passes with 0393f542d72.

+1. Thanks!

>>> Other context from this discussion:
>>> I dunno, is that the only extra check that the --disable-spinlocks
>>> implementation is providing?
>>
>> I think it also provides the (valuable!) check that spinlocks were actually
>> initialized. But that again seems like something we'd be better off adding
>> more general infrastructure for - nobody runs --disable-spinlocks locally, we
>> shouldn't need to run this on the buildfarm to find problems like this.

Note that the "check" for double-release with the fallback 
implementation wasn't an explicit check either. It just incremented the 
underlying semaphore, which caused very weird failures later in 
completely unrelated code. An explicit assert would be much nicer.

+1 for removing --disable-spinlocks, but let's add this assertion first.

>>> I'm kind of allergic to putting Asserts into spinlocked code segments,
>>> mostly on the grounds that it violates the straight-line-code precept.
>>> I suppose it's not really that bad for tests that you don't expect
>>> to fail, but still ...
>>
>> I don't think the spinlock implementation itself is really affected by that
>> rule - after all, the --disable-spinlocks implementation actually consists out
>> of several layers of external function calls (including syscalls in some
>> cases!).

Yeah I'm not worried about that at all. Also, the assert is made when 
you have already released the spinlock; you are already out of the 
critical section.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  9. Re: Detect double-release of spinlock

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-29T17:25:22Z

    Heikki Linnakangas <hlinnaka@iki.fi> writes:
> Yeah I'm not worried about that at all. Also, the assert is made when 
> you have already released the spinlock; you are already out of the 
> critical section.

Not in the patch Andres posted.

			regards, tom lane

  10. Re: pgsql: Fix double-release of spinlock

    Robert Haas <robertmhaas@gmail.com> — 2024-07-29T17:37:52Z

    On Mon, Jul 29, 2024 at 12:40 PM Andres Freund <andres@anarazel.de> wrote:
> I think it also provides the (valuable!) check that spinlocks were actually
> initialized. But that again seems like something we'd be better off adding
> more general infrastructure for - nobody runs --disable-spinlocks locally, we
> shouldn't need to run this on the buildfarm to find problems like this.

+1. It sucks to have to do special builds to catch a certain kind of
problem. I know I've been guilty of that (ahem, debug_parallel_query
f/k/a force_parallel_mode) but I'm not going to put it on my CV as one
of my great accomplishments. It's much better if we can find a way for
a standard 'make check-world' to tell us about as many things as
possible, so that we don't commit and then find out.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  11. Re: pgsql: Fix double-release of spinlock

    Andres Freund <andres@anarazel.de> — 2024-07-29T17:46:09Z

    On 2024-07-29 12:45:19 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2024-07-29 12:33:13 -0400, Tom Lane wrote:
> >> I dunno, is that the only extra check that the --disable-spinlocks
> >> implementation is providing?
>
> > I think it also provides the (valuable!) check that spinlocks were actually
> > initialized. But that again seems like something we'd be better off adding
> > more general infrastructure for - nobody runs --disable-spinlocks locally, we
> > shouldn't need to run this on the buildfarm to find problems like this.
>
> Hmm, but how?

I think there's a few ways:

> One of the things we gave up by nuking HPPA support
> was that that platform's representation of an initialized, free
> spinlock was not all-zeroes, so that it'd catch this type of problem.
> I think all the remaining platforms do use zeroes, so it's hard to
> see how anything short of valgrind would be likely to catch it.

1) There's nothing forcing us to use 0/1 for most of the spinlock
implementations. E.g. for x86-64 we could use 0 for uninitialized, 1 for free
and 2 for locked.

2) We could also change the layout of slock_t in assert enabled builds, adding
a dedicated 'initialized' field when assertions are enabled. But that might be
annoying from an ABI POV?


1) seems preferrable, so I gave it a quick try. Seems to work. There's a
*slight* difference in the instruction sequence:

old:
    41f6:	f0 86 10             	lock xchg %dl,(%rax)
    41f9:	84 d2                	test   %dl,%dl
    41fb:	75 1b                	jne    4218 <GetRecoveryState+0x38>

new:
    4216:	f0 86 10             	lock xchg %dl,(%rax)
    4219:	80 fa 02             	cmp    $0x2,%dl
    421c:	74 22                	je     4240 <GetRecoveryState+0x40>

I.e. the version using 2 as the locked state uses a three byte instruction vs
a two byte instruction before.


*If* we are worried about this, we could

a) Change the representation only for assert enabled builds, but that'd have
   ABI issues again.

b) Instead define the spinlock to have 1 as the unlocked state and 0 as the
   locked state. That makes it a bit harder to understand that initialization
   is missing, compared to a dedicated state, as the first use of the spinlock
   just blocks.


To make 1) b) easier to understand it might be worth changing the slock_t
typedef to be something like

typedef struct slock_t
{
        char is_free;
} slock_t;

which also might help catch some cases of type confusion - the char typedef is
too forgiving imo.

Greetings,

Andres Freund

  12. Re: Detect double-release of spinlock

    Andres Freund <andres@anarazel.de> — 2024-07-29T17:48:53Z

    Hi,

On 2024-07-29 13:25:22 -0400, Tom Lane wrote:
> Heikki Linnakangas <hlinnaka@iki.fi> writes:
> > Yeah I'm not worried about that at all. Also, the assert is made when 
> > you have already released the spinlock; you are already out of the 
> > critical section.
> 
> Not in the patch Andres posted.

Which seems fairly fundamental - once outside of the critical section, we
can't actually assert that the lock isn't acquired, somebody else *validly*
might have acquired it by then.

However, I still don't think it's a problem to assert that the lock is held in
in the unlock "routine". As mentioned before, the spinlock implementation
itself has never followed the "just straight line code" rule that users of
spinlocks are supposed to follow.

Greetings,

Andres Freund

  13. Re: pgsql: Fix double-release of spinlock

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-29T17:56:05Z

    Andres Freund <andres@anarazel.de> writes:
> On 2024-07-29 12:45:19 -0400, Tom Lane wrote:
>> Hmm, but how?

> ...
> I.e. the version using 2 as the locked state uses a three byte instruction vs
> a two byte instruction before.
> *If* we are worried about this, we could
> a) Change the representation only for assert enabled builds, but that'd have
>    ABI issues again.

Agreed, that would be a very bad idea.  It would for example break the
case of a non-assert-enabled extension used with an assert-enabled
core or vice versa, which is something we've gone out of our way to
allow.

> b) Instead define the spinlock to have 1 as the unlocked state and 0 as the
>    locked state. That makes it a bit harder to understand that initialization
>    is missing, compared to a dedicated state, as the first use of the spinlock
>    just blocks.

This option works for me.

> To make 1) b) easier to understand it might be worth changing the slock_t
> typedef to be something like

> typedef struct slock_t
> {
>         char is_free;
> } slock_t;

+1

How much of this would we change across platforms, and how much
would be x86-only?  I think there are enough people developing on
ARM (e.g. Mac) now to make it worth covering that, but maybe we
don't care so much about anything else.

			regards, tom lane

  14. Re: Detect double-release of spinlock

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-29T17:57:02Z

    Andres Freund <andres@anarazel.de> writes:
> However, I still don't think it's a problem to assert that the lock is held in
> in the unlock "routine". As mentioned before, the spinlock implementation
> itself has never followed the "just straight line code" rule that users of
> spinlocks are supposed to follow.

Yeah, that's fair.

			regards, tom lane

  15. Re: Detect double-release of spinlock

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-07-29T18:00:35Z

    On 29/07/2024 20:48, Andres Freund wrote:
> On 2024-07-29 13:25:22 -0400, Tom Lane wrote:
>> Heikki Linnakangas <hlinnaka@iki.fi> writes:
>>> Yeah I'm not worried about that at all. Also, the assert is made when
>>> you have already released the spinlock; you are already out of the
>>> critical section.
>>
>> Not in the patch Andres posted.
> 
> Which seems fairly fundamental - once outside of the critical section, we
> can't actually assert that the lock isn't acquired, somebody else *validly*
> might have acquired it by then.

You could do:

bool was_free = S_LOCK_FREE(lock);

S_UNLOCK(lock);
Assert(!was_free);

Depending on the underlying implementation, you could also use 
compare-and-exchange. That makes the assertion-enabled instructions a 
little different than without assertions though.

> However, I still don't think it's a problem to assert that the lock is held in
> in the unlock "routine". As mentioned before, the spinlock implementation
> itself has never followed the "just straight line code" rule that users of
> spinlocks are supposed to follow.

Agreed.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  16. Re: Detect double-release of spinlock

    Andres Freund <andres@anarazel.de> — 2024-07-29T18:12:19Z

    Hi,

On 2024-07-29 21:00:35 +0300, Heikki Linnakangas wrote:
> On 29/07/2024 20:48, Andres Freund wrote:
> > On 2024-07-29 13:25:22 -0400, Tom Lane wrote:
> > > Heikki Linnakangas <hlinnaka@iki.fi> writes:
> > > > Yeah I'm not worried about that at all. Also, the assert is made when
> > > > you have already released the spinlock; you are already out of the
> > > > critical section.
> > > 
> > > Not in the patch Andres posted.
> > 
> > Which seems fairly fundamental - once outside of the critical section, we
> > can't actually assert that the lock isn't acquired, somebody else *validly*
> > might have acquired it by then.
> 
> You could do:
> 
> bool was_free = S_LOCK_FREE(lock);
> 
> S_UNLOCK(lock);
> Assert(!was_free);

I don't really see the point - we're about to crash with an assertion failure,
why would we want to do that outside of the critical section? If anything that
will make it harder to debug the issue in a core dump, because other backends
might "destroy evidence" due to being able to acquire the spinlock.


> Depending on the underlying implementation, you could also use
> compare-and-exchange.

That'd scale a lot worse, at least on x86-64, as it requires the unlock to be
an atomic op, whereas today it's a simple store (+ compiler barrier).

I've experimented with replacing all spinlocks with lwlocks, and the fact that
you need an atomic op for an rwlock release is one of the two major reasons
they have a higher overhead (the remainder is boring stuff like the overhead
of external function calls and ownership management).

Greetings,

Andres Freund

  17. Re: Detect double-release of spinlock

    Andres Freund <andres@anarazel.de> — 2024-07-29T18:29:52Z

    Hi,

Partially replying here to an email on -committers [1].

On 2024-07-29 13:57:02 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > However, I still don't think it's a problem to assert that the lock is held in
> > in the unlock "routine". As mentioned before, the spinlock implementation
> > itself has never followed the "just straight line code" rule that users of
> > spinlocks are supposed to follow.
>
> Yeah, that's fair.

Cool.


On 2024-07-29 13:56:05 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > b) Instead define the spinlock to have 1 as the unlocked state and 0 as the
> >    locked state. That makes it a bit harder to understand that initialization
> >    is missing, compared to a dedicated state, as the first use of the spinlock
> >    just blocks.
>
> This option works for me.

> > To make 1) b) easier to understand it might be worth changing the slock_t
> > typedef to be something like
>
> > typedef struct slock_t
> > {
> >         char is_free;
> > } slock_t;
>
> +1

Cool. I've attached a prototype.


I just realized there's a nice little advantage to the "inverted"
representation - it detects missing initialization even in optimized builds.


> How much of this would we change across platforms, and how much
> would be x86-only?  I think there are enough people developing on
> ARM (e.g. Mac) now to make it worth covering that, but maybe we
> don't care so much about anything else.

Not sure. Right now I've only hacked up x86-64 (not even touching i386), but
it shouldn't be hard to change at least some additional platforms.

My current prototype requires S_UNLOCK, S_LOCK_FREE, S_INIT_LOCK to be
implemented for x86-64 instead of using the "generic" implementation. That'd
be mildly annoying duplication if we did so for a few more platforms.


It'd be more palatable to just change all platforms if we made more of them
use __sync_lock_test_and_set (or some other intrinsic(s))...

Greetings,

Andres Freund

[1] https://postgr.es/m/2812376.1722275765%40sss.pgh.pa.us