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

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

[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>
---
 fs/btrfs/ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 72bc9d4f7708..607329dedd50 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2458,7 +2458,7 @@ static int btrfs_ioctl_defrag(struct file *file, void __user *argp)
 		 * running and allows defrag on files open in read-only mode.
 		 */
 		if (!capable(CAP_SYS_ADMIN) &&
-		    inode_permission(&nop_mnt_idmap, inode, MAY_WRITE)) {
+		    inode_permission(file_mnt_idmap(file), inode, MAY_WRITE)) {
 			ret = -EPERM;
 			goto out;
 		}
-- 
2.43.0

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

[Qu Wenruo]

在 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")

Otherwise looks good to me.

Reviewed-by: Qu Wenruo <wqu@suse.com>
> ---
>   fs/btrfs/ioctl.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 72bc9d4f7708..607329dedd50 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2458,7 +2458,7 @@ static int btrfs_ioctl_defrag(struct file *file, void __user *argp)
>   		 * running and allows defrag on files open in read-only mode.
>   		 */
>   		if (!capable(CAP_SYS_ADMIN) &&
> -		    inode_permission(&nop_mnt_idmap, inode, MAY_WRITE)) {
> +		    inode_permission(file_mnt_idmap(file), inode, MAY_WRITE)) {
>   			ret = -EPERM;
>   			goto out;
>   		}

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

[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.

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?

Thanks,
Seth

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

[Qu Wenruo]

在 2026/8/14 02:41, 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.

OK, removed from for-next, and will not add the 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?

I'll let the author to do the explanation.

Thanks,
Qu

> 
> Thanks,
> Seth
> 

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

[Tao Cui]

在 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.

Thanks,
Tao

> Thanks,
> Seth

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

[Seth Forshee]

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