| Message ID | 20180717220900.3588-1-kilobyte@angband.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 18, 2018 at 12:08:59AM +0200, Adam Borowski wrote: > Requiring a rw descriptor conflicts both ways with exec, returning ETXTBSY > whenever you try to defrag a program that's currently being run, or > causing intermittent exec failures on a live system being defragged. > > As defrag doesn't change the file's contents in any way, there's no reason > to consider it a rw operation. Thus, let's check only whether the file > could have been opened rw. Such access control is still needed as > currently defrag can use extra disk space, and might trigger bugs. > > Signed-off-by: Adam Borowski <kilobyte@angband.pl> > --- > fs/btrfs/ioctl.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 43ecbe620dea..01c150b6ab62 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -2941,7 +2941,8 @@ static int btrfs_ioctl_defrag(struct file *file, void __user *argp) > ret = btrfs_defrag_root(root); > break; > case S_IFREG: > - if (!(file->f_mode & FMODE_WRITE)) { > + if (!capable(CAP_SYS_ADMIN) && > + inode_permission(inode, MAY_WRITE)) { The dedupe ioctl does the admin check or the FMODE_WRITE, which means admin can dedupe anything but user has to have the file write open. Doing the same for defrag should be ok for same reasons it is for dedupe. I'm not sure about using plain inode_permissions here, though it looks correct and I'm not able to see inode vs file descriptor issues that could cause trouble here. There are uid/gid, rws, acl, immutable, capabilities, namespace aware checks, security hooks. So, I'll add the patch to 4.19 queue. It's small and isolated change so a revert would be easy in case we find something bad. The 2nd patch should be IMHO part of this change as it's logical to return the error code in the patch that modifies the user visible behaviour. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 23, 2018 at 05:26:24PM +0200, David Sterba wrote: > On Wed, Jul 18, 2018 at 12:08:59AM +0200, Adam Borowski wrote: (Combined with as-folded) | | btrfs: allow defrag on a file opened read-only that has rw permissions | | > > Requiring a rw descriptor conflicts both ways with exec, returning ETXTBSY > > whenever you try to defrag a program that's currently being run, or > > causing intermittent exec failures on a live system being defragged. > > > > As defrag doesn't change the file's contents in any way, there's no reason > > to consider it a rw operation. Thus, let's check only whether the file > > could have been opened rw. Such access control is still needed as > > currently defrag can use extra disk space, and might trigger bugs. <----- | | We give EINVAL when the request is invalid; here it's ok but merely the | | user has insufficient privileges. Thus, this return value reflects the | | error better -- as discussed in the identical case for dedupe. | | | | According to codesearch.debian.net, no userspace program distinguishes | | these values beyond strerror(). | | | | Signed-off-by: Adam Borowski <kilobyte@angband.pl> | | Reviewed-by: David Sterba <dsterba@suse.com> | | [ fold the EPERM patch from Adam ] | | Signed-off-by: David Sterba <dsterba@suse.com> [...] > So, I'll add the patch to 4.19 queue. It's small and isolated change so > a revert would be easy in case we find something bad. The 2nd patch > should be IMHO part of this change as it's logical to return the error > code in the patch that modifies the user visible behaviour. A nitpick: the new commit message has a dangling pointer "this" to the title of the commit that was squashed. It was: | btrfs: defrag: return EPERM not EINVAL when only permissions fail It'd be nice if it could be inserted in some form in the place I marked with an arrow. But then, commit messages are not vital. The actual functionality patch has been applied correctly. And thanks for adding the comment. Meow!
On Tue, Jul 24, 2018 at 10:35:28PM +0200, Adam Borowski wrote: > On Mon, Jul 23, 2018 at 05:26:24PM +0200, David Sterba wrote: > > On Wed, Jul 18, 2018 at 12:08:59AM +0200, Adam Borowski wrote: > (Combined with as-folded) > > | | btrfs: allow defrag on a file opened read-only that has rw permissions > | | > > > Requiring a rw descriptor conflicts both ways with exec, returning ETXTBSY > > > whenever you try to defrag a program that's currently being run, or > > > causing intermittent exec failures on a live system being defragged. > > > > > > As defrag doesn't change the file's contents in any way, there's no reason > > > to consider it a rw operation. Thus, let's check only whether the file > > > could have been opened rw. Such access control is still needed as > > > currently defrag can use extra disk space, and might trigger bugs. > <----- > | | We give EINVAL when the request is invalid; here it's ok but merely the > | | user has insufficient privileges. Thus, this return value reflects the > | | error better -- as discussed in the identical case for dedupe. > | | > | | According to codesearch.debian.net, no userspace program distinguishes > | | these values beyond strerror(). > | | > | | Signed-off-by: Adam Borowski <kilobyte@angband.pl> > | | Reviewed-by: David Sterba <dsterba@suse.com> > | | [ fold the EPERM patch from Adam ] > | | Signed-off-by: David Sterba <dsterba@suse.com> > > [...] > > So, I'll add the patch to 4.19 queue. It's small and isolated change so > > a revert would be easy in case we find something bad. The 2nd patch > > should be IMHO part of this change as it's logical to return the error > > code in the patch that modifies the user visible behaviour. > > A nitpick: the new commit message has a dangling pointer "this" to the title > of the commit that was squashed. It was: > > | btrfs: defrag: return EPERM not EINVAL when only permissions fail > > It'd be nice if it could be inserted in some form in the place I marked with > an arrow. Right. I've replaced 'this' with 'EPERM', thanks for catching it. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 43ecbe620dea..01c150b6ab62 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -2941,7 +2941,8 @@ static int btrfs_ioctl_defrag(struct file *file, void __user *argp) ret = btrfs_defrag_root(root); break; case S_IFREG: - if (!(file->f_mode & FMODE_WRITE)) { + if (!capable(CAP_SYS_ADMIN) && + inode_permission(inode, MAY_WRITE)) { ret = -EINVAL; goto out; }
Requiring a rw descriptor conflicts both ways with exec, returning ETXTBSY whenever you try to defrag a program that's currently being run, or causing intermittent exec failures on a live system being defragged. As defrag doesn't change the file's contents in any way, there's no reason to consider it a rw operation. Thus, let's check only whether the file could have been opened rw. Such access control is still needed as currently defrag can use extra disk space, and might trigger bugs. Signed-off-by: Adam Borowski <kilobyte@angband.pl> --- fs/btrfs/ioctl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)