Bugfix: Critical sysuser home directory creation failure with BTRFS_SUBVOLUME_HOME=yes on non-Btrfs paths#1609
Bugfix: Critical sysuser home directory creation failure with BTRFS_SUBVOLUME_HOME=yes on non-Btrfs paths#1609silverhadch wants to merge 3 commits intoshadow-maint:masterfrom
Conversation
|
CC: @Conan-Kudo |
|
The commit message needs to be reformatted so that it's 70 chars for the title and the rest of the information needs to be in the commit body (wrapped at 79 chars). |
silverhadch
force-pushed
the
master
branch
2 times, most recently
from
29f0d3f to
f7ccf92
Compare
silverhadch
changed the title
silverhadch
changed the title
silverhadch
force-pushed
the
master
branch
3 times, most recently
from
e5d68a8 to
aaa34da
Compare
ikerexxe
left a comment
ikerexxe
left a comment
There was a problem hiding this comment.
@alejandro-colomar I noticed something potentially confusing in this PR that I wanted your thoughts on. When I see if (is_btrfs(path)), I naturally expect boolean semantics where any non-zero value means "true". But here, -1 means "error" and should be handled differently than 1 (success).
This PR correctly handles it with if (is_btrfs(btrfs_check) > 0), but the function name makes this non-obvious. A future developer might write if (is_btrfs(path)) and accidentally treat errors as "is BTRFS".
Should we consider renaming to something like btrfs_check_filesystem() or get_btrfs_status()?
btrfs_check_filesystem() sounds about right. I could rename it in this PR. |
Agree. It seems confusing.
I like the first suggestion. It should return 0 if it's btrfs, and non-zero (not success) if not. |
silverhadch
force-pushed
the
master
branch
3 times, most recently
from
32303ae to
be35017
Compare
| <replaceable>LOGIN</replaceable> name to | ||
| <replaceable>BASE_DIR</replaceable> and use that as the | ||
| login directory name. | ||
| login directory name. |
There was a problem hiding this comment.
For the second commit:
Reviewed-by: Alejandro Colomar <alx@kernel.org>
There was a problem hiding this comment.
This was only for the second commit (now third).
Commits 1 and 2 are not reviewed. Please remove the Reviewed-by tags from them.
When the --btrfs-subvolume-home option is used but the parent directory is not on a BTRFS filesystem, useradd previously failed with an error. This is too strict; instead, fall back to creating a regular directory and issue a warning. The subvolume creation is attempted only when the parent is BTRFS. Otherwise, a regular directory is created and a syslog warning is logged. Fixes: 3e8c105 (2026-01-02; "src/useradd: Support config for creating home dirs as Btrfs subvolumes") Signed-off-by: Hadi Chokr <hadichokr@icloud.com> Reviewed-by: Alejandro Colomar <alx@kernel.org>
The old return value (1=btrfs, 0=not, -1=err) conflated false and error. New convention: 0 – path is on a Btrfs filesystem 1 – path is not on Btrfs (statfs succeeded) -1 – error (statfs failed) Update prototypes.h and all call sites. Signed-off-by: Hadi Chokr <hadichokr@icloud.com> Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Hadi Chokr <hadichokr@icloud.com> Reviewed-by: Alejandro Colomar <alx@kernel.org>
| ret = is_btrfs(path); | ||
| if (ret <= 0) | ||
| return ret; | ||
| ret = btrfs_check_filesystem(path); | ||
| if (ret == -1) | ||
| return -1; | ||
| if (ret != 0) | ||
| return 0; |
There was a problem hiding this comment.
I would prefer the refacctor to happen before the behavior change; that is, reorder commits 1 and 2. Would that be possible?
4 participants
This fixes a critical bug. Previously, Btrfs subvolume creation was only applied on-demand, but now it's forced for all users whenever BTRFS_SUBVOLUME_HOME=yes is set. That logic is too aggressive: it causes system users with home directories in places like /var or /run to fail because those paths aren't Btrfs-backed. KDE Linux fails to boot with Btrfs home enabled because sysuser configurations require a home directory on tmpfs, yet the option forces a Btrfs subvolume even for those state directories. Without this patch, sysusers aren't created properly, as seen in this failing pipeline: https://invent.kde.org/kde-linux/kde-linux/-/pipelines/1215866. With the patch, the system falls back to a regular directory when a Btrfs home is requested by config or flag but the target path isn't Btrfs-backed, and it throws a warning to stderr. That allows sysusers on tmpfs or other non-Btrfs state directories to be created successfully, and the pipeline passes: https://invent.kde.org/kde-linux/kde-linux/-/pipelines/1216261. This resolves the boot failure, fixes Plasma login and setup, and generally prevents breakage for any sysuser with a home on a non-Btrfs filesystem with said option set.