| Message ID | 20210922072326.3538-1-huangjianan@oppo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] ovl: fix null pointer when filesystem doesn't support direct IO | expand |
在 2021/9/22 15:23, Huang Jianan 写道: > From: Huang Jianan <huangjianan@oppo.com> > > At present, overlayfs provides overlayfs inode to users. Overlayfs > inode provides ovl_aops with noop_direct_IO to avoid open failure > with O_DIRECT. But some compressed filesystems, such as erofs and > squashfs, don't support direct_IO. > > Users who use f_mapping->a_ops->direct_IO to check O_DIRECT support, > will read file through this way. This will cause overlayfs to access > a non-existent direct_IO function and cause panic due to null pointer: I just looked around the code more closely, in open_with_fake_path(), do_dentry_open() has already checked O_DIRECT open flag and a_ops->direct_IO of underlying real address_space. Am I missing something? Thanks, Chengguang > > Kernel panic - not syncing: CFI failure (target: 0x0) > CPU: 6 PID: 247 Comm: loop0 > Call Trace: > panic+0x188/0x45c > __cfi_slowpath+0x0/0x254 > __cfi_slowpath+0x200/0x254 > generic_file_read_iter+0x14c/0x150 > vfs_iocb_iter_read+0xac/0x164 > ovl_read_iter+0x13c/0x2fc > lo_rw_aio+0x2bc/0x458 > loop_queue_work+0x4a4/0xbc0 > kthread_worker_fn+0xf8/0x1d0 > loop_kthread_worker_fn+0x24/0x38 > kthread+0x29c/0x310 > ret_from_fork+0x10/0x30 > > The filesystem may only support direct_IO for some file types. For > example, erofs supports direct_IO for uncompressed files. So return > -EINVAL when the file doesn't support direct_IO to fix this problem. > > Fixes: 5b910bd615ba ("ovl: fix GPF in swapfile_activate of file from overlayfs over xfs") > Signed-off-by: Huang Jianan <huangjianan@oppo.com> > --- > change since v2: > - Return error in ovl_open directly. (Chengguang Xu) > > Change since v1: > - Return error to user rather than fall back to buffered io. (Chengguang Xu) > > fs/overlayfs/file.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c > index d081faa55e83..a0c99ea35daf 100644 > --- a/fs/overlayfs/file.c > +++ b/fs/overlayfs/file.c > @@ -157,6 +157,10 @@ static int ovl_open(struct inode *inode, struct file *file) > if (IS_ERR(realfile)) > return PTR_ERR(realfile); > > + if ((f->f_flags & O_DIRECT) && (!realfile->f_mapping->a_ops || > + !realfile->f_mapping->a_ops->direct_IO)) > + return -EINVAL; > + > file->private_data = realfile; > > return 0;
在 2021/9/22 16:06, Chengguang Xu 写道: > 在 2021/9/22 15:23, Huang Jianan 写道: >> From: Huang Jianan <huangjianan@oppo.com> >> >> At present, overlayfs provides overlayfs inode to users. Overlayfs >> inode provides ovl_aops with noop_direct_IO to avoid open failure >> with O_DIRECT. But some compressed filesystems, such as erofs and >> squashfs, don't support direct_IO. >> >> Users who use f_mapping->a_ops->direct_IO to check O_DIRECT support, >> will read file through this way. This will cause overlayfs to access >> a non-existent direct_IO function and cause panic due to null pointer: > > I just looked around the code more closely, in open_with_fake_path(), > > do_dentry_open() has already checked O_DIRECT open flag and > a_ops->direct_IO of underlying real address_space. > > Am I missing something? > > It seems that loop_update_dio will set lo->use_dio after open file without set O_DIRECT. loop_update_dio will check f_mapping->a_ops->direct_IO but it deal with ovl_aops with noop _direct_IO. So I think we still need a new aops? Thanks, Jianan > Thanks, > > Chengguang > > >> >> Kernel panic - not syncing: CFI failure (target: 0x0) >> CPU: 6 PID: 247 Comm: loop0 >> Call Trace: >> panic+0x188/0x45c >> __cfi_slowpath+0x0/0x254 >> __cfi_slowpath+0x200/0x254 >> generic_file_read_iter+0x14c/0x150 >> vfs_iocb_iter_read+0xac/0x164 >> ovl_read_iter+0x13c/0x2fc >> lo_rw_aio+0x2bc/0x458 >> loop_queue_work+0x4a4/0xbc0 >> kthread_worker_fn+0xf8/0x1d0 >> loop_kthread_worker_fn+0x24/0x38 >> kthread+0x29c/0x310 >> ret_from_fork+0x10/0x30 >> >> The filesystem may only support direct_IO for some file types. For >> example, erofs supports direct_IO for uncompressed files. So return >> -EINVAL when the file doesn't support direct_IO to fix this problem. >> >> Fixes: 5b910bd615ba ("ovl: fix GPF in swapfile_activate of file from >> overlayfs over xfs") >> Signed-off-by: Huang Jianan <huangjianan@oppo.com> >> --- >> change since v2: >> - Return error in ovl_open directly. (Chengguang Xu) >> >> Change since v1: >> - Return error to user rather than fall back to buffered io. >> (Chengguang Xu) >> >> fs/overlayfs/file.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c >> index d081faa55e83..a0c99ea35daf 100644 >> --- a/fs/overlayfs/file.c >> +++ b/fs/overlayfs/file.c >> @@ -157,6 +157,10 @@ static int ovl_open(struct inode *inode, struct >> file *file) >> if (IS_ERR(realfile)) >> return PTR_ERR(realfile); >> + if ((f->f_flags & O_DIRECT) && (!realfile->f_mapping->a_ops || >> + !realfile->f_mapping->a_ops->direct_IO)) >> + return -EINVAL; >> + >> file->private_data = realfile; >> return 0; >
在 2021/9/22 16:24, Huang Jianan 写道: > > > 在 2021/9/22 16:06, Chengguang Xu 写道: >> 在 2021/9/22 15:23, Huang Jianan 写道: >>> From: Huang Jianan <huangjianan@oppo.com> >>> >>> At present, overlayfs provides overlayfs inode to users. Overlayfs >>> inode provides ovl_aops with noop_direct_IO to avoid open failure >>> with O_DIRECT. But some compressed filesystems, such as erofs and >>> squashfs, don't support direct_IO. >>> >>> Users who use f_mapping->a_ops->direct_IO to check O_DIRECT support, >>> will read file through this way. This will cause overlayfs to access >>> a non-existent direct_IO function and cause panic due to null pointer: >> >> I just looked around the code more closely, in open_with_fake_path(), >> >> do_dentry_open() has already checked O_DIRECT open flag and >> a_ops->direct_IO of underlying real address_space. >> >> Am I missing something? >> >> > > It seems that loop_update_dio will set lo->use_dio after open file > without set O_DIRECT. > loop_update_dio will check f_mapping->a_ops->direct_IO but it deal > with ovl_aops with > noop _direct_IO. > > So I think we still need a new aops? It means we should only set ->direct_IO for overlayfs inodes whose underlying fs has DIRECT IO ability. Hi Miklos, Is it right solution for this kind of issue? What do you think? Thanks, Chengguang > > Thanks, > Jianan > >> Thanks, >> >> Chengguang >> >> >>> >>> Kernel panic - not syncing: CFI failure (target: 0x0) >>> CPU: 6 PID: 247 Comm: loop0 >>> Call Trace: >>> panic+0x188/0x45c >>> __cfi_slowpath+0x0/0x254 >>> __cfi_slowpath+0x200/0x254 >>> generic_file_read_iter+0x14c/0x150 >>> vfs_iocb_iter_read+0xac/0x164 >>> ovl_read_iter+0x13c/0x2fc >>> lo_rw_aio+0x2bc/0x458 >>> loop_queue_work+0x4a4/0xbc0 >>> kthread_worker_fn+0xf8/0x1d0 >>> loop_kthread_worker_fn+0x24/0x38 >>> kthread+0x29c/0x310 >>> ret_from_fork+0x10/0x30 >>> >>> The filesystem may only support direct_IO for some file types. For >>> example, erofs supports direct_IO for uncompressed files. So return >>> -EINVAL when the file doesn't support direct_IO to fix this problem. >>> >>> Fixes: 5b910bd615ba ("ovl: fix GPF in swapfile_activate of file from >>> overlayfs over xfs") >>> Signed-off-by: Huang Jianan <huangjianan@oppo.com> >>> --- >>> change since v2: >>> - Return error in ovl_open directly. (Chengguang Xu) >>> >>> Change since v1: >>> - Return error to user rather than fall back to buffered io. >>> (Chengguang Xu) >>> >>> fs/overlayfs/file.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c >>> index d081faa55e83..a0c99ea35daf 100644 >>> --- a/fs/overlayfs/file.c >>> +++ b/fs/overlayfs/file.c >>> @@ -157,6 +157,10 @@ static int ovl_open(struct inode *inode, struct >>> file *file) >>> if (IS_ERR(realfile)) >>> return PTR_ERR(realfile); >>> + if ((f->f_flags & O_DIRECT) && (!realfile->f_mapping->a_ops || >>> + !realfile->f_mapping->a_ops->direct_IO)) >>> + return -EINVAL; >>> + >>> file->private_data = realfile; >>> return 0; >> >
On Wed, 22 Sept 2021 at 15:21, Chengguang Xu <cgxu519@139.com> wrote: > > 在 2021/9/22 16:24, Huang Jianan 写道: > > > > > > 在 2021/9/22 16:06, Chengguang Xu 写道: > >> 在 2021/9/22 15:23, Huang Jianan 写道: > >>> From: Huang Jianan <huangjianan@oppo.com> > >>> > >>> At present, overlayfs provides overlayfs inode to users. Overlayfs > >>> inode provides ovl_aops with noop_direct_IO to avoid open failure > >>> with O_DIRECT. But some compressed filesystems, such as erofs and > >>> squashfs, don't support direct_IO. > >>> > >>> Users who use f_mapping->a_ops->direct_IO to check O_DIRECT support, > >>> will read file through this way. This will cause overlayfs to access > >>> a non-existent direct_IO function and cause panic due to null pointer: > >> > >> I just looked around the code more closely, in open_with_fake_path(), > >> > >> do_dentry_open() has already checked O_DIRECT open flag and > >> a_ops->direct_IO of underlying real address_space. > >> > >> Am I missing something? > >> > >> > > > > It seems that loop_update_dio will set lo->use_dio after open file > > without set O_DIRECT. > > loop_update_dio will check f_mapping->a_ops->direct_IO but it deal > > with ovl_aops with > > noop _direct_IO. > > > > So I think we still need a new aops? > > > It means we should only set ->direct_IO for overlayfs inodes whose > underlying fs has DIRECT IO ability. First let's fix the oops: ovl_read_iter()/ovl_write_iter() must check real file's ->direct_IO if IOCB_DIRECT is set in iocb->ki_flags and return -EINVAL if not. To fix the loop -> overlay -> squashfs case your suggestion of having separate aops depending on the real inode's ->direct_IO sounds good. Thanks, Miklos
On Wed, Sep 22, 2021 at 04:00:47PM +0200, Miklos Szeredi wrote: > First let's fix the oops: ovl_read_iter()/ovl_write_iter() must check > real file's ->direct_IO if IOCB_DIRECT is set in iocb->ki_flags and > return -EINVAL if not. And here's that fix. Please test. Thanks, Miklos --- From: Miklos Szeredi <mszeredi@redhat.com> Subject: ovl: fix IOCB_DIRECT if underlying fs doesn't support direct IO Normally the check at open time suffices, but e.g loop device does set IOCB_DIRECT after doing its own checks (which are not sufficent for overlayfs). Make sure we don't call the underlying filesystem read/write method with the IOCB_DIRECT if it's not supported. Reported-by: Huang Jianan <huangjianan@oppo.com> Fixes: 16914e6fc7e1 ("ovl: add ovl_read_iter()") Cc: <stable@vger.kernel.org> # v4.19 Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> --- fs/overlayfs/file.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) --- a/fs/overlayfs/file.c +++ b/fs/overlayfs/file.c @@ -296,6 +296,12 @@ static ssize_t ovl_read_iter(struct kioc if (ret) return ret; + ret = -EINVAL; + if (iocb->ki_flags & IOCB_DIRECT && + (!real.file->f_mapping->a_ops || + !real.file->f_mapping->a_ops->direct_IO)) + goto out_fdput; + old_cred = ovl_override_creds(file_inode(file)->i_sb); if (is_sync_kiocb(iocb)) { ret = vfs_iter_read(real.file, iter, &iocb->ki_pos, @@ -320,7 +326,7 @@ static ssize_t ovl_read_iter(struct kioc out: revert_creds(old_cred); ovl_file_accessed(file); - +out_fdput: fdput(real); return ret; @@ -349,6 +355,12 @@ static ssize_t ovl_write_iter(struct kio if (ret) goto out_unlock; + ret = -EINVAL; + if (iocb->ki_flags & IOCB_DIRECT && + (!real.file->f_mapping->a_ops || + !real.file->f_mapping->a_ops->direct_IO)) + goto out_fdput; + if (!ovl_should_sync(OVL_FS(inode->i_sb))) ifl &= ~(IOCB_DSYNC | IOCB_SYNC); @@ -384,6 +396,7 @@ static ssize_t ovl_write_iter(struct kio } out: revert_creds(old_cred); +out_fdput: fdput(real); out_unlock:
在 2021/9/27 17:38, Miklos Szeredi 写道: > On Wed, Sep 22, 2021 at 04:00:47PM +0200, Miklos Szeredi wrote: > >> First let's fix the oops: ovl_read_iter()/ovl_write_iter() must check >> real file's ->direct_IO if IOCB_DIRECT is set in iocb->ki_flags and >> return -EINVAL if not. > And here's that fix. Please test. This patch can fix the oops. Tested-by: Huang Jianan <huangjianan@oppo.com> Thanks, Jianan > Thanks, > Miklos > > --- > From: Miklos Szeredi <mszeredi@redhat.com> > Subject: ovl: fix IOCB_DIRECT if underlying fs doesn't support direct IO > > Normally the check at open time suffices, but e.g loop device does set > IOCB_DIRECT after doing its own checks (which are not sufficent for > overlayfs). > > Make sure we don't call the underlying filesystem read/write method with > the IOCB_DIRECT if it's not supported. > > Reported-by: Huang Jianan <huangjianan@oppo.com> > Fixes: 16914e6fc7e1 ("ovl: add ovl_read_iter()") > Cc: <stable@vger.kernel.org> # v4.19 > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > --- > fs/overlayfs/file.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > --- a/fs/overlayfs/file.c > +++ b/fs/overlayfs/file.c > @@ -296,6 +296,12 @@ static ssize_t ovl_read_iter(struct kioc > if (ret) > return ret; > > + ret = -EINVAL; > + if (iocb->ki_flags & IOCB_DIRECT && > + (!real.file->f_mapping->a_ops || > + !real.file->f_mapping->a_ops->direct_IO)) > + goto out_fdput; > + > old_cred = ovl_override_creds(file_inode(file)->i_sb); > if (is_sync_kiocb(iocb)) { > ret = vfs_iter_read(real.file, iter, &iocb->ki_pos, > @@ -320,7 +326,7 @@ static ssize_t ovl_read_iter(struct kioc > out: > revert_creds(old_cred); > ovl_file_accessed(file); > - > +out_fdput: > fdput(real); > > return ret; > @@ -349,6 +355,12 @@ static ssize_t ovl_write_iter(struct kio > if (ret) > goto out_unlock; > > + ret = -EINVAL; > + if (iocb->ki_flags & IOCB_DIRECT && > + (!real.file->f_mapping->a_ops || > + !real.file->f_mapping->a_ops->direct_IO)) > + goto out_fdput; > + > if (!ovl_should_sync(OVL_FS(inode->i_sb))) > ifl &= ~(IOCB_DSYNC | IOCB_SYNC); > > @@ -384,6 +396,7 @@ static ssize_t ovl_write_iter(struct kio > } > out: > revert_creds(old_cred); > +out_fdput: > fdput(real); > > out_unlock:
On Tue, 28 Sept 2021 at 09:01, Huang Jianan <huangjianan@oppo.com> wrote: > > 在 2021/9/27 17:38, Miklos Szeredi 写道: > > On Wed, Sep 22, 2021 at 04:00:47PM +0200, Miklos Szeredi wrote: > > > >> First let's fix the oops: ovl_read_iter()/ovl_write_iter() must check > >> real file's ->direct_IO if IOCB_DIRECT is set in iocb->ki_flags and > >> return -EINVAL if not. > > And here's that fix. Please test. > > This patch can fix the oops. > > Tested-by: Huang Jianan <huangjianan@oppo.com> Thanks for testing! Miklos
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c index d081faa55e83..a0c99ea35daf 100644 --- a/fs/overlayfs/file.c +++ b/fs/overlayfs/file.c @@ -157,6 +157,10 @@ static int ovl_open(struct inode *inode, struct file *file) if (IS_ERR(realfile)) return PTR_ERR(realfile); + if ((f->f_flags & O_DIRECT) && (!realfile->f_mapping->a_ops || + !realfile->f_mapping->a_ops->direct_IO)) + return -EINVAL; + file->private_data = realfile; return 0;