[RFC] Allow to exec "btrfs subvolume delete" by a non root user

[RFC] Allow to exec "btrfs subvolume delete" by a non root user

[Goffredo Baroncelli]

[-- Attachment #1: Type: Text/Plain, Size: 4249 bytes --]

Hi all

like my previous patch, this one allow to remove a subvolume by an ordinary
 user. Instead of adding this capability to the rmdir(2) syscall, I update the
BTRFS_IOC_SNAP_DESTROY ioctl, relaxing the rules to be execute.
The checks are the ones performed by the rmdir(2) syscall. So a 
subvolume must be empty to be removed by a non-root user. I think that this
 increases a lot the usefulness of the snapshot/subvolume.

It is possible to pull the code from the branch named "rm-subvolume-not-root" 
of the following repository:

      http://cassiopea.homelinux.net/git/btrfs-unstable.git  

Comments are welcome.

Reagrds
G.Baroncelli

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 9254b3d..5bbb6bc 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -395,6 +395,76 @@ fail:
 	return ret;
 }
 
+/*  copy of check_sticky in fs/namei.c() 
+* It's inline, so penalty for filesystems that don't use sticky bit is
+* minimal.
+*/
+static inline int btrfs_check_sticky(struct inode *dir, struct inode *inode)
+{
+	uid_t fsuid = current_fsuid();
+
+	if (!(dir->i_mode & S_ISVTX))
+		return 0;
+	if (inode->i_uid == fsuid)
+		return 0;
+	if (dir->i_uid == fsuid)
+		return 0;
+	return !capable(CAP_FOWNER);
+}
+
+/*  copy of may_delete in fs/namei.c() 
+ *	Check whether we can remove a link victim from directory dir, check
+ *  whether the type of victim is right.
+ *  1. We can't do it if dir is read-only (done in permission())
+ *  2. We should have write and exec permissions on dir
+ *  3. We can't remove anything from append-only dir
+ *  4. We can't do anything with immutable dir (done in permission())
+ *  5. If the sticky bit on dir is set we should either
+ *	a. be owner of dir, or
+ *	b. be owner of victim, or
+ *	c. have CAP_FOWNER capability
+ *  6. If the victim is append-only or immutable we can't do antyhing with
+ *     links pointing to it.
+ *  7. If we were asked to remove a directory and victim isn't one - ENOTDIR.
+ *  8. If we were asked to remove a non-directory and victim isn't one - EISDIR.
+ *  9. We can't remove a root or mountpoint.
+ * 10. We don't allow removal of NFS sillyrenamed files; it's handled by
+ *     nfs_async_unlink().
+ */
+
+static int btrfs_may_delete(struct inode *dir,struct dentry *victim,int isdir)
+{
+	int error;
+
+	if (!victim->d_inode)
+		return -ENOENT;
+
+	BUG_ON(victim->d_parent->d_inode != dir);
+	audit_inode_child(victim, dir);
+
+	error = inode_permission(dir, MAY_WRITE | MAY_EXEC);
+	if (error)
+		return error;
+	if (IS_APPEND(dir))
+		return -EPERM;
+	if (btrfs_check_sticky(dir, victim->d_inode)||
+		IS_APPEND(victim->d_inode)||
+	    IS_IMMUTABLE(victim->d_inode) || IS_SWAPFILE(victim->d_inode))
+		return -EPERM;
+	if (isdir) {
+		if (!S_ISDIR(victim->d_inode->i_mode))
+			return -ENOTDIR;
+		if (IS_ROOT(victim))
+			return -EBUSY;
+	} else if (S_ISDIR(victim->d_inode->i_mode))
+		return -EISDIR;
+	if (IS_DEADDIR(dir))
+		return -ENOENT;
+	if (victim->d_flags & DCACHE_NFSFS_RENAMED)
+		return -EBUSY;
+	return 0;
+}
+
 /* copy of may_create in fs/namei.c() */
 static inline int btrfs_may_create(struct inode *dir, struct dentry *child)
 {
@@ -1227,9 +1297,6 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 	int ret;
 	int err = 0;
 
-	if (!capable(CAP_SYS_ADMIN))
-		return -EPERM;
-
 	vol_args = memdup_user(arg, sizeof(*vol_args));
 	if (IS_ERR(vol_args))
 		return PTR_ERR(vol_args);
@@ -1259,6 +1326,20 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 	}
 
 	inode = dentry->d_inode;
