pacemaker fails to release clustered filesystem dlm locks on failover

It appears the linker is eliding the section that libqb's "linker magic" expects when the linker detects that the compiled program doesn't actually use that section, which seems completely reasonable for the linker to do. This seems to be a case of libqb trying to be "clever" by relying on an implementation detail of the linker, which is bad.

A slightly adjusted test program, to actually use the libqb log functions, does not reproduce the problem, e.g.:

#include <qb/qblog.h>

QB_LOG_INIT_DATA(test);

int main(int argc, char* argv[])
{
  qb_log_init("test", LOG_USER, LOG_INFO);

  qb_log(LOG_ERR, "test\n");

  qb_log_fini();

  return 0;
}

Compiling that with or without -DQB_KILL_ATTRIBUTE_SECTION results in the message "test" logged to syslog when run, so it appears safe to compile pacemaker (and any other programs using libqb that show this problem) with that define, to work around this issue.

I have checked it again on Groovy and looks like change from https://github.com/ClusterLabs/libqb/pull/322 did not make it to Groovy version of libqb. Also in a test the behavior was identical to Focal, so I have targetted the bug to the series.

Adding -DQB_KILL_ATTRIBUTE_SECTION to CFLAGS seems to result in some symbols disappearing during the build:
https://paste.ubuntu.com/p/hmBpMXGjqy/

The list of symbols missing seem to be consistent with the onces defined with the CRM_TRACE_INIT_DATA macro:
lib/lrmd/lrmd_client.c
46:CRM_TRACE_INIT_DATA(lrmd);

lib/pacemaker/pcmk_trans_unpack.c
20:CRM_TRACE_INIT_DATA(transitioner);

lib/fencing/st_client.c
37:CRM_TRACE_INIT_DATA(stonith);

lib/pacemaker/pcmk_sched_allocate.c
24:CRM_TRACE_INIT_DATA(pe_allocate);

lib/cluster/cluster.c
25:CRM_TRACE_INIT_DATA(cluster);

lib/common/utils.c
57:CRM_TRACE_INIT_DATA(common);

lib/pengine/unpack.c
28:CRM_TRACE_INIT_DATA(pe_status);

lib/pengine/rules.c
25:CRM_TRACE_INIT_DATA(pe_rules);

The macro itself is defined in the following way:
include/crm/common/logging.h
112: #define CRM_TRACE_INIT_DATA(name) QB_LOG_INIT_DATA(name)

On the other hand QB_LOG_INIT_DATA is defined in libqb as follows:
#if defined(QB_KILL_ATTRIBUTE_SECTION) || defined(S_SPLINT_S)
#undef QB_HAVE_ATTRIBUTE_SECTION
#endif /* defined(QB_KILL_ATTRIBUTE_SECTION) || defined(S_SPLINT_S) */

#ifdef QB_HAVE_ATTRIBUTE_SECTION
// ...
#else
#define QB_LOG_INIT_DATA(name)
#endif /* QB_HAVE_ATTRIBUTE_SECTION */

So in the end with the QB_KILL_ATTRIBUTE_SECTION defined the macro QB_LOG_INIT_DATA is being left empty. Hence the missing symbols.

The symbols defined with CRM_TRACE_INIT_DATA doesn't seem to be used anywhere inside pacemaker and it's less than likely those are used anywhere outside of it.

The definitions seem to be strictly logging related without any other functionality declared.

I'm pasting an email answer here so we keep track of all that has been discussed for this issue:

----

> Me and Dan have been working recently on a customer case reported as LP: #1915828. Turns out that some linker "magic" used inside libqb broke pacemaker (and potentially any package using QB_LOG_INIT_DATA from the library or even every packaged linked against libqb).

I have checked libqb commits and: https://paste.ubuntu.com/p/vTdYJdC4Hc/ . I see that upstream added some options in configure.ac related to the linking issue, even quoting debian/ubuntu option to override libtool variable by force. There is also an existing test for the __attribute__((section("__verbose"))) breakage detection now (gcc_has_attribute_section_visible variable in configure.ac.

> Dan found an ABI-compatible way of mitigating the problem by rebuilding pacemaker with the QB_KILL_ATTRIBUTE_SECTION defined. This strips the magic and makes pacemaker useful again.

I see the mitigation (QB_KILL_ATTRIBUTE_SECTION) comes from:

--
32555d8 tests: add a script to generate callsite-heavy logging client…
 ...so as to evaluate use of resources. In particular, the intention
 here is to uncover the observable differences between the same logging
 code built with callsite section (default when available) and
 purposefully (overriding that default by force) without it.
...
--

And they even added “tests/functional/log_callsite_bench_gen.py” to measure the impact of this mitigation.

