Hi,

I was looking into performing an upgrade test of libselinux1 with
piuparts and that didn't go well. I spare you the piuparts stuff and go
into crafting a minimal reproducer using mmdebstrap:

mmdebstrap --variant=apt unstable /dev/null "deb http://deb.debian.org/debian unstable main" "deb http://deb.debian.org/debian experimental main" --chrooted-customize-hook="apt-get -y install libselinux1t64"

This looks fairly innocuous. We create a minimal sid chroot and install
libselinux1t64 using apt. What could possibly go wrong? Well, apt thinks
that it would be a good idea to avoid coinstalling breaking packages and
first removes libselinux1 before proceeding to install libselinux1t64.
Unfortunately, libselinux1 is transitively essential and dpkg links it,
so this is what you get:

| Reading package lists... Done
| Building dependency tree... Done
| The following packages will be REMOVED:
|   libselinux1
| The following NEW packages will be installed:
|   libselinux1t64
| 0 upgraded, 1 newly installed, 1 to remove and 0 not upgraded.
| Need to get 75.2 kB of archives.
| After this operation, 4096 B of additional disk space will be used.
| Get:1 http://deb.debian.org/debian experimental/main amd64 libselinux1t64 amd64 3.5-2.1~exp1 [75.2 kB]
| Fetched 75.2 kB in 0s (6067 kB/s)
| debconf: delaying package configuration, since apt-utils is not installed
| dpkg: libselinux1:amd64: dependency problems, but removing anyway as you requested:
|  util-linux depends on libselinux1 (>= 3.1~).
|  tar depends on libselinux1 (>= 3.1~).
|  sed depends on libselinux1 (>= 3.1~).
|  libpam-modules-bin depends on libselinux1 (>= 3.1~).
|  libpam-modules:amd64 depends on libselinux1 (>= 3.1~).
|  libmount1:amd64 depends on libselinux1 (>= 3.1~).
|  findutils depends on libselinux1 (>= 3.1~).
|  dpkg depends on libselinux1 (>= 3.1~).
|  coreutils depends on libselinux1 (>= 3.1~).
|  base-passwd depends on libselinux1 (>= 3.1~).
|
| (Reading database ... 6230 files and directories currently installed.)
| Removing libselinux1:amd64 (3.5-2) ...
| /usr/bin/dpkg: error while loading shared libraries: libselinux.so.1: cannot open shared object file: No such file or directory
| E: Sub-process /usr/bin/dpkg returned an error code (127)

At that point stuff is fairly broken and we cannot easily recover as
both dpkg and tar are now broken. This is pretty bad. To make matters
worse, the situation arises from the combination of Breaks + Provides
and there is nothing libselinux1t64 could do in maintainer scripts to
prevent this from happening, because no libselinux1t64 maintainer script
has been run by the time damage has happened.

I also looked into whether I could reproduce a similar failure with
other packages such as libpam0t64 or libaudit1, but in no other case, I
was able to construct a comparable outcome.

I also looked into why libselinux was being time-bumped. Do I understand
correctly that libselinux is entirely unaffected by time64?
https://adrien.dcln.fr/misc/armhf-time_t/2024-02-01T09:53:00/compat_reports/libselinux1-dev/lfs_to_time_t/compat_report.html
It still is affected by LFS due to using ino_t in the public ABI of
matchpathcon_filespec_add:
https://adrien.dcln.fr/misc/armhf-time_t/2024-02-01T09:53:00/compat_reports/libselinux1-dev/base_to_lfs/compat_report.html
Since we also complete the LFS transition here, not bumping it would
result in an ABI break regarding this symbol. If we were to opt
libselinux out of the LFS transition (e.g. by removing the flags in
debian/rules), then other packages being rebuilt against libselinux-dev
with these flags enabled would be ABI-incompatible though.

An option I see here is to provide ABI-duality for libselinux:

Hi Helmut,

Thanks for identifying and raising this issue.
After Graham mentioned this to me, I also looked at the reports and came
to the same conclusion: the change is actually LFS due to ino_t in
matchpathcon_filespec_add().

Providing two APIs makes me quite uneasy due to having core components
that would behave differently from the rest of the distribution. It
sounds like something that will come back to bite for a long time.

I checked on codesearch.d.n and there are very few users on this API;
actually, none I think. Per
https://codesearch.debian.net/search?q=matchpathcon_filespec_add&literal=1&perpkg=1
- box64 mentions that API but the "code" is commented-out,
- busybox uses it in the "setfiles" applet which isn't built,
- android-platform-external-libselinux has it in headers but also has
  its own implementation

