Skip to content

useradd: skip btrfs subvolume creation for system users#1619

Open
silverhadch wants to merge 3 commits into
shadow-maint:masterfrom
silverhadch:work/hadi/uid-guard-btrfs
Open

useradd: skip btrfs subvolume creation for system users#1619
silverhadch wants to merge 3 commits into
shadow-maint:masterfrom
silverhadch:work/hadi/uid-guard-btrfs

Conversation

@silverhadch

Copy link
Copy Markdown
Contributor

Gate subvolume creation on user_id >= UID_MIN to exclude system users regardless of their home path.

Fixes: c1d36a8 (2019-05-04; "Add support for btrfs subvolumes for user homes")

@alejandro-colomar

Copy link
Copy Markdown
Collaborator

The code seems good to me.

Do we need to document that BTRFS configuration is ignored for system users?

Also, I think the commit message should document why we don't want BTRFS subvolumes for system users.

@silverhadch

Copy link
Copy Markdown
Contributor Author

The code seems good to me.

Do we need to document that BTRFS configuration is ignored for system users?

Also, I think the commit message should document why we don't want BTRFS subvolumes for system users.

Subvolumes for non intercative users are wasteful as they multiply metadata operations and generate unneccesary filesystem objects for operations, a system user who doesnt generate snapshot worthy data, wont use.

But I dont think its worth documenting as this is simply sane design. This BTRFS Backend never mattered to system users as the BTRFS Backend orginally released only supported interactive subvol creation. There is no usage for subvols (aka wasting metadata) for system users with state home dirs.

@alejandro-colomar

alejandro-colomar commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator

The code seems good to me.
Do we need to document that BTRFS configuration is ignored for system users?
Also, I think the commit message should document why we don't want BTRFS subvolumes for system users.

Subvolumes for non intercative users are wasteful as they multiply metadata operations and generate unneccesary filesystem objects for operations, a system user who doesnt generate snapshot worthy data, wont use.

Please copy this into the commit message.

But I dont think its worth documenting as this is simply sane design. This BTRFS Backend never mattered to system users as the BTRFS Backend orginally released only supported interactive subvol creation. There is no usage for subvols (aka wasting metadata) for system users with state home dirs.

This is still useful information for someone like me that has never used BTRFS and doesn't know anything about it other than the fact that it's a file system. :)

I'd like to have it at least in the commit message. I don't care too much about the manual page.

@silverhadch silverhadch force-pushed the work/hadi/uid-guard-btrfs branch from efd0339 to f9906f3 Compare April 29, 2026 12:08
@silverhadch

Copy link
Copy Markdown
Contributor Author

The code seems good to me.
Do we need to document that BTRFS configuration is ignored for system users?
Also, I think the commit message should document why we don't want BTRFS subvolumes for system users.

Subvolumes for non intercative users are wasteful as they multiply metadata operations and generate unneccesary filesystem objects for operations, a system user who doesnt generate snapshot worthy data, wont use.

Please copy this into the commit message.

But I dont think its worth documenting as this is simply sane design. This BTRFS Backend never mattered to system users as the BTRFS Backend orginally released only supported interactive subvol creation. There is no usage for subvols (aka wasting metadata) for system users with state home dirs.

This is still useful information for someone like me that has never used BTRFS and doesn't know anything about it other than the fact that it's a file system. :)

I'd like to have it at least in the commit message. I don't care too much about the manual page.

Done. Documented in the commit message.

@silverhadch silverhadch force-pushed the work/hadi/uid-guard-btrfs branch from f9906f3 to 2176bd2 Compare April 29, 2026 12:17

@hallyn hallyn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It seems to me this should be optional, no?

I don't think everyone would agree that every user under UID_MIN does not benefit from subvolumes.

@silverhadch

Copy link
Copy Markdown
Contributor Author

It seems to me this should be optional, no?

I don't think everyone would agree that every user under UID_MIN does not benefit from subvolumes.

How so?

Why should a System User need to waste resources?

@ikerexxe

ikerexxe commented May 5, 2026

Copy link
Copy Markdown
Collaborator

I think this should be optional rather than hardcoded. Following the Unix philosophy of "mechanism, not policy," we should default the behavior to off (no subvolumes for system users). This addresses the performance concerns while providing a configuration toggle for edge cases or environments that still need it.

My two cents

@hallyn

hallyn commented May 5, 2026

Copy link
Copy Markdown
Member

Yup I'd be fine with making it the default to not create the subvolume for system users.

@hallyn

hallyn commented Jul 8, 2026

Copy link
Copy Markdown
Member

@silverhadch do you mind making the change to make this non-default?

@silverhadch

Copy link
Copy Markdown
Contributor Author

@silverhadch do you mind making the change to make this non-default?

Sure once I finish the Security fix then I will piçk up this PR again.

@hallyn

hallyn commented Jul 9, 2026 via email

Copy link
Copy Markdown
Member

@silverhadch silverhadch force-pushed the work/hadi/uid-guard-btrfs branch from 2176bd2 to 5a500aa Compare July 10, 2026 11:40
Comment thread src/useradd.c Outdated
Gate subvolume creation on UID_MIN <= UID <= UID_MAX to exclude system
users regardless of their home path.

System users hold no snapshot-worthy data, so subvolumes for them just
waste metadata and add filesystem objects nobody uses. The BTRFS
backend was always aimed at interactive users anyway.

However you can set BTRFS_SUBVOLUME_SYSTEM=yes in /etc/default/useradd
to get the old behavior back. Has no effect unless subvolume creation
is enabled in the first place.

Document the new variable in useradd(8).

Fixes: c1d36a8 (2019-05-04; "Add support for btrfs subvolumes for user homes")
Signed-off-by: Hadi Chokr <hadichokr@icloud.com>
Some minimal container images do not ship /etc/default, so writing
/etc/default/useradd directly fails. Add a Shadow role helper that
creates the directory when missing and writes the file. Both are
reverted on teardown.

Signed-off-by: Hadi Chokr <hadichokr@icloud.com>
Check that a system user created with --btrfs-subvolume-home gets a
plain home directory by default, and a subvolume when
BTRFS_SUBVOLUME_SYSTEM=yes is set in /etc/default/useradd.

Signed-off-by: Hadi Chokr <hadichokr@icloud.com>
@silverhadch silverhadch force-pushed the work/hadi/uid-guard-btrfs branch from 8ce8dca to 4532b9c Compare July 10, 2026 12:05
@alejandro-colomar

Copy link
Copy Markdown
Collaborator

I abstain about this patch set.

I'll say that the quality of the source code meets my expectations.

I don't have an opinion on the feature itself.

@silverhadch

Copy link
Copy Markdown
Contributor Author

I abstain about this patch set.

I'll say that the quality of the source code meets my expectations.

I don't have an opinion on the feature itself.

Who should I ask a review from?

@alejandro-colomar

Copy link
Copy Markdown
Collaborator

I abstain about this patch set.
I'll say that the quality of the source code meets my expectations.
I don't have an opinion on the feature itself.

Who should I ask a review from?

@hallyn

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants