| Message ID | 20170111095139.17783-1-jthumshirn@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2017-01-11 at 10:51 +0100, Johannes Thumshirn wrote: > Introduce FMODE_SPLICE_READ and FMODE_SPLICE_WRITE. These modes check > whether it is legal to read or write a file using splice. Both get > automatically set on regular files and are not checked when a 'struct > fileoperations' includes the splice_{read,write} methods. > Could you add a description of the problem that this solves? I assume you hit a problem trying to splice to/from a non-regular file, but it'd be good to know what that problem was. Thanks, > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Al Viro <viro@zeniv.linux.org.uk> > Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> > --- > fs/open.c | 4 ++++ > fs/splice.c | 6 ++++++ > include/linux/fs.h | 5 +++++ > 3 files changed, 15 insertions(+) > > diff --git a/fs/open.c b/fs/open.c > index 9921f70..b71259c 100644 > --- a/fs/open.c > +++ b/fs/open.c > @@ -733,6 +733,10 @@ static int do_dentry_open(struct file *f, > if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)) > f->f_mode |= FMODE_ATOMIC_POS; > > + if (S_ISREG(inode->i_mode)) > + f->f_mode |= FMODE_SPLICE_WRITE | FMODE_SPLICE_READ; > + > + > f->f_op = fops_get(inode->i_fop); > if (unlikely(WARN_ON(!f->f_op))) { > error = -ENODEV; > diff --git a/fs/splice.c b/fs/splice.c > index 873d831..b0cfcb2 100644 > --- a/fs/splice.c > +++ b/fs/splice.c > @@ -397,6 +397,9 @@ static ssize_t default_file_splice_read(struct file *in, loff_t *ppos, > if (pipe->nrbufs == pipe->buffers) > return -EAGAIN; > > + if (unlikely(!(in->f_mode & FMODE_SPLICE_READ))) > + return -EINVAL; > + > /* > * Try to keep page boundaries matching to source pagecache ones - > * it probably won't be much help, but... > @@ -825,6 +828,9 @@ static ssize_t default_file_splice_write(struct pipe_inode_info *pipe, > { > ssize_t ret; > > + if (unlikely(!(out->f_mode & FMODE_SPLICE_WRITE))) > + return -EINVAL; > + > ret = splice_from_pipe(pipe, out, ppos, len, flags, write_pipe_buf); > if (ret > 0) > *ppos += ret; > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 2ba0743..30477c5 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -143,6 +143,11 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset, > /* File was opened by fanotify and shouldn't generate fanotify events */ > #define FMODE_NONOTIFY ((__force fmode_t)0x4000000) > > +/* File can be read using splice */ > +#define FMODE_SPLICE_READ ((__force fmode_t)0x8000000) > +/* File can be written using splice */ > +#define FMODE_SPLICE_WRITE ((__force fmode_t)0x10000000) > + > /* > * Flag for rw_copy_check_uvector and compat_rw_copy_check_uvector > * that indicates that they should check the contents of the iovec are
On Wed, Jan 11, 2017 at 07:20:13AM -0500, Jeff Layton wrote: > On Wed, 2017-01-11 at 10:51 +0100, Johannes Thumshirn wrote: > > Introduce FMODE_SPLICE_READ and FMODE_SPLICE_WRITE. These modes check > > whether it is legal to read or write a file using splice. Both get > > automatically set on regular files and are not checked when a 'struct > > fileoperations' includes the splice_{read,write} methods. > > > > Could you add a description of the problem that this solves? I assume > you hit a problem trying to splice to/from a non-regular file, but it'd > be good to know what that problem was. The problem is that a driver's ->write() is called under KERNEL_DS this way. This happened for sg and bsg and caused 128394eff 'sg_write()/bsg_write() is not fit to be called under KERNEL_DS' as well as a0ac402cf 'Don't feed anything but regular iovec's to blk_rq_map_user_iov'. There have also been patches for InfiniBand AFAIR doing similar things. So this is to solve it for future abuses. HTH, Johannes
On Wed, Jan 11, 2017 at 07:20:13AM -0500, Jeff Layton wrote: > On Wed, 2017-01-11 at 10:51 +0100, Johannes Thumshirn wrote: > > Introduce FMODE_SPLICE_READ and FMODE_SPLICE_WRITE. These modes check > > whether it is legal to read or write a file using splice. Both get > > automatically set on regular files and are not checked when a 'struct > > fileoperations' includes the splice_{read,write} methods. > > > > Could you add a description of the problem that this solves? I assume > you hit a problem trying to splice to/from a non-regular file, but it'd > be good to know what that problem was. Insane ->write() instances, basically. I'm not at all convinced that it's a good idea - sure, we can go and mark sane ones as such one-by-one, but it's a _lot_ of code churn and insane ones are very few. Moreover, I would argue that the right way to handle that is to reject any new instances of that insanity - splice or no splice, write(2) that includes userland pointers in payload and dereferences them is not fit to live. /dev/sg, /dev/bsg and infinibarf are examples of really bad APIs; sure, we can't kill them off (at least /dev/sg is used by a bunch of userland programs and all of them expect that semantics), but that doesn't excuse any new drivers trying to introduce the same. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" 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/open.c b/fs/open.c index 9921f70..b71259c 100644 --- a/fs/open.c +++ b/fs/open.c @@ -733,6 +733,10 @@ static int do_dentry_open(struct file *f, if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)) f->f_mode |= FMODE_ATOMIC_POS; + if (S_ISREG(inode->i_mode)) + f->f_mode |= FMODE_SPLICE_WRITE | FMODE_SPLICE_READ; + + f->f_op = fops_get(inode->i_fop); if (unlikely(WARN_ON(!f->f_op))) { error = -ENODEV; diff --git a/fs/splice.c b/fs/splice.c index 873d831..b0cfcb2 100644 --- a/fs/splice.c +++ b/fs/splice.c @@ -397,6 +397,9 @@ static ssize_t default_file_splice_read(struct file *in, loff_t *ppos, if (pipe->nrbufs == pipe->buffers) return -EAGAIN; + if (unlikely(!(in->f_mode & FMODE_SPLICE_READ))) + return -EINVAL; + /* * Try to keep page boundaries matching to source pagecache ones - * it probably won't be much help, but... @@ -825,6 +828,9 @@ static ssize_t default_file_splice_write(struct pipe_inode_info *pipe, { ssize_t ret; + if (unlikely(!(out->f_mode & FMODE_SPLICE_WRITE))) + return -EINVAL; + ret = splice_from_pipe(pipe, out, ppos, len, flags, write_pipe_buf); if (ret > 0) *ppos += ret; diff --git a/include/linux/fs.h b/include/linux/fs.h index 2ba0743..30477c5 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -143,6 +143,11 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset, /* File was opened by fanotify and shouldn't generate fanotify events */ #define FMODE_NONOTIFY ((__force fmode_t)0x4000000) +/* File can be read using splice */ +#define FMODE_SPLICE_READ ((__force fmode_t)0x8000000) +/* File can be written using splice */ +#define FMODE_SPLICE_WRITE ((__force fmode_t)0x10000000) + /* * Flag for rw_copy_check_uvector and compat_rw_copy_check_uvector * that indicates that they should check the contents of the iovec are
Introduce FMODE_SPLICE_READ and FMODE_SPLICE_WRITE. These modes check whether it is legal to read or write a file using splice. Both get automatically set on regular files and are not checked when a 'struct fileoperations' includes the splice_{read,write} methods. Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Cc: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> --- fs/open.c | 4 ++++ fs/splice.c | 6 ++++++ include/linux/fs.h | 5 +++++ 3 files changed, 15 insertions(+)