+	if (!capable(CAP_SYS_ADMIN)){
+		/* regolar user */
+		/* check if subvolume is empty */
+		if (inode->i_size > BTRFS_EMPTY_DIR_SIZE){
+			err = -ENOTEMPTY;
+			goto out_dput;
+		}
+		/* check if subvolume may be deleted by a non-root user */	
+		if (btrfs_may_delete(dir, dentry, 1)){
+			err = -EPERM;
+			goto out_dput;
+		}
+	}
+
 	if (inode->i_ino != BTRFS_FIRST_FREE_OBJECTID) {
 		err = -EINVAL;
 		goto out_dput;



-- 
gpg key@ keyserver.linux.it: Goffredo Baroncelli (ghigo) <kreijack@inwind.it>
Key fingerprint = 4769 7E51 5293 D36C 814E  C054 BF04 F161 3DC5 0512

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [RFC] Allow to exec "btrfs subvolume delete" by a non root user

[Sage Weil]

On Mon, 18 Oct 2010, Goffredo Baroncelli wrote:

> Hi all
> 
> like my previous patch, this one allow to remove a subvolume by an ordinary
>  user. Instead of adding this capability to the rmdir(2) syscall, I update the
> BTRFS_IOC_SNAP_DESTROY ioctl, relaxing the rules to be execute.
> The checks are the ones performed by the rmdir(2) syscall. So a 
> subvolume must be empty to be removed by a non-root user. I think that this
>  increases a lot the usefulness of the snapshot/subvolume.
> 
> It is possible to pull the code from the branch named "rm-subvolume-not-root" 
> of the following repository:
> 
>       http://cassiopea.homelinux.net/git/btrfs-unstable.git  
> 
> Comments are welcome.

This looks okay to me.  I posted a similar patch a while back[1], but 
didn't want to duplicate the check_sticky and may_delete code and 
implemented a simpler set of checks instead.  The full checks are probably 
a better route, although it would be nice if we could avoid duplicating 
the VFS checks in the process.  Whether those helpers should be exported 
is someone else's call, though.  (The only other may_ functions that are 
exported are may_umount and may_umount_tree.)

sage

[1] http://marc.info/?l=linux-btrfs&m=128086492512628&w=2


