Re: [PATCH] btrfs: use mount idmap for defrag permission check

[Seth Forshee <sforshee@kernel.org>]

On Fri, Aug 14, 2026 at 01:15:51PM +0800, Tao Cui wrote:
> 
> 
> 在 2026/8/14 01:11, Seth Forshee 写道:
> > On Thu, Aug 13, 2026 at 04:04:05PM +0930, Qu Wenruo wrote:
> >>
> >>
> >> 在 2026/8/13 13:11, Tao Cui 写道:
> >>> From: Tao Cui <cuitao@kylinos.cn>
> >>>
> >>> btrfs_ioctl_defrag() checks MAY_WRITE with nop_mnt_idmap, which skips
> >>> the mount idmap.  On an idmapped mount the owner comparison then uses
> >>> the caller's fsuid against the raw on-disk uid, dropping the mapping.
> >>> Every other permission/owner check in btrfs ioctl uses
> >>> file_mnt_idmap(file) (e.g. :1152, :1310, :1946); this one missed it.
> >>>
> >>> Switch to file_mnt_idmap(file).  It equals nop_mnt_idmap on a normal
> >>> mount, and the check stays behind !capable(CAP_SYS_ADMIN), so only
> >>> unprivileged callers on idmapped btrfs change.  The RO-fd note in the
> >>> comment above is about the file descriptor, not this inode check, and
> >>> is unaffected.
> >>>
> >>> Signed-off-by: Tao Cui <cuitao@kylinos.cn>
> >>
> >> Fixes: 4609e1f18e19 ("fs: port ->permission() to pass mnt_idmap")
> > 
> > This is a misattribution. Using nop_mnt_idmap there keeps the behavior
> > the same as it was before the commit. Switching to the mount idmap opens
> > up BTRFS_IOC_DEFRAG* to idmapped users when they weren't before, which
> > is a separate policy decision that didn't belong in that change.
> > 
> Agreed, I'll resend it as a behavior change with no Fixes tag.
> > Saying that these ioctls should be allowed for idmapped users just
> > because others are isn't a good justification. Why are these ioctls safe
> > for idmapped users?  Do they actually require this capability?
> > 
> The check isn't a privilege gate. 616d374efa23 added it to test "whether
> the file could have been opened rw", and the caller can already do that
> on the idmapped mount -- open O_RDWR, write(), fallocate(). Only the
> regular-file branch changes, and defrag only reorganizes the caller's
> own extents; the S_IFDIR branch (whole-subvolume defrag) keeps its
> capable(CAP_SYS_ADMIN).
> 
> Nothing is weakened: !capable(CAP_SYS_ADMIN) stays exactly as is.
> 
> may_dedupe_file() in fs/remap_range.c already gates FIDEDUPERANGE the
> same way -- capable(CAP_SYS_ADMIN), then i_uid_into_vfsuid(
> file_mnt_idmap(file)) / inode_permission(idmap, MAY_WRITE) -- and dedupe
> is stronger, since it can rewrite one file's extents from another file's
> contents. Idmapped users can already dedupe on btrfs.

It seems perfectly reasonable to relax the check. I just think the
justification in the commit message should be updated to explain why
these specific ioctls should be allowed for idmapped users, similar to
pervious commits allowing idmapped use of the other ioctls.

Thanks,
Seth