From: Huang Jianan <huangjianan@oppo.com> To: Chengguang Xu <cgxu519@139.com>, linux-unionfs@vger.kernel.org, miklos@szeredi.hu, linux-erofs@lists.ozlabs.org, xiang@kernel.org, chao@kernel.org Cc: guoweichao@oppo.com, yh@oppo.com, zhangshiming@oppo.com, guanyuwei@oppo.com, jnhuang95@gmail.com, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, cgxu519@mykernel.net Subject: Re: [PATCH v2] ovl: fix null pointer when filesystem doesn't support direct IO Date: Wed, 22 Sep 2021 15:18:36 +0800 [thread overview] Message-ID: <9ef909de-1854-b4be-d272-2b4cda52329f@oppo.com> (raw) In-Reply-To: <4ccc5c89-eb13-5e91-9283-c94f755a9c17@139.com> 在 2021/9/22 13:09, Chengguang Xu 写道: > 在 2021/9/22 11:47, Huang Jianan 写道: >> 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: >> >> 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 reset >> f_mapping->a_ops to NULL 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 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..38118d3b46f8 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)) >> + file->f_mapping->a_ops = NULL; > > > There are many other functions in a_ops and also address_space struct > will be shared > > between files which belong to same inode. Although overlayfs currently > only defines > > ->direct_IO in a_ops, it will be extended in the future. (like > containerized sycnfs [1]) > > > It seems the simplest solution is directly return error to upper layer. > I think that after reset a_ops, do_dentry_open will check f_mapping->a_ops->direct_IO and return error. But return error directly in ovl_open seems to be a better solution, and won't affect future extend of ovl_aops. Thanks for pointing this out. Thanks, Jianan > > Thanks, > > Chengguang > > > [1] > https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Flinux-unionfs%2Fmsg08569.html&data=04%7C01%7Chuangjianan%40oppo.com%7Ce01c8bb9ad4e4ac2670008d97d87321c%7Cf1905eb1c35341c5951662b4a54b5ee6%7C0%7C0%7C637678842352759179%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Wo2uMfbYhqDOzDPSwHci2AVtM9y9nNstmayb741gspQ%3D&reserved=0 > > > >> + >> file->private_data = realfile; >> return 0; >
WARNING: multiple messages have this Message-ID (diff)
From: Huang Jianan via Linux-erofs <linux-erofs@lists.ozlabs.org> To: Chengguang Xu <cgxu519@139.com>, linux-unionfs@vger.kernel.org, miklos@szeredi.hu, linux-erofs@lists.ozlabs.org, xiang@kernel.org, chao@kernel.org Cc: zhangshiming@oppo.com, linux-kernel@vger.kernel.org, cgxu519@mykernel.net, yh@oppo.com, guanyuwei@oppo.com, linux-fsdevel@vger.kernel.org, guoweichao@oppo.com Subject: Re: [PATCH v2] ovl: fix null pointer when filesystem doesn't support direct IO Date: Wed, 22 Sep 2021 15:18:36 +0800 [thread overview] Message-ID: <9ef909de-1854-b4be-d272-2b4cda52329f@oppo.com> (raw) In-Reply-To: <4ccc5c89-eb13-5e91-9283-c94f755a9c17@139.com> 在 2021/9/22 13:09, Chengguang Xu 写道: > 在 2021/9/22 11:47, Huang Jianan 写道: >> 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: >> >> 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 reset >> f_mapping->a_ops to NULL 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 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..38118d3b46f8 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)) >> + file->f_mapping->a_ops = NULL; > > > There are many other functions in a_ops and also address_space struct > will be shared > > between files which belong to same inode. Although overlayfs currently > only defines > > ->direct_IO in a_ops, it will be extended in the future. (like > containerized sycnfs [1]) > > > It seems the simplest solution is directly return error to upper layer. > I think that after reset a_ops, do_dentry_open will check f_mapping->a_ops->direct_IO and return error. But return error directly in ovl_open seems to be a better solution, and won't affect future extend of ovl_aops. Thanks for pointing this out. Thanks, Jianan > > Thanks, > > Chengguang > > > [1] > https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Flinux-unionfs%2Fmsg08569.html&data=04%7C01%7Chuangjianan%40oppo.com%7Ce01c8bb9ad4e4ac2670008d97d87321c%7Cf1905eb1c35341c5951662b4a54b5ee6%7C0%7C0%7C637678842352759179%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Wo2uMfbYhqDOzDPSwHci2AVtM9y9nNstmayb741gspQ%3D&reserved=0 > > > >> + >> file->private_data = realfile; >> return 0; >
next prev parent reply other threads:[~2021-09-22 7:18 UTC|newest] Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-09-18 12:13 [PATCH] ovl: fix null pointer when filesystem doesn't support direct IO Huang Jianan 2021-09-18 12:13 ` Huang Jianan via Linux-erofs 2021-09-22 1:56 ` Chengguang Xu 2021-09-22 1:56 ` Chengguang Xu 2021-09-22 3:39 ` Huang Jianan 2021-09-22 3:39 ` Huang Jianan via Linux-erofs 2021-09-22 3:47 ` [PATCH v2] " Huang Jianan 2021-09-22 3:47 ` Huang Jianan via Linux-erofs 2021-09-22 5:09 ` Chengguang Xu 2021-09-22 5:09 ` Chengguang Xu 2021-09-22 7:18 ` Huang Jianan [this message] 2021-09-22 7:18 ` Huang Jianan via Linux-erofs 2021-09-22 7:23 ` [PATCH v3] " Huang Jianan 2021-09-22 7:23 ` Huang Jianan via Linux-erofs 2021-09-22 8:06 ` Chengguang Xu 2021-09-22 8:06 ` Chengguang Xu 2021-09-22 8:24 ` Huang Jianan 2021-09-22 8:24 ` Huang Jianan via Linux-erofs 2021-09-22 13:20 ` [PATCH v3] ovl: fix null pointer when filesystemdoesn'tsupportdirect IO Chengguang Xu 2021-09-22 13:20 ` Chengguang Xu 2021-09-22 14:00 ` Miklos Szeredi 2021-09-22 14:00 ` Miklos Szeredi 2021-09-27 9:38 ` Miklos Szeredi 2021-09-27 9:38 ` Miklos Szeredi 2021-09-28 7:01 ` Huang Jianan 2021-09-28 7:01 ` Huang Jianan via Linux-erofs 2021-09-28 7:17 ` Miklos Szeredi 2021-09-28 7:17 ` Miklos Szeredi
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=9ef909de-1854-b4be-d272-2b4cda52329f@oppo.com \ --to=huangjianan@oppo.com \ --cc=cgxu519@139.com \ --cc=cgxu519@mykernel.net \ --cc=chao@kernel.org \ --cc=guanyuwei@oppo.com \ --cc=guoweichao@oppo.com \ --cc=jnhuang95@gmail.com \ --cc=linux-erofs@lists.ozlabs.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-unionfs@vger.kernel.org \ --cc=miklos@szeredi.hu \ --cc=xiang@kernel.org \ --cc=yh@oppo.com \ --cc=zhangshiming@oppo.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.