Skip to content

libselinux: fix build on 32-bit LFS platforms#391728

Merged
numinit merged 1 commit intoNixOS:stagingfrom
mildsunrise:libselinux/fix-build-32lfs
Apr 23, 2025
Merged

libselinux: fix build on 32-bit LFS platforms#391728
numinit merged 1 commit intoNixOS:stagingfrom
mildsunrise:libselinux/fix-build-32lfs

Conversation

@mildsunrise
Copy link
Contributor

@mildsunrise mildsunrise commented Mar 21, 2025

Compilation of libselinux on modern 32-bit platforms has been broken since v3.8. For example:

nix-build --arg crossSystem '{ config="riscv32-unknown-linux-gnu"; }' -A libselinux

A debian maintainer and me prepared a fix in SELinuxProject/selinux#464 (see discussion and first commit), and I'd like to get this patch here while it's reviewed and released upstream (hopefully in some months). The patch is small and I've tested it doesn't break the build on x64 (a 64-bit platform) and x86 (a 32-bit non-LFS platform).

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
  • N/A Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • N/A (Package updates) Added a release notes entry if the change is major or breaking
    • N/A (Module updates) Added a release notes entry if the change is significant
    • N/A (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@NixOSInfra NixOSInfra added the 12.first-time contribution This PR is the author's first one; please be gentle! label Mar 21, 2025
@nix-owners nix-owners bot requested a review from RossComputerGuy March 21, 2025 03:40
@github-actions github-actions bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. labels Mar 21, 2025
@mildsunrise
Copy link
Contributor Author

I've also checked we still get the two symbols in x86

# nm -D /nix/store/4agsd6q8snyzihf1zkn3n7izdmxafycb-libselinux-i686-unknown-linux-gnu-3.8/lib/libselinux.so | grep add
         U ___tls_get_addr@GLIBC_2.3
000091cd T avc_add_callback@@LIBSELINUX_1.0
0001b358 T matchpathcon_filespec_add@@LIBSELINUX_1.0
0001b0fb T matchpathcon_filespec_add64@@LIBSELINUX_3.8
         U readdir64@GLIBC_2.2
0001eadd T selinux_failsafe_context_path@@LIBSELINUX_1.0

as expected this causes a mass rebuild so I'll target staging

@mildsunrise mildsunrise force-pushed the libselinux/fix-build-32lfs branch from adbde4f to 3f447cd Compare March 21, 2025 04:18
@github-actions github-actions bot added 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: kernel The Linux kernel 8.has: changelog This PR adds or changes release notes 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: emacs Text editor 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. 6.topic: ruby A dynamic, open source programming language with a focus on simplicity and productivity. 6.topic: fetch Fetchers (e.g. fetchgit, fetchsvn, ...) 6.topic: stdenv Standard environment 6.topic: nodejs Node.js is a free, open-source, cross-platform JavaScript runtime environment 6.topic: lua Lua is a powerful, efficient, lightweight, embeddable scripting language. 6.topic: systemd Software suite that provides an array of system components for Linux operating systems. 6.topic: java Including JDK, tooling, other languages, other VMs 6.topic: mate The MATE Desktop Environment 6.topic: games Gaming on NixOS labels Mar 21, 2025
@mildsunrise mildsunrise changed the base branch from master to staging March 21, 2025 04:19
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 21, 2025
@github-actions github-actions bot removed 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: kernel The Linux kernel 8.has: changelog This PR adds or changes release notes 8.has: module (update) This PR changes an existing module in `nixos/` labels Mar 21, 2025
@mildsunrise mildsunrise force-pushed the libselinux/fix-build-32lfs branch 2 times, most recently from dc49ec2 to a006460 Compare March 21, 2025 04:33
@mildsunrise mildsunrise force-pushed the libselinux/fix-build-32lfs branch from a006460 to c7acf11 Compare March 21, 2025 04:45
Copy link
Member

@RossComputerGuy RossComputerGuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now, I'll hold off on merging until the upstream PR is closer to being merged in case anything changes.

@wegank wegank added 12.approvals: 1 This PR was reviewed and approved by one person. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. labels Mar 21, 2025
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 7, 2025
see the linked pull request for more details, basically
upstream tried to improve compatibility for platforms
where ino_t defaults to 32-bit, and in the process
broke 32-bit platforms where ino_t defaults to 64-bit.

patch is in upstream review.
@mildsunrise mildsunrise force-pushed the libselinux/fix-build-32lfs branch from c7acf11 to 3bf8021 Compare April 13, 2025 21:32
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 13, 2025
@nix-owners nix-owners bot requested a review from numinit April 13, 2025 21:41
@numinit
Copy link
Contributor

numinit commented Apr 13, 2025

Patch looks fine to me.

@wegank wegank added 12.approvals: 2 This PR was reviewed and approved by two persons. and removed 12.approvals: 1 This PR was reviewed and approved by one person. labels Apr 20, 2025
@mildsunrise
Copy link
Contributor Author

upstream has (to the best of my knowledge) remained silent on this issue for almost 2 months now.
can we fix the build now, and if/once they fix this upstream, update the patch?

@numinit
Copy link
Contributor

numinit commented Apr 21, 2025

That sounds fine to me.

@numinit numinit merged commit 0549489 into NixOS:staging Apr 23, 2025
28 checks passed
@mildsunrise mildsunrise deleted the libselinux/fix-build-32lfs branch April 26, 2025 07:18
@mildsunrise
Copy link
Contributor Author

thanks!!

@alyssais
Copy link
Member

Broke musl :(

@mildsunrise
Copy link
Contributor Author

huh, i am surprised that this even compiled with musl to begin with, I thought __ino_t was a glibc nonportable type... let me see

@alyssais
Copy link
Member

Already submitted a patch upstream :)

@alyssais
Copy link
Member

Please test: #402027

@mildsunrise
Copy link
Contributor Author

thanks for fixing! I think you forgot to add the defined(__INO64_T_TYPE) guard to the header too? (look at the previous hunk in the patch). otherwise it looks correct to me... I'm not sure what's the best way to detect whether the libc participates in the 2038y proofness design or not, but since glibc is the only participant I'm aware of, I guess __INO64_T_TYPE is just as good as any other glibc internal define.

okay it isn't 100% correct, as it would IIUC still fail when building with USE_LFS=n (which would be weird to touch when building with musl anyway, but it might still trip some folks).

but I'm still confused... the code was using __ino_t even before my patch, so it couldn't possibly build outside of glibc, at the very least for 32-bit platforms? am I missing something here? do we only test/care about musl builds with 64-bit platforms?

@alyssais
Copy link
Member

'm not sure what's the best way to detect whether the libc participates in the 2038y proofness design or not, but since glibc is the only participant I'm aware of

It's not that other libcs aren't 2038-proof, it's that only Glibc lets you mix 2038-proof and non-2038-proof types. musl only provides 64-bit types.

do we only test/care about musl builds with 64-bit platforms?

I certainly don't care about 32-bit platforms, and I've never come across anybody else who does for musl in Nixpkgs either, so I think it's just that nobody noticed.

@alyssais
Copy link
Member

I think you forgot to add the defined(__INO64_T_TYPE) guard to the header too? (look at the previous hunk in the patch). otherwise it looks correct to me

I guess I did — surprising to me that it built…

@mildsunrise
Copy link
Contributor Author

It's not that other libcs aren't 2038-proof, it's that only Glibc lets you mix 2038-proof and non-2038-proof types. musl only provides 64-bit types.

I know, y2038 proofness design refers to the whole _TIME_BITS macro system. AFAIK glibc is the only libc that follows this design, the rest fixed the problem by breaking ABI and making ino_t unconditionally 64-bit

I guess I did — surprising to me that it built…

yeah, it does build, just with the symbol renamed to [...]add64 since the define affects both prototype and definition in the .c

I certainly don't care about 32-bit platforms, and I've never come across anybody else who does for musl in Nixpkgs either, so I think it's just that nobody noticed.

ah okay, that expains it then :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 12.approvals: 2 This PR was reviewed and approved by two persons. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. 12.first-time contribution This PR is the author's first one; please be gentle!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants