| Message ID | 1500658149-20410-1-git-send-email-jbacik@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 21, 2017 at 01:29:07PM -0400, josef@toxicpanda.com wrote: > From: Josef Bacik <jbacik@fb.com> > > We need to use file->private_data for readdir on directories, so just > don't allow user space transactions on directories. > > Signed-off-by: Josef Bacik <jbacik@fb.com> > --- > fs/btrfs/ioctl.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index bedeec6..ddb3811 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -3968,6 +3968,9 @@ static long btrfs_ioctl_trans_start(struct file *file) > struct btrfs_trans_handle *trans; > int ret; > > + if (S_ISDIR(inode->i_mode)) > + return -EINVAL; You can't do this, starting a transaction on a directory needs to work. The most natural way to run the ioctl is on the mount point. The file private data would need to be able to hold multipe values, so you can add struct btrfs_inode { ... struct priv_data { void *for_readdir; void *for_tranc_ioctl; }; ... }; then set file->file_private = &btrfs_inode->priv_data; and update all uses to check for the embedded pointers. > + > ret = -EPERM; > if (!capable(CAP_SYS_ADMIN)) > goto out; > -- > 2.7.4 > > -- > 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 -- 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 24, 2017 at 02:42:29PM +0200, David Sterba wrote: > On Fri, Jul 21, 2017 at 01:29:07PM -0400, josef@toxicpanda.com wrote: > > From: Josef Bacik <jbacik@fb.com> > > > > We need to use file->private_data for readdir on directories, so just > > don't allow user space transactions on directories. > > > > Signed-off-by: Josef Bacik <jbacik@fb.com> > > --- > > fs/btrfs/ioctl.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > > index bedeec6..ddb3811 100644 > > --- a/fs/btrfs/ioctl.c > > +++ b/fs/btrfs/ioctl.c > > @@ -3968,6 +3968,9 @@ static long btrfs_ioctl_trans_start(struct file *file) > > struct btrfs_trans_handle *trans; > > int ret; > > > > + if (S_ISDIR(inode->i_mode)) > > + return -EINVAL; > > You can't do this, starting a transaction on a directory needs to work. > The most natural way to run the ioctl is on the mount point. > > The file private data would need to be able to hold multipe values, so > you can add > > struct btrfs_inode { > ... > struct priv_data { > void *for_readdir; > void *for_tranc_ioctl; > }; > ... > }; > > then set file->file_private = &btrfs_inode->priv_data; and update all > uses to check for the embedded pointers. So this cannot be attached to the inode but to struct file itself, otherwise this won't work for parallel readdir obviously. -- 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 24, 2017 at 02:42:29PM +0200, David Sterba wrote: > On Fri, Jul 21, 2017 at 01:29:07PM -0400, josef@toxicpanda.com wrote: > > From: Josef Bacik <jbacik@fb.com> > > > > We need to use file->private_data for readdir on directories, so just > > don't allow user space transactions on directories. > > > > Signed-off-by: Josef Bacik <jbacik@fb.com> > > --- > > fs/btrfs/ioctl.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > > index bedeec6..ddb3811 100644 > > --- a/fs/btrfs/ioctl.c > > +++ b/fs/btrfs/ioctl.c > > @@ -3968,6 +3968,9 @@ static long btrfs_ioctl_trans_start(struct file *file) > > struct btrfs_trans_handle *trans; > > int ret; > > > > + if (S_ISDIR(inode->i_mode)) > > + return -EINVAL; > > You can't do this, starting a transaction on a directory needs to work. > The most natural way to run the ioctl is on the mount point. > > The file private data would need to be able to hold multipe values, so > you can add > > struct btrfs_inode { > ... > struct priv_data { > void *for_readdir; > void *for_tranc_ioctl; > }; > ... > }; > > then set file->file_private = &btrfs_inode->priv_data; and update all > uses to check for the embedded pointers. > Blah I really want to just jetison the user space transaction stuff altogether so I was hoping this would be a first step. But yeah we can do it your way too. Thanks, Josef -- 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 24, 2017 at 10:02:30AM -0400, Josef Bacik wrote: > On Mon, Jul 24, 2017 at 02:42:29PM +0200, David Sterba wrote: > > On Fri, Jul 21, 2017 at 01:29:07PM -0400, josef@toxicpanda.com wrote: > > > From: Josef Bacik <jbacik@fb.com> > > > > > > We need to use file->private_data for readdir on directories, so just > > > don't allow user space transactions on directories. > > > > > > Signed-off-by: Josef Bacik <jbacik@fb.com> > > > --- > > > fs/btrfs/ioctl.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > > > index bedeec6..ddb3811 100644 > > > --- a/fs/btrfs/ioctl.c > > > +++ b/fs/btrfs/ioctl.c > > > @@ -3968,6 +3968,9 @@ static long btrfs_ioctl_trans_start(struct file *file) > > > struct btrfs_trans_handle *trans; > > > int ret; > > > > > > + if (S_ISDIR(inode->i_mode)) > > > + return -EINVAL; > > > > You can't do this, starting a transaction on a directory needs to work. > > The most natural way to run the ioctl is on the mount point. > > > > The file private data would need to be able to hold multipe values, so > > you can add > > > > struct btrfs_inode { > > ... > > struct priv_data { > > void *for_readdir; > > void *for_tranc_ioctl; > > }; > > ... > > }; > > > > then set file->file_private = &btrfs_inode->priv_data; and update all > > uses to check for the embedded pointers. > > Blah I really want to just jetison the user space transaction stuff altogether > so I was hoping this would be a first step. But yeah we can do it your way too. I'm fine with removing the trans ioctl, ceph does not use it. We may need one or two release cycles when we mark it deprecated and WARN_ON_ONCE when used. But as it's undocumented and tricky to use I guess nobody will notice. Unfortunatelly this means you still have to add the extra structures for readdir. -- 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 bedeec6..ddb3811 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -3968,6 +3968,9 @@ static long btrfs_ioctl_trans_start(struct file *file) struct btrfs_trans_handle *trans; int ret; + if (S_ISDIR(inode->i_mode)) + return -EINVAL; + ret = -EPERM; if (!capable(CAP_SYS_ADMIN)) goto out;