That should hopefully give more freedom although I'm not sure what would
be the preferred route.

Can you elaborate?

Keep in mind that for all the 64bit architectures, there is no abi
difference as the symbol is only added for those architectures, that
currently use a 32bit ino_t.

And here you are arguing that there are no practical users of the added
symbol, so with luck, we'd be adding an unused symbol on armhf. With bad
luck, we'd have some users and for those users we'd be ABI-incompatible
with the rest of the world on armhf.

Helmut

Hi!

Yes, I'm not sure I understand either. This is what symbol versioning
makes possible, even providing different variants for the same symbol,
see for example glibc or libbsd.

In any case, if going the bi-ABI path, I think upstream should get
involved, and the shape of this decided with them. In addition
the library should also be built with LFS by the upstream build
system, which it does not currently, to control its ABI.

I think there are only three ways to go about this, excluding the t64
attempt:

 1) Build the library with LFS unconditionally (except on i386). As there
    are no users in Debian, this would not break there, but would
    break for any external packages and locally unpackaged users of
    the library.

 2) Bump the SONAME, ideally coordinate with upstream or alternatively
    with a Debian specific one. This does not break locally built
    packages nor locally unpackaged code linking against the library.

 3) Build the library with LFS support (everywhere including i386),
    and on systems w/o built-in LFS, make the old symbol use 32-bit ino_t,
    and add a new symbol that uses 64-bit ino_t. This preserves the
    ABI, for external packages and locally unpackaged code linking
    against the library.

I think the three options would cause no upgrade issues, as they
include either no SONAME bump (option 1 and 3), or an actual SONAME
bump (option 2) with no file conflicts involved.

Personally I'd like to be able to cleanly and safely build dpkg with
time64 everywhere (including i386, otherwise the port will not be even
installable), so my preference is for options that make that possible
(2 and 3), and from those the one with best backwards compatibility with
was the main concern for excluding i386 from the time64 transition would
be option 3. So I think that would be the preferred option here.

If you'd like assistance with trying to get a proposal for 3 to
present upstream I could look into that. But I think they should be
involved early on to see what they'd like to see and what they might
outright reject.

Thanks,
Guillem

Hi Guillem,

I think symbol versioning is subtly different and glibc does not use
symbol versioning for e.g. gettimeofday selection. With symbol
versioning, you select a default version at release time and stick to
it. In other words, building against the updated libselinux does not
allow you to use the older 32bit variant of the symbol even if you opt
out of lfs and time64 and you always get the 64bit symbol. What glibc
does is a little more fancy than my simplistic #define in that it uses
asm("name") instead. Still this approach allows for selecting which
symbol is being used via macros (e.g. _FILE_OFFSET_BITS). Please correct
me if I am misrepresenting this as my experience with symbol versioning
is fairly limited.

I agree that involving upstream is a good idea and my understanding is
that someone from Canonical is doing that already, which is why the
schedule was delayed.

My real question here though is what's the downsides of providing two
variants of this symbol (whether with symbol versioning or name
redirection). From my pov, this effectively is your option 3 and what I
sketched is the most stupid implementation of it. My sketch did assume
that libselinux would be built with LFS support everywhere including
i386. Enabling that on the upstream side definitely is even better,
because it gets us to not have a Debian-specific ABI.

Thanks for confirming that I've reported a real problem.
it all depends on what upstream says. If upstream cooperates on any
option, that's better still as we avoid ABI deviation.

Going from here, I also looked a bit into whether we could additionally
use an upstream-cooperating approach for other packages central to
Debian to avoid t64 bumps.

pam seems difficult:
| extern time_t pam_misc_conv_warn_time; /* time that we should warn user */
| extern time_t pam_misc_conv_die_time;         /* cut-off time for input */

We cannot symbol-version these in a reasonable way. All we could do is
ask upstream for a real soname bump. We have a slight advantage here: On
little endian (such as armhf), we can extend this to 64bit and 32bit
accesses will continue to work for small values. However, doing this to
m68k would break horribly. I also couldn't find any in-Debian users of
these symbols (super merely vendors pam source), so just bumping it and
accepting breakage (Guillems option 1) might be worth a go?

For libaudit1, I fail to understand why we bump it at all. Both reports
look fine to me:
https://adrien.dcln.fr/misc/armhf-time_t/2024-02-01T09:53:00/compat_reports/libaudit-dev/base_to_lfs/compat_report.html
https://adrien.dcln.fr/misc/armhf-time_t/2024-02-01T09:53:00/compat_reports/libaudit-dev/lfs_to_time_t/compat_report.html
This does not extend to libauparse0 where the report gives a reason, but
libaudit1 is the one that interacts with /usr-move and libauparse0 not,
so can we skip the dance for libaudit1?