I’m particularly worried with:

—
    Based on the above, we can conclude that leveraging the callsite
    section for logging as facilitated by the toolchain intrinsics is
    beneficial, especially for performance-critical applications (corosync
    being the showcase here). Therefore it's desired to struggle for
    retaining this nifty trick despite some troubles emerged with recent
    binutils releases (starting with 2.29) and the changed behaviour we
    relied on so far in respective ld.bfd linkers (as mentioned in
    preceding commits). That motive is immediately followed -- well,
    judging the impact fairly, actually outclassed -- with the intention
    to preserve binary compatibility (incl. continuous library support for
    callsite section offloading spread in the existing client space widely
    for quite some years already) to the utmost extent possible.
--

I believe this will be accepted by the SRU team but this, for sure, has to be mentioned in the public bug. I would add to [regression potential] the fact that the logging mechanism would stir heap more often (commit log has even a time execution delta).

Shouldn’t this bug also affect all the libqb0 rdepends ? I can see pacemaker, sbd, corosync and usbguard source packages.

> The problem is (more details in comments #3-#5) some symbols disappear from the package. Those symbols doesn't seem to be used anywhere explicitly, but we were wondering if it's ok to just drop those symbols or maybe to implement a change in libqb to create dummy constructors (e.g. https://pastebin.canonical.com/p/Y4fk747YfK/) to ensure the symbols are available just in case.

For the pacemaker fix I’ll let the SRU team to discuss whether they would ...

SRU proposal for focal

SRU proposal for groovy

Hello Jim, or anyone else affected,

Accepted pacemaker into groovy-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/pacemaker/2.0.4-2ubuntu3.2 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-groovy to verification-done-groovy. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-groovy. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

Hello Jim, or anyone else affected,

Accepted pacemaker into focal-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/pacemaker/2.0.3-3ubuntu4.2 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-focal to verification-done-focal. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-focal. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

All autopkgtests for the newly accepted pacemaker (2.0.3-3ubuntu4.2) for focal have finished running.
The following regressions have been reported in tests triggered by the package:

pacemaker/2.0.3-3ubuntu4.2 (ppc64el)

Please visit the excuses page listed below and investigate the failures, proceeding afterwards as per the StableReleaseUpdates policy regarding autopkgtest regressions [1].

https://people.canonical.com/~ubuntu-archive/proposed-migration/focal/update_excuses.html#pacemaker

[1] https://wiki.ubuntu.com/StableReleaseUpdates#Autopkgtest_Regressions

Thank you!

# verification focal

$ apt-cache policy libcrmcommon34 | grep Installed
  Installed: 2.0.3-3ubuntu4.1

# dlm_stonith -t 5 -n 1
dlm_stonith: utils.c:57: common: Assertion `"implicit callsite section is observable, otherwise target's and/or libqb's build is at fault, preventing reliable logging" && work_s1 != NULL && work_s2 != NULL' failed.
Aborted (core dumped)

$ apt-cache policy libcrmcommon34 | grep Installed
  Installed: 2.0.3-3ubuntu4.2

# dlm_stonith -t 5 -n 1
kick_helper error -79 nodeid 1

# verification groovy

$ apt-cache policy libcrmcommon34 | grep Installed
  Installed: 2.0.4-2ubuntu3.1

# dlm_stonith -t 5 -n 1089
dlm_stonith: utils.c:48: common: Assertion `"implicit callsite section is observable, otherwise target's and/or libqb's build is at fault, preventing reliable logging" && work_s1 != NULL && work_s2 != NULL' failed.
Aborted (core dumped)

$ apt-cache policy libcrmcommon34 | grep Installed
  Installed: 2.0.4-2ubuntu3.2

# dlm_stonith -t 5 -n 1089
kick_helper error -107 nodeid 1089

The verification of the Stable Release Update for pacemaker has completed successfully and the package is now being released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

This bug was fixed in the package pacemaker - 2.0.4-2ubuntu3.2

---------------
pacemaker (2.0.4-2ubuntu3.2) groovy; urgency=medium

  * d/rules: Rebuild with QB_KILL_ATTRIBUTE_SECTION to overcome a problem in
    libqb (LP: #1915828)

 -- Dariusz Gadomski <email address hidden> Thu, 04 Mar 2021 10:52:40 +0100

This bug was fixed in the package pacemaker - 2.0.3-3ubuntu4.2

---------------
pacemaker (2.0.3-3ubuntu4.2) focal; urgency=medium

  * d/rules: Rebuild with QB_KILL_ATTRIBUTE_SECTION to overcome a problem in
    libqb (LP: #1915828)

 -- Dariusz Gadomski <email address hidden> Thu, 04 Mar 2021 10:29:48 +0100