> 
> Reagrds
> G.Baroncelli
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 9254b3d..5bbb6bc 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -395,6 +395,76 @@ fail:
>  	return ret;
>  }
>  
> +/*  copy of check_sticky in fs/namei.c() 
> +* It's inline, so penalty for filesystems that don't use sticky bit is
> +* minimal.
> +*/
> +static inline int btrfs_check_sticky(struct inode *dir, struct inode *inode)
> +{
> +	uid_t fsuid = current_fsuid();
> +
> +	if (!(dir->i_mode & S_ISVTX))
> +		return 0;
> +	if (inode->i_uid == fsuid)
> +		return 0;
> +	if (dir->i_uid == fsuid)
> +		return 0;
> +	return !capable(CAP_FOWNER);
> +}
> +
> +/*  copy of may_delete in fs/namei.c() 
> + *	Check whether we can remove a link victim from directory dir, check
> + *  whether the type of victim is right.
> + *  1. We can't do it if dir is read-only (done in permission())
> + *  2. We should have write and exec permissions on dir
> + *  3. We can't remove anything from append-only dir
> + *  4. We can't do anything with immutable dir (done in permission())
> + *  5. If the sticky bit on dir is set we should either
> + *	a. be owner of dir, or
> + *	b. be owner of victim, or
> + *	c. have CAP_FOWNER capability
> + *  6. If the victim is append-only or immutable we can't do antyhing with
> + *     links pointing to it.
> + *  7. If we were asked to remove a directory and victim isn't one - ENOTDIR.
> + *  8. If we were asked to remove a non-directory and victim isn't one - EISDIR.
> + *  9. We can't remove a root or mountpoint.
> + * 10. We don't allow removal of NFS sillyrenamed files; it's handled by
> + *     nfs_async_unlink().
> + */
> +
> +static int btrfs_may_delete(struct inode *dir,struct dentry *victim,int isdir)
> +{
> +	int error;
> +
> +	if (!victim->d_inode)
> +		return -ENOENT;
> +
> +	BUG_ON(victim->d_parent->d_inode != dir);
> +	audit_inode_child(victim, dir);
> +
> +	error = inode_permission(dir, MAY_WRITE | MAY_EXEC);
> +	if (error)
> +		return error;
> +	if (IS_APPEND(dir))
> +		return -EPERM;
> +	if (btrfs_check_sticky(dir, victim->d_inode)||
> +		IS_APPEND(victim->d_inode)||
> +	    IS_IMMUTABLE(victim->d_inode) || IS_SWAPFILE(victim->d_inode))
> +		return -EPERM;
> +	if (isdir) {
> +		if (!S_ISDIR(victim->d_inode->i_mode))
> +			return -ENOTDIR;
> +		if (IS_ROOT(victim))
> +			return -EBUSY;
> +	} else if (S_ISDIR(victim->d_inode->i_mode))
> +		return -EISDIR;
> +	if (IS_DEADDIR(dir))
> +		return -ENOENT;
> +	if (victim->d_flags & DCACHE_NFSFS_RENAMED)
> +		return -EBUSY;
> +	return 0;
> +}
> +
>  /* copy of may_create in fs/namei.c() */
>  static inline int btrfs_may_create(struct inode *dir, struct dentry *child)
>  {
> @@ -1227,9 +1297,6 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
>  	int ret;
>  	int err = 0;
>  
> -	if (!capable(CAP_SYS_ADMIN))
> -		return -EPERM;
> -
>  	vol_args = memdup_user(arg, sizeof(*vol_args));
>  	if (IS_ERR(vol_args))
>  		return PTR_ERR(vol_args);
> @@ -1259,6 +1326,20 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
>  	}
>  
>  	inode = dentry->d_inode;
> +	if (!capable(CAP_SYS_ADMIN)){
> +		/* regolar user */
> +		/* check if subvolume is empty */
> +		if (inode->i_size > BTRFS_EMPTY_DIR_SIZE){
> +			err = -ENOTEMPTY;
> +			goto out_dput;
> +		}
> +		/* check if subvolume may be deleted by a non-root user */	
> +		if (btrfs_may_delete(dir, dentry, 1)){
> +			err = -EPERM;
> +			goto out_dput;
> +		}
> +	}
> +
>  	if (inode->i_ino != BTRFS_FIRST_FREE_OBJECTID) {
>  		err = -EINVAL;
>  		goto out_dput;
> 
> 
> 
> -- 
> gpg key@ keyserver.linux.it: Goffredo Baroncelli (ghigo) <kreijack@inwind.it>
> Key fingerprint = 4769 7E51 5293 D36C 814E  C054 BF04 F161 3DC5 0512
> 

Re: [RFC] Allow to exec "btrfs subvolume delete" by a non root user

[Goffredo Baroncelli]

On Wednesday, 20 October, 2010, Sage Weil wrote:
> On Mon, 18 Oct 2010, Goffredo Baroncelli wrote:
> 
> > Hi all
> > 
> > like my previous patch, this one allow to remove a subvolume by an 
ordinary
> >  user. Instead of adding this capability to the rmdir(2) syscall, I update 
the
> > BTRFS_IOC_SNAP_DESTROY ioctl, relaxing the rules to be execute.
> > The checks are the ones performed by the rmdir(2) syscall. So a 
> > subvolume must be empty to be removed by a non-root user. I think that 
this
> >  increases a lot the usefulness of the snapshot/subvolume.
> > 
> > It is possible to pull the code from the branch named "rm-subvolume-not-
root" 
> > of the following repository:
> > 
> >       http://cassiopea.homelinux.net/git/btrfs-unstable.git  
> > 
> > Comments are welcome.
> 
> This looks okay to me.  I posted a similar patch a while back[1], but 
> didn't want to duplicate the check_sticky and may_delete code and 
> implemented a simpler set of checks instead.  The full checks are probably 
> a better route, although it would be nice if we could avoid duplicating 
> the VFS checks in the process.  Whether those helpers should be exported 
> is someone else's call, though.  (The only other may_ functions that are 
> exported are may_umount and may_umount_tree.)

I agree about the may_* function. But also there is the case of the 
may_create..
Anyway I want to highlight that the main differences between our patches is 
the fact that may patches needed the subvolume to be empty. So I skip all the 
problem related to removing a "not owned directory - not empty directory"

> 
> sage
> 
> [1] http://marc.info/?l=linux-btrfs&m=128086492512628&w=2


-- 
gpg key@ keyserver.linux.it: Goffredo Baroncelli (ghigo) <kreijack@inwind.it>
Key fingerprint = 4769 7E51 5293 D36C 814E  C054 BF04 F161 3DC5 0512