For libtirpc, it is only about rpcb_gettime, which returns time via a
time_t* and can indicate success/failure via return. It seems fairly
simple to implement ABI duality here and libtirpc already does symbol
versioning. Maybe we can also approach upstream about this?

For libfuse2, I think the ABI analysis is broken. The base-to-lfs report
supposedly is ok
https://adrien.dcln.fr/misc/armhf-time_t/2024-02-01T09:53:00/compat_reports/libfuse-dev/base_to_lfs/compat_report.html
and then going lfs-to-time changes ino_t
https://adrien.dcln.fr/misc/armhf-time_t/2024-02-01T09:53:00/compat_reports/libfuse-dev/lfs_to_time_t/compat_report.html
while I would have expected ino_t to change with lfs already.  Are we
sure about this? In any case, this is more of an academic question as
adding ABI-duality would be more involved here. Moreover, I don't see
any ACC report for libfuse3-dev. Did that fail to analyze?

libiw30 only has one affected symbol:
iw_print_timeval ( char* buffer, int buflen, struct timeval const* time, struct timezone const* tz )
Providing ABI duality for this seems doable. Moreover, libiw30 already
has soname 30, so maybe upstream is open to bumping it again? The
resulting library transition is fairly small.

ntfs-3g might be worth a second look. It does use time_t and timeval
quite a bit, but it seems to do so in inline functions from ntfstime.h
and internally use a "typedef sle64 ntfs_time;", so the library might
actually be unaffected and automatically provide ABI-duality via inline
functions! The lfs side looks less bright as e.g. FILEREADER embeds
off_t.

I also looked into a few more libraries affected by both /usr-move and
time64 and figured that none of the others seems worth a deeper look. In
general though approaching upstreams for doing a soname bump to
accommodate lfs+time64 seems like a reasonable thing to ask and maybe
like 10% agree?

Candidates approaching for soname bumps:
 * libeinfo1
 * libgsmme1
 * libiv-unidraw2
 * libparted-fs-resize0
 * libparted2
 * librc1

Also when looking into effort, please keep in mind that for every case
mentioned in this email, we're looking into adding a protective
diversion due to the package rename without so rename and then in forky
we'll have to clean up all those diversions, and in forky+1 we'll have
to delete the cleanup code, so while investing more now may seem more
expensive, it saves later.

Helmut

[...]> This looks fairly innocuous. We create a minimal sid chroot and install
[...]
[...]

Hello,

color me stupid but isn't this fishy?

Package: libselinux1t64
Replaces: libselinux1
Provides: libselinux1 (= 3.5-2.1~exp1)
Breaks: libselinux1 (<< 3.5-2.1~exp1)

Afaiui libselinux1t64 must not fullfill dpkg 1.22.4's dependency on
"libselinux1 (>= 3.1~)". dpkg needs to be rebuilt and the rebuilt
version gets a dep on "libselinux1t64 (>= 3.5)".

Will ${t64:Provides} stop expanding to "libselinux1 = ${binary:Version
for real t64-builds? (The ones in experimental are not.) If that is case
this bug and this way of testing does not make sense.

Otherwise the plan looks flawed.

cu Andreas

Hi Andreas,

The *t64 libraries only break ABI on some architectures. Notably, on all
64bit architectures, i386 and x32, the ABI will not change. On the next
upload after the transition, library dependencies will move to the t64
variants, yes.

No, the t64:Provides will remain that way for all architectures that do
not break ABI. In theory, we could have skipped the rename on those
architectures, but having architecture-dependent package names is
annoyingly hard. Hence, we rename them on e.g. amd64 as well even though
nothing changes there.

Hope this explains

Helmut

07.02.2024 11:06, Helmut Grohne :
..

Attached is a sketch to make pam compatible.

I had a more complete and *tested* fix 2 days ago but I forgot
it was in /tmp and I rebooted the machine, so had to do it again
yesterday.

The idea is to have both die_time and die_time64, and keep them
in sync (using minimum between two values which is !=0).

This is a sketch using a #define, though better is to use symbol
versioning here and have time32 compat stuff for old programs
and 64bit time stuff for new, using redirection at the link time,
instead of the #defines which makes whole thing rather difficult
to read, - that's extra several lines of code, also to the .map
file.

