Horton

Thread

Commits

  1. Use WaitEventSet API for postmaster's event loop.

  2. On NetBSD, force dynamic symbol resolution at postmaster start.

  1. Postmaster self-deadlock due to PLT linkage resolution

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-29T19:43:55Z

    Buildfarm member mamba (NetBSD-current on prairiedog's former hardware)
has failed repeatedly since I set it up.  I have now run the cause of
that to ground [1], and here's what's happening: if the postmaster
receives a signal just before it first waits at the select() in
ServerLoop, it can self-deadlock.  During the postmaster's first use of
select(), the dynamic loader needs to resolve the PLT branch table entry
that the core executable uses to reach select() in libc.so, and it locks
the loader's internal data structures while doing that.  If we enter
a signal handler while the lock is held, and the handler needs to do
anything that also requires the lock, the postmaster is frozen.

The probability of this happening seems remarkably small, since there's
only one narrow window per postmaster lifetime, and there's just not
that many potential signal causes active at that time either.
Nonetheless I have traces showing it happening (1) because we receive
SIGCHLD for startup process termination and (2) because we receive
SIGUSR1 from the startup process telling us to start walreceivers.
I guess that mamba's slow single-CPU hardware interacts with the
NetBSD scheduler in just the right way to make it more probable than
you'd think.  On typical modern machines, the postmaster would almost
certainly manage to wait before the startup process is able to signal
it.  Still, "almost certainly" is not "certainly".

The attached patch seems to fix the problem, by forcing resolution of
the PLT link before we unblock signals.  It depends on the assumption
that another select() call appearing within postmaster.c will share
the same PLT link, which seems pretty safe.

I'd originally intended to make this code "#ifdef __NetBSD__",
but on looking into the FreeBSD sources I find much the same locking
logic in their dynamic loader, and now I'm wondering if such behavior
isn't pretty standard.  The added calls should have negligible cost,
so it doesn't seem unreasonable to do them everywhere.

(Of course, a much better answer is to get out of the business of
doing nontrivial stuff in signal handlers.  But even if we get that
done soon, we'd surely not back-patch it.)

Thoughts?

			regards, tom lane

[1] https://gnats.netbsd.org/56979

  2. Re: Postmaster self-deadlock due to PLT linkage resolution

    Noah Misch <noah@leadboat.com> — 2022-08-30T02:42:01Z

    On Mon, Aug 29, 2022 at 03:43:55PM -0400, Tom Lane wrote:
> I'd originally intended to make this code "#ifdef __NetBSD__",
> but on looking into the FreeBSD sources I find much the same locking
> logic in their dynamic loader, and now I'm wondering if such behavior
> isn't pretty standard.

I doubt it's standard.  POSIX specifies select() to be async-signal-safe.
This NetBSD bug makes select() not be async-signal-safe.

> The added calls should have negligible cost,
> so it doesn't seem unreasonable to do them everywhere.

Agreed.  I would make the comment mention the NetBSD version that prompted
this, so we have a better chance of removing the workaround in a few decades.

  3. Re: Postmaster self-deadlock due to PLT linkage resolution

    Thomas Munro <thomas.munro@gmail.com> — 2022-08-30T12:16:36Z

    On Tue, Aug 30, 2022 at 7:44 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Buildfarm member mamba (NetBSD-current on prairiedog's former hardware)
> has failed repeatedly since I set it up.  I have now run the cause of
> that to ground [1], and here's what's happening: if the postmaster
> receives a signal just before it first waits at the select() in
> ServerLoop, it can self-deadlock.  During the postmaster's first use of
> select(), the dynamic loader needs to resolve the PLT branch table entry
> that the core executable uses to reach select() in libc.so, and it locks
> the loader's internal data structures while doing that.  If we enter
> a signal handler while the lock is held, and the handler needs to do
> anything that also requires the lock, the postmaster is frozen.

. o O ( pselect() wouldn't have this problem, but it's slightly too
new for the back branches that didn't yet require SUSv3... drat )

> I'd originally intended to make this code "#ifdef __NetBSD__",
> but on looking into the FreeBSD sources I find much the same locking
> logic in their dynamic loader, and now I'm wondering if such behavior
> isn't pretty standard.  The added calls should have negligible cost,
> so it doesn't seem unreasonable to do them everywhere.

FWIW I suspect FreeBSD can't break like this in a program linked with
libthr, because it has a scheme for deferring signals while the
runtime linker holds locks.  _rtld_bind calls _thr_rtld_rlock_acquire,
which uses the THR_CRITICAL_ENTER mechanism to cause thr_sighandler to
defer until release.  For a non-thread program, I'm not entirely sure,
but I don't think the fork() problem exists there.  (Could be wrong,
based on a quick look.)

> (Of course, a much better answer is to get out of the business of
> doing nontrivial stuff in signal handlers.  But even if we get that
> done soon, we'd surely not back-patch it.)

+1

  4. Re: Postmaster self-deadlock due to PLT linkage resolution

    Robert Haas <robertmhaas@gmail.com> — 2022-08-30T12:26:27Z

    On Tue, Aug 30, 2022 at 8:17 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> FWIW I suspect FreeBSD can't break like this in a program linked with
> libthr, because it has a scheme for deferring signals while the
> runtime linker holds locks.  _rtld_bind calls _thr_rtld_rlock_acquire,
> which uses the THR_CRITICAL_ENTER mechanism to cause thr_sighandler to
> defer until release.  For a non-thread program, I'm not entirely sure,
> but I don't think the fork() problem exists there.  (Could be wrong,
> based on a quick look.)

Well that seems a bit ironic, considering that Tom has worried in the
past that linking with threading libraries would break stuff.

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

  5. Re: Postmaster self-deadlock due to PLT linkage resolution

    Thomas Munro <thomas.munro@gmail.com> — 2022-08-30T13:34:55Z

    On Wed, Aug 31, 2022 at 12:26 AM Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Aug 30, 2022 at 8:17 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> > FWIW I suspect FreeBSD can't break like this in a program linked with
> > libthr, because it has a scheme for deferring signals while the
> > runtime linker holds locks.  _rtld_bind calls _thr_rtld_rlock_acquire,
> > which uses the THR_CRITICAL_ENTER mechanism to cause thr_sighandler to
> > defer until release.  For a non-thread program, I'm not entirely sure,
> > but I don't think the fork() problem exists there.  (Could be wrong,
> > based on a quick look.)
>
> Well that seems a bit ironic, considering that Tom has worried in the
> past that linking with threading libraries would break stuff.

Hah.  To clarify, non-thread builds don't have that exact fork()
problem, but it turns out they do have a related state clobbering
problem elsewhere, which I've reported.

  6. Re: Postmaster self-deadlock due to PLT linkage resolution

    Andres Freund <andres@anarazel.de> — 2022-08-30T17:17:20Z

    Hi,

On 2022-08-29 15:43:55 -0400, Tom Lane wrote:
> Buildfarm member mamba (NetBSD-current on prairiedog's former hardware)
> has failed repeatedly since I set it up.  I have now run the cause of
> that to ground [1], and here's what's happening: if the postmaster
> receives a signal just before it first waits at the select() in
> ServerLoop, it can self-deadlock.  During the postmaster's first use of
> select(), the dynamic loader needs to resolve the PLT branch table entry
> that the core executable uses to reach select() in libc.so, and it locks
> the loader's internal data structures while doing that.  If we enter
> a signal handler while the lock is held, and the handler needs to do
> anything that also requires the lock, the postmaster is frozen.

Ick.


> The attached patch seems to fix the problem, by forcing resolution of
> the PLT link before we unblock signals.  It depends on the assumption
> that another select() call appearing within postmaster.c will share
> the same PLT link, which seems pretty safe.

Hm, what stops the same problem from occuring with other functions?

Perhaps it'd be saner to default to building with -Wl,-z,now? That should fix
the problem too, right (and if we combine it with relro, it'd be a security
improvement to boot).

Greetings,

Andres Freund

  7. Re: Postmaster self-deadlock due to PLT linkage resolution

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-30T17:24:39Z

    Andres Freund <andres@anarazel.de> writes:
> On 2022-08-29 15:43:55 -0400, Tom Lane wrote:
>> The attached patch seems to fix the problem, by forcing resolution of
>> the PLT link before we unblock signals.  It depends on the assumption
>> that another select() call appearing within postmaster.c will share
>> the same PLT link, which seems pretty safe.

> Hm, what stops the same problem from occuring with other functions?

These few lines are the only part of the postmaster that runs with
signals enabled and unblocked.

> Perhaps it'd be saner to default to building with -Wl,-z,now? That should fix
> the problem too, right (and if we combine it with relro, it'd be a security
> improvement to boot).

Hm.  Not sure if that works on NetBSD, but I'll check it out.

			regards, tom lane

  8. Re: Postmaster self-deadlock due to PLT linkage resolution

    Andres Freund <andres@anarazel.de> — 2022-08-30T17:41:16Z

    Hi,

On 2022-08-30 13:24:39 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Perhaps it'd be saner to default to building with -Wl,-z,now? That should fix
> > the problem too, right (and if we combine it with relro, it'd be a security
> > improvement to boot).
> 
> Hm.  Not sure if that works on NetBSD, but I'll check it out.

FWIW, it's a decently (well over 10 years) old thing I think. And it's documented in
the netbsd ld manpage and their packaging guide (albeit indirectly, with their
tooling doing the work of specifying the flags):
https://www.netbsd.org/docs/pkgsrc/hardening.html#hardening.audit.relrofull

Greetings,

Andres Freund

  9. Re: Postmaster self-deadlock due to PLT linkage resolution

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-30T18:07:41Z

    Andres Freund <andres@anarazel.de> writes:
> On 2022-08-30 13:24:39 -0400, Tom Lane wrote:
>> Andres Freund <andres@anarazel.de> writes:
>>> Perhaps it'd be saner to default to building with -Wl,-z,now? That should fix
>>> the problem too, right (and if we combine it with relro, it'd be a security
>>> improvement to boot).

>> Hm.  Not sure if that works on NetBSD, but I'll check it out.

> FWIW, it's a decently (well over 10 years) old thing I think. And it's documented in
> the netbsd ld manpage and their packaging guide (albeit indirectly, with their
> tooling doing the work of specifying the flags):
> https://www.netbsd.org/docs/pkgsrc/hardening.html#hardening.audit.relrofull

It does appear that they use GNU ld, and I've just finished confirming
that each of those switches has the expected effects on my PPC box.
So yeah, this looks like a better answer.

Do we want to install this just for NetBSD, or more widely?
I think we'd better back-patch it for NetBSD, so I'm inclined
to be conservative about the change.

			regards, tom lane

  10. Re: Postmaster self-deadlock due to PLT linkage resolution

    Andres Freund <andres@anarazel.de> — 2022-08-30T18:20:00Z

    Hi,

On 2022-08-30 14:07:41 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2022-08-30 13:24:39 -0400, Tom Lane wrote:
> >> Andres Freund <andres@anarazel.de> writes:
> >>> Perhaps it'd be saner to default to building with -Wl,-z,now? That should fix
> >>> the problem too, right (and if we combine it with relro, it'd be a security
> >>> improvement to boot).
> 
> >> Hm.  Not sure if that works on NetBSD, but I'll check it out.
> 
> > FWIW, it's a decently (well over 10 years) old thing I think. And it's documented in
> > the netbsd ld manpage and their packaging guide (albeit indirectly, with their
> > tooling doing the work of specifying the flags):
> > https://www.netbsd.org/docs/pkgsrc/hardening.html#hardening.audit.relrofull
> 
> It does appear that they use GNU ld, and I've just finished confirming
> that each of those switches has the expected effects on my PPC box.
> So yeah, this looks like a better answer.

Cool.


> Do we want to install this just for NetBSD, or more widely?
> I think we'd better back-patch it for NetBSD, so I'm inclined
> to be conservative about the change.

It's likely a good idea to enable it everywhere applicable, but I agree that
we shouldn't unnecessarily do so in the backbranches. So I'd be inclined to
add it to the netbsd template for the backbranches.

For HEAD I can see putting it into all the applicable templates, adding an
AC_LINK_IFELSE() test, or just putting it into the meson stuff.

Greetings,

Andres Freund

  11. Re: Postmaster self-deadlock due to PLT linkage resolution

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-30T18:32:26Z

    Andres Freund <andres@anarazel.de> writes:
> On 2022-08-30 14:07:41 -0400, Tom Lane wrote:
>> Do we want to install this just for NetBSD, or more widely?
>> I think we'd better back-patch it for NetBSD, so I'm inclined
>> to be conservative about the change.

> It's likely a good idea to enable it everywhere applicable, but I agree that
> we shouldn't unnecessarily do so in the backbranches. So I'd be inclined to
> add it to the netbsd template for the backbranches.

> For HEAD I can see putting it into all the applicable templates, adding an
> AC_LINK_IFELSE() test, or just putting it into the meson stuff.

For the moment I'll stick it into the netbsd template.  I'm not on
board with having the meson stuff generating different executables
than the Makefiles do, so if someone wants to propose applying
this widely, they'll need to fix both.  Seems like that is a good
thing to consider after the meson patches land.  We don't need
unnecessary churn in that area before that.

			regards, tom lane

  12. Re: Postmaster self-deadlock due to PLT linkage resolution

    Andres Freund <andres@anarazel.de> — 2022-08-30T18:46:32Z

    Hi,

On 2022-08-30 14:32:26 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2022-08-30 14:07:41 -0400, Tom Lane wrote:
> >> Do we want to install this just for NetBSD, or more widely?
> >> I think we'd better back-patch it for NetBSD, so I'm inclined
> >> to be conservative about the change.
> 
> > It's likely a good idea to enable it everywhere applicable, but I agree that
> > we shouldn't unnecessarily do so in the backbranches. So I'd be inclined to
> > add it to the netbsd template for the backbranches.
> 
> > For HEAD I can see putting it into all the applicable templates, adding an
> > AC_LINK_IFELSE() test, or just putting it into the meson stuff.
> 
> For the moment I'll stick it into the netbsd template.

Cool.


> I'm not on board with having the meson stuff generating different
> executables than the Makefiles do, so if someone wants to propose applying
> this widely, they'll need to fix both.  Seems like that is a good thing to
> consider after the meson patches land.  We don't need unnecessary churn in
> that area before that.

Yea, I didn't like that idea either, hence listing it last...

Greetings,

Andres Freund

  13. Re: Postmaster self-deadlock due to PLT linkage resolution

    Thomas Munro <thomas.munro@gmail.com> — 2022-09-05T02:28:48Z

    On Wed, Aug 31, 2022 at 1:34 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> On Wed, Aug 31, 2022 at 12:26 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > On Tue, Aug 30, 2022 at 8:17 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> > > FWIW I suspect FreeBSD can't break like this in a program linked with
> > > libthr, because it has a scheme for deferring signals while the
> > > runtime linker holds locks.  _rtld_bind calls _thr_rtld_rlock_acquire,
> > > which uses the THR_CRITICAL_ENTER mechanism to cause thr_sighandler to
> > > defer until release.  For a non-thread program, I'm not entirely sure,
> > > but I don't think the fork() problem exists there.  (Could be wrong,
> > > based on a quick look.)
> >
> > Well that seems a bit ironic, considering that Tom has worried in the
> > past that linking with threading libraries would break stuff.
>
> Hah.  To clarify, non-thread builds don't have that exact fork()
> problem, but it turns out they do have a related state clobbering
> problem elsewhere, which I've reported.

For the record, reporting that resulted in a change for non-libthr rtld:

https://cgit.freebsd.org/src/commit/?id=a687683b997c5805ecd6d8278798b7ef00d9908f

  14. Re: Postmaster self-deadlock due to PLT linkage resolution

    Thomas Munro <thomas.munro@gmail.com> — 2023-08-07T04:49:00Z

    After commit 7389aad6, I think commit 8acd8f86's linker changes (+
meson.build's equivalent) must now be redundant?