| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAOQ4uxi5eyXocmFaDdT_1Jvo0ZiEf66bC9u5qn6B2Rdd_Fuqyw@mail.gmail.com>
Date: Tue, 3 Jun 2025 14:43:02 +0200
From: Amir Goldstein <amir73il@...il.com>
To: wangtao <tao.wangtao@...or.com>
Cc: "sumit.semwal@...aro.org" <sumit.semwal@...aro.org>,
"christian.koenig@....com" <christian.koenig@....com>, "kraxel@...hat.com" <kraxel@...hat.com>,
"vivek.kasireddy@...el.com" <vivek.kasireddy@...el.com>,
"viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>, "brauner@...nel.org" <brauner@...nel.org>,
"hughd@...gle.com" <hughd@...gle.com>, "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
"benjamin.gaignard@...labora.com" <benjamin.gaignard@...labora.com>,
"Brian.Starkey@....com" <Brian.Starkey@....com>, "jstultz@...gle.com" <jstultz@...gle.com>,
"tjmercier@...gle.com" <tjmercier@...gle.com>, "jack@...e.cz" <jack@...e.cz>,
"baolin.wang@...ux.alibaba.com" <baolin.wang@...ux.alibaba.com>,
"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
"linaro-mm-sig@...ts.linaro.org" <linaro-mm-sig@...ts.linaro.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>, "linux-mm@...ck.org" <linux-mm@...ck.org>,
"wangbintian(BintianWang)" <bintian.wang@...or.com>, yipengxiang <yipengxiang@...or.com>,
liulu 00013167 <liulu.liu@...or.com>, hanfeng 00012985 <feng.han@...or.com>
Subject: Re: [PATCH v4 1/4] fs: allow cross-FS copy_file_range for memory file
with direct I/O
On Tue, Jun 3, 2025 at 2:38 PM wangtao <tao.wangtao@...or.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Amir Goldstein <amir73il@...il.com>
> > Sent: Tuesday, June 3, 2025 6:57 PM
> > To: wangtao <tao.wangtao@...or.com>
> > Cc: sumit.semwal@...aro.org; christian.koenig@....com;
> > kraxel@...hat.com; vivek.kasireddy@...el.com; viro@...iv.linux.org.uk;
> > brauner@...nel.org; hughd@...gle.com; akpm@...ux-foundation.org;
> > benjamin.gaignard@...labora.com; Brian.Starkey@....com;
> > jstultz@...gle.com; tjmercier@...gle.com; jack@...e.cz;
> > baolin.wang@...ux.alibaba.com; linux-media@...r.kernel.org; dri-
> > devel@...ts.freedesktop.org; linaro-mm-sig@...ts.linaro.org; linux-
> > kernel@...r.kernel.org; linux-fsdevel@...r.kernel.org; linux-
> > mm@...ck.org; wangbintian(BintianWang) <bintian.wang@...or.com>;
> > yipengxiang <yipengxiang@...or.com>; liulu 00013167
> > <liulu.liu@...or.com>; hanfeng 00012985 <feng.han@...or.com>
> > Subject: Re: [PATCH v4 1/4] fs: allow cross-FS copy_file_range for memory
> > file with direct I/O
> >
> > On Tue, Jun 3, 2025 at 11:53 AM wangtao <tao.wangtao@...or.com> wrote:
> > >
> > > Memory files can optimize copy performance via copy_file_range callbacks:
> > > -Compared to mmap&read: reduces GUP (get_user_pages) overhead
> > > -Compared to sendfile/splice: eliminates one memory copy -Supports
> > > dma-buf direct I/O zero-copy implementation
> > >
> > > Suggested by: Christian König <christian.koenig@....com> Suggested by:
> > > Amir Goldstein <amir73il@...il.com>
> > > Signed-off-by: wangtao <tao.wangtao@...or.com>
> > > ---
> > > fs/read_write.c | 64 +++++++++++++++++++++++++++++++++++++-----
> > ----
> > > include/linux/fs.h | 2 ++
> > > 2 files changed, 54 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/fs/read_write.c b/fs/read_write.c index
> > > bb0ed26a0b3a..ecb4f753c632 100644
> > > --- a/fs/read_write.c
> > > +++ b/fs/read_write.c
> > > @@ -1469,6 +1469,31 @@ COMPAT_SYSCALL_DEFINE4(sendfile64, int,
> > out_fd,
> > > int, in_fd, } #endif
> > >
> > > +static const struct file_operations *memory_copy_file_ops(
> > > + struct file *file_in, struct file *file_out) {
> > > + if ((file_in->f_op->fop_flags & FOP_MEMORY_FILE) &&
> > > + (file_in->f_mode & FMODE_CAN_ODIRECT) &&
> > > + file_in->f_op->copy_file_range && file_out->f_op->write_iter)
> > > + return file_in->f_op;
> > > + else if ((file_out->f_op->fop_flags & FOP_MEMORY_FILE) &&
> > > + (file_out->f_mode & FMODE_CAN_ODIRECT) &&
> > > + file_in->f_op->read_iter && file_out->f_op->copy_file_range)
> > > + return file_out->f_op;
> > > + else
> > > + return NULL;
> > > +}
> > > +
> > > +static int essential_file_rw_checks(struct file *file_in, struct file
> > > +*file_out) {
> > > + if (!(file_in->f_mode & FMODE_READ) ||
> > > + !(file_out->f_mode & FMODE_WRITE) ||
> > > + (file_out->f_flags & O_APPEND))
> > > + return -EBADF;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > /*
> > > * Performs necessary checks before doing a file copy
> > > *
> > > @@ -1484,9 +1509,16 @@ static int generic_copy_file_checks(struct file
> > *file_in, loff_t pos_in,
> > > struct inode *inode_out = file_inode(file_out);
> > > uint64_t count = *req_count;
> > > loff_t size_in;
> > > + bool splice = flags & COPY_FILE_SPLICE;
> > > + const struct file_operations *mem_fops;
> > > int ret;
> > >
> > > - ret = generic_file_rw_checks(file_in, file_out);
> > > + /* The dma-buf file is not a regular file. */
> > > + mem_fops = memory_copy_file_ops(file_in, file_out);
> > > + if (splice || mem_fops == NULL)
> >
> > nit: use !mem_fops please
> >
> > Considering that the flag COPY_FILE_SPLICE is not allowed from userspace
> > and is only called by nfsd and ksmbd I think we should assert and deny the
> > combination of mem_fops && splice because it is very much unexpected.
> >
> > After asserting this, it would be nicer to write as:
> > if (mem_fops)
> > ret = essential_file_rw_checks(file_in, file_out);
> > else
> > ret = generic_file_rw_checks(file_in, file_out);
> >
> Got it. Thanks.
> > > + else
> > > + ret = essential_file_rw_checks(file_in, file_out);
> > > if (ret)
> > > return ret;
> > >
> > > @@ -1500,8 +1532,10 @@ static int generic_copy_file_checks(struct file
> > *file_in, loff_t pos_in,
> > > * and several different sets of file_operations, but they all end up
> > > * using the same ->copy_file_range() function pointer.
> > > */
> > > - if (flags & COPY_FILE_SPLICE) {
> > > + if (splice) {
> > > /* cross sb splice is allowed */
> > > + } else if (mem_fops != NULL) {
> >
> > With the assertion that splice && mem_fops is not allowed if (splice ||
> > mem_fops) {
> >
> > would go well together because they both allow cross-fs copy not only cross
> > sb.
> >
> Git it.
>
> > > + /* cross-fs copy is allowed for memory file. */
> > > } else if (file_out->f_op->copy_file_range) {
> > > if (file_in->f_op->copy_file_range !=
> > > file_out->f_op->copy_file_range) @@ -1554,6
> > > +1588,7 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
> > > ssize_t ret;
> > > bool splice = flags & COPY_FILE_SPLICE;
> > > bool samesb = file_inode(file_in)->i_sb ==
> > > file_inode(file_out)->i_sb;
> > > + const struct file_operations *mem_fops;
> > >
> > > if (flags & ~COPY_FILE_SPLICE)
> > > return -EINVAL;
> > > @@ -1574,18 +1609,27 @@ ssize_t vfs_copy_file_range(struct file *file_in,
> > loff_t pos_in,
> > > if (len == 0)
> > > return 0;
> > >
> > > + if (splice)
> > > + goto do_splice;
> > > +
> > > file_start_write(file_out);
> > >
> >
> > goto do_splice needs to be after file_start_write
> >
> > Please wait for feedback from vfs maintainers before posting another
> > version addressing my review comments.
> >
> Are you asking whether both the goto do_splice and the do_splice label should
> be enclosed between file_start_write and file_end_write?
No I was just wrong please ignore this comment.
Thanks,
Amir.
Powered by blists - more mailing lists