What the whole thing needs is the criteria to use to enable the
trick.  Right now I used #ifdef NEED_TIME64_COMPAT which should
be defined somehow, - since I don't know the precise list of
architectures where this has to be done.  This is an externally-
controlled thing, there's no way to determine this directly from
the .c code (short of using architecture list in the .h file),
so it must be some symbol substituted at the package build time,
like Provides: t64:Compat (or whatever it is) is substituted in
d/control.

This is a less-intrusve-to-original-code version of the sketch,
ie, I tried to keep all changes outside of the original code as
possible, keeping all the original logic as it is.

/mjt

06.02.2024 12:34, Helmut Grohne:
...

It's good for a sketch to show an idea but it wont work in practice, -
you can't use sizeof(foo) in a preprocessor condition.  That's what
WORDSIZE #defines are for.  But it's a minor nit.

glibc already has all the support for LFS which can be used directly,
by copying code from any glibc header, like eg for lseek definition...
and keeping this #define here instead of using internal in-glibc
symbol redirection stuff.

And ofc we need to define the compat wrapper for matchpathcon_filespec_add
to the source, and the new 64bit symbol to libselinux.map, with the same
arch-specific condition.

/mjt

    Helmut> pam seems difficult: | extern time_t
    Helmut> pam_misc_conv_warn_time; /* time that we should warn user */
    Helmut> | extern time_t pam_misc_conv_die_time; /* cut-off time for
    Helmut> input */

    Helmut> We cannot symbol-version these in a reasonable way. All we
    Helmut> could do is ask upstream for a real soname bump. We have a
    Helmut> slight advantage here: On little endian (such as armhf), we
    Helmut> can extend this to 64bit and 32bit accesses will continue to
    Helmut> work for small values. However, doing this to m68k would
    Helmut> break horribly. I also couldn't find any in-Debian users of
    Helmut> these symbols (super merely vendors pam source), so just
    Helmut> bumping it and accepting breakage (Guillems option 1) might
    Helmut> be worth a go?

Steve and I are unaware of usage in Debian either.

Agreed.  libselinux as it happens does use a symbol version map so there is
symbol versioning involved in some sense? but not the sense you really mean.

(We could make the symbol map expose the two different function variants
under the same name but different symbols; that's fine but I'll leave that
for upstream to decide.)

Well, "already" is not exactly correct, but the need to resolve this
critical showstopper bug in libselinux before making changes to the
toolchain behavior in unstable and breaking the world has affected the
timeline, yes.

I now have a tested patch that I've raised as an MP in salsa:

https://salsa.debian.org/selinux-team/libselinux/-/merge_requests/9

I welcome review from the Debian libselinux maintainers prior to opening a
discussion with upstream.  (Which I will plan to do sometime Thursday
US/Pacific)

Patch now forwarded upstream for review.

https://lore.kernel.org/selinux/Zc6tzKPsYZRICHya@homer.dodds.net/T/#t

Hi!

Thanks for preparing the patch. I checked it and left a comment on the
MR there.

Regards,
Guillem

Hi,

Paul asked me to follow up on these rc bugs.

I am closing #1063135, because we will not move forward with a rename to
libselinux1t64. The package has been removed from experimental
meanwhile, so this is fully done.

We have addressed the package upgrade by not moving from libselinux1 to
libselinux1t64, but that means libselinux now breaks ABI of
matchpathcon_filespec_add. To that end, Steve really did fix libselinux
upstream.

This has been merged and is available upstream.
https://github.com/SELinuxProject/selinux/commit/9395cc03226a0e1a220a37d71d1a4158635c4284

The change is not in the latest libselinux upload.
https://sources.debian.org/src/libselinux/3.7-3/include/selinux/selinux.h/

As a result, unstable presently has broken ABI and applying the patch
(that has been accepted upstream) will revert the ABI breakage. Beware
that after applying the patch, we must schedule binNMUs for reverse
dependencies on armel and armhf (+ multiarch sync) as a symbol is being
redirected. Otherwise, we have reverse ABI breakage due to the earlier
breakage having been picked up.

So no, this is not done, but it's not much left to be done.

Helmut

Hi,


But it now is in unstable and testing as far as I checked.

Do I understand correctly that we still need to binNMU those binaries
that were built with the broken version?

Paul

control: tags -1 fixed-upstream,help

Hi,

as discussed in
https://salsa.debian.org/selinux-team/libselinux/-/merge_requests/9
the ABI breakage has benn resolved since libselinux version 3.8-1
(present in Debian stable).

Is there anything left to do here, e.g. as suggested in
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1063329#89 by Paul a
rebuild of binaries built against a broken version?
I am not familiar with binNMUs or rebuild so any suggestions and help
is welcome.