| Message ID | 20200921072127.373125-1-yangx.jy@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ovl: Support FS_IOC_[SG]ETFLAGS and FS_IOC_FS[SG]ETXATTR ioctls on directories | expand |
On Mon, Sep 21, 2020 at 10:41 AM Xiao Yang <yangx.jy@cn.fujitsu.com> wrote: > > Factor out ovl_ioctl() and ovl_compat_ioctl() and take use of them for > directories. > > Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com> > --- This change is buggy. I had already posted it and self NACKed myself [1]. You can find an hopefully non-buggy version of it on my ovl-shutdown [2] branch. As long as you are changing ovl_ioctl(), please also take the second commit on that branch to replace the open coded capability check with the vfs_ioc_setflags_prepare() generic helper. Thanks, Amir. [1] https://lore.kernel.org/linux-unionfs/CAOQ4uxhRgL2sMok7xsAZN6cZXSfoPxx=O8ADE=72+Ta3hGoLbw@mail.gmail.com/ [2] https://github.com/amir73il/linux/commits/ovl-shutdown
On 2020/9/21 16:17, Amir Goldstein wrote: > On Mon, Sep 21, 2020 at 10:41 AM Xiao Yang<yangx.jy@cn.fujitsu.com> wrote: >> Factor out ovl_ioctl() and ovl_compat_ioctl() and take use of them for >> directories. >> >> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com> >> --- > This change is buggy. I had already posted it and self NACKed myself [1]. > > You can find an hopefully non-buggy version of it on my ovl-shutdown [2] branch. > > As long as you are changing ovl_ioctl(), please also take the second > commit on that > branch to replace the open coded capability check with the > vfs_ioc_setflags_prepare() > generic helper. Hi Amir, Thanks a lot for your quick reply. :-) I will try to read and understand the metioned patches on your ovl-shutdown branch. Best Regards, Xiao Yang > Thanks, > Amir. > > > [1] https://lore.kernel.org/linux-unionfs/CAOQ4uxhRgL2sMok7xsAZN6cZXSfoPxx=O8ADE=72+Ta3hGoLbw@mail.gmail.com/ > [2] https://github.com/amir73il/linux/commits/ovl-shutdown > > > . >
On Mon, Sep 21, 2020 at 11:55 AM Xiao Yang <yangx.jy@cn.fujitsu.com> wrote: > > On 2020/9/21 16:17, Amir Goldstein wrote: > > On Mon, Sep 21, 2020 at 10:41 AM Xiao Yang<yangx.jy@cn.fujitsu.com> wrote: > >> Factor out ovl_ioctl() and ovl_compat_ioctl() and take use of them for > >> directories. > >> > >> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com> > >> --- > > This change is buggy. I had already posted it and self NACKed myself [1]. > > > > You can find an hopefully non-buggy version of it on my ovl-shutdown [2] branch. > > > > As long as you are changing ovl_ioctl(), please also take the second > > commit on that > > branch to replace the open coded capability check with the > > vfs_ioc_setflags_prepare() > > generic helper. > Hi Amir, > > Thanks a lot for your quick reply. :-) > I will try to read and understand the metioned patches on your > ovl-shutdown branch. Please also verify my claim in the patch commit message, that the the test result of xfstest generic/079 changes from "notrun" to "success". Thanks, Amir.
On 2020/9/21 17:09, Amir Goldstein wrote: > On Mon, Sep 21, 2020 at 11:55 AM Xiao Yang<yangx.jy@cn.fujitsu.com> wrote: >> On 2020/9/21 16:17, Amir Goldstein wrote: >>> On Mon, Sep 21, 2020 at 10:41 AM Xiao Yang<yangx.jy@cn.fujitsu.com> wrote: >>>> Factor out ovl_ioctl() and ovl_compat_ioctl() and take use of them for >>>> directories. >>>> >>>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com> >>>> --- >>> This change is buggy. I had already posted it and self NACKed myself [1]. >>> >>> You can find an hopefully non-buggy version of it on my ovl-shutdown [2] branch. >>> >>> As long as you are changing ovl_ioctl(), please also take the second >>> commit on that >>> branch to replace the open coded capability check with the >>> vfs_ioc_setflags_prepare() >>> generic helper. >> Hi Amir, >> >> Thanks a lot for your quick reply. :-) >> I will try to read and understand the metioned patches on your >> ovl-shutdown branch. > Please also verify my claim in the patch commit message, that the > the test result of xfstest generic/079 changes from "notrun" to "success". Hi Amir, With your patches, I have confirmed that generic/079 actually changed from "notrun" to "success". Besides, one minor issue: Could we avoid the following compiler warning? ------------------------------------------------- fs/overlayfs/readdir.c: In function ‘ovl_dir_real_file’: fs/overlayfs/readdir.c:883:37: warning: passing argument 1 of ‘ovl_dir_open_realfile’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] 883 | realfile = ovl_dir_open_realfile(file, &upperpath); | ^~~~ fs/overlayfs/readdir.c:842:56: note: expected ‘struct file *’ but argument is of type ‘const struct file *’ 842 | static struct file *ovl_dir_open_realfile(struct file *file, | ~~~~~~~~~~~~~^~~~ ------------------------------------------------- Best Regards, Xiao Yang > Thanks, > Amir. > > > . >
On Tue, Sep 22, 2020 at 11:15 AM Xiao Yang <yangx.jy@cn.fujitsu.com> wrote: > > On 2020/9/21 17:09, Amir Goldstein wrote: > > On Mon, Sep 21, 2020 at 11:55 AM Xiao Yang<yangx.jy@cn.fujitsu.com> wrote: > >> On 2020/9/21 16:17, Amir Goldstein wrote: > >>> On Mon, Sep 21, 2020 at 10:41 AM Xiao Yang<yangx.jy@cn.fujitsu.com> wrote: > >>>> Factor out ovl_ioctl() and ovl_compat_ioctl() and take use of them for > >>>> directories. > >>>> > >>>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com> > >>>> --- > >>> This change is buggy. I had already posted it and self NACKed myself [1]. > >>> > >>> You can find an hopefully non-buggy version of it on my ovl-shutdown [2] branch. > >>> > >>> As long as you are changing ovl_ioctl(), please also take the second > >>> commit on that > >>> branch to replace the open coded capability check with the > >>> vfs_ioc_setflags_prepare() > >>> generic helper. > >> Hi Amir, > >> > >> Thanks a lot for your quick reply. :-) > >> I will try to read and understand the metioned patches on your > >> ovl-shutdown branch. > > Please also verify my claim in the patch commit message, that the > > the test result of xfstest generic/079 changes from "notrun" to "success". > Hi Amir, > > With your patches, I have confirmed that generic/079 actually changed from > "notrun" to "success". Besides, one minor issue: > Could we avoid the following compiler warning? > ------------------------------------------------- > fs/overlayfs/readdir.c: In function ‘ovl_dir_real_file’: > fs/overlayfs/readdir.c:883:37: warning: passing argument 1 of > ‘ovl_dir_open_realfile’ discards ‘const’ qualifier from pointer target > type [-Wdiscarded-qualifiers] > 883 | realfile = ovl_dir_open_realfile(file, &upperpath); > | ^~~~ > fs/overlayfs/readdir.c:842:56: note: expected ‘struct file *’ but > argument is of type ‘const struct file *’ > 842 | static struct file *ovl_dir_open_realfile(struct file *file, > | ~~~~~~~~~~~~~^~~~ > ------------------------------------------------- > Shouldn't be a problem to change ovl_dir_open_realfile() to take a const struct file * argument I think. Thanks, Amir.
On 2020/9/22 20:01, Amir Goldstein wrote: > On Tue, Sep 22, 2020 at 11:15 AM Xiao Yang<yangx.jy@cn.fujitsu.com> wrote: >> On 2020/9/21 17:09, Amir Goldstein wrote: >>> On Mon, Sep 21, 2020 at 11:55 AM Xiao Yang<yangx.jy@cn.fujitsu.com> wrote: >>>> On 2020/9/21 16:17, Amir Goldstein wrote: >>>>> On Mon, Sep 21, 2020 at 10:41 AM Xiao Yang<yangx.jy@cn.fujitsu.com> wrote: >>>>>> Factor out ovl_ioctl() and ovl_compat_ioctl() and take use of them for >>>>>> directories. >>>>>> >>>>>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com> >>>>>> --- >>>>> This change is buggy. I had already posted it and self NACKed myself [1]. >>>>> >>>>> You can find an hopefully non-buggy version of it on my ovl-shutdown [2] branch. >>>>> >>>>> As long as you are changing ovl_ioctl(), please also take the second >>>>> commit on that >>>>> branch to replace the open coded capability check with the >>>>> vfs_ioc_setflags_prepare() >>>>> generic helper. >>>> Hi Amir, >>>> >>>> Thanks a lot for your quick reply. :-) >>>> I will try to read and understand the metioned patches on your >>>> ovl-shutdown branch. >>> Please also verify my claim in the patch commit message, that the >>> the test result of xfstest generic/079 changes from "notrun" to "success". >> Hi Amir, >> >> With your patches, I have confirmed that generic/079 actually changed from >> "notrun" to "success". Besides, one minor issue: >> Could we avoid the following compiler warning? >> ------------------------------------------------- >> fs/overlayfs/readdir.c: In function ‘ovl_dir_real_file’: >> fs/overlayfs/readdir.c:883:37: warning: passing argument 1 of >> ‘ovl_dir_open_realfile’ discards ‘const’ qualifier from pointer target >> type [-Wdiscarded-qualifiers] >> 883 | realfile = ovl_dir_open_realfile(file,&upperpath); >> | ^~~~ >> fs/overlayfs/readdir.c:842:56: note: expected ‘struct file *’ but >> argument is of type ‘const struct file *’ >> 842 | static struct file *ovl_dir_open_realfile(struct file *file, >> | ~~~~~~~~~~~~~^~~~ >> ------------------------------------------------- >> > Shouldn't be a problem to change ovl_dir_open_realfile() > to take a const struct file * argument I think. Hi Amir, Other than the compiler warning I tested your patches on our enviroment and didn't find any issue, so add: Reviewed-by: Xiao Yang <yangx.jy@.cn.fujisu.com> Thank you for sharing these patches again. :-) Best Regards, Xiao Yang > Thanks, > Amir. > > > . >
On Fri, Sep 25, 2020 at 6:13 PM Xiao Yang <yangx.jy@cn.fujitsu.com> wrote: > > On 2020/9/22 20:01, Amir Goldstein wrote: > > On Tue, Sep 22, 2020 at 11:15 AM Xiao Yang<yangx.jy@cn.fujitsu.com> wrote: > >> On 2020/9/21 17:09, Amir Goldstein wrote: > >>> On Mon, Sep 21, 2020 at 11:55 AM Xiao Yang<yangx.jy@cn.fujitsu.com> wrote: > >>>> On 2020/9/21 16:17, Amir Goldstein wrote: > >>>>> On Mon, Sep 21, 2020 at 10:41 AM Xiao Yang<yangx.jy@cn.fujitsu.com> wrote: > >>>>>> Factor out ovl_ioctl() and ovl_compat_ioctl() and take use of them for > >>>>>> directories. > >>>>>> > >>>>>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com> > >>>>>> --- > >>>>> This change is buggy. I had already posted it and self NACKed myself [1]. > >>>>> > >>>>> You can find an hopefully non-buggy version of it on my ovl-shutdown [2] branch. > >>>>> > >>>>> As long as you are changing ovl_ioctl(), please also take the second > >>>>> commit on that > >>>>> branch to replace the open coded capability check with the > >>>>> vfs_ioc_setflags_prepare() > >>>>> generic helper. > >>>> Hi Amir, > >>>> > >>>> Thanks a lot for your quick reply. :-) > >>>> I will try to read and understand the metioned patches on your > >>>> ovl-shutdown branch. > >>> Please also verify my claim in the patch commit message, that the > >>> the test result of xfstest generic/079 changes from "notrun" to "success". > >> Hi Amir, > >> > >> With your patches, I have confirmed that generic/079 actually changed from > >> "notrun" to "success". Besides, one minor issue: > >> Could we avoid the following compiler warning? > >> ------------------------------------------------- > >> fs/overlayfs/readdir.c: In function ‘ovl_dir_real_file’: > >> fs/overlayfs/readdir.c:883:37: warning: passing argument 1 of > >> ‘ovl_dir_open_realfile’ discards ‘const’ qualifier from pointer target > >> type [-Wdiscarded-qualifiers] > >> 883 | realfile = ovl_dir_open_realfile(file,&upperpath); > >> | ^~~~ > >> fs/overlayfs/readdir.c:842:56: note: expected ‘struct file *’ but > >> argument is of type ‘const struct file *’ > >> 842 | static struct file *ovl_dir_open_realfile(struct file *file, > >> | ~~~~~~~~~~~~~^~~~ > >> ------------------------------------------------- > >> > > Shouldn't be a problem to change ovl_dir_open_realfile() > > to take a const struct file * argument I think. > Hi Amir, > > Other than the compiler warning I tested your patches on our > enviroment and didn't find any issue, so add: > Reviewed-by: Xiao Yang <yangx.jy@.cn.fujisu.com> > > Thank you for sharing these patches again. :-) Please post the fixed patches rebased on top of git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-next. Please leave my signed-off-by and add your own as well. Thanks, Amir.
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c index 0d940e29d62b..94ad7f9c9c76 100644 --- a/fs/overlayfs/file.c +++ b/fs/overlayfs/file.c @@ -640,7 +640,7 @@ static long ovl_ioctl_set_fsxflags(struct file *file, unsigned int cmd, ovl_fsxflags_to_iflags(fa.fsx_xflags)); } -static long ovl_ioctl(struct file *file, unsigned int cmd, unsigned long arg) +long ovl_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { long ret; @@ -665,8 +665,7 @@ static long ovl_ioctl(struct file *file, unsigned int cmd, unsigned long arg) return ret; } -static long ovl_compat_ioctl(struct file *file, unsigned int cmd, - unsigned long arg) +long ovl_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { switch (cmd) { case FS_IOC32_GETFLAGS: diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h index 29bc1ec699e7..bfb499314dcc 100644 --- a/fs/overlayfs/overlayfs.h +++ b/fs/overlayfs/overlayfs.h @@ -301,6 +301,8 @@ bool ovl_is_metacopy_dentry(struct dentry *dentry); char *ovl_get_redirect_xattr(struct dentry *dentry, int padding); ssize_t ovl_getxattr(struct dentry *dentry, char *name, char **value, size_t padding); +long ovl_ioctl(struct file *file, unsigned int cmd, unsigned long arg); +long ovl_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg); static inline bool ovl_is_impuredir(struct dentry *dentry) { diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c index 6918b98faeb6..cde909b6fe7d 100644 --- a/fs/overlayfs/readdir.c +++ b/fs/overlayfs/readdir.c @@ -945,6 +945,8 @@ const struct file_operations ovl_dir_operations = { .llseek = ovl_dir_llseek, .fsync = ovl_dir_fsync, .release = ovl_dir_release, + .unlocked_ioctl = ovl_ioctl, + .compat_ioctl = ovl_compat_ioctl, }; int ovl_check_empty_dir(struct dentry *dentry, struct list_head *list)
Factor out ovl_ioctl() and ovl_compat_ioctl() and take use of them for directories. Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com> --- fs/overlayfs/file.c | 5 ++--- fs/overlayfs/overlayfs.h | 2 ++ fs/overlayfs/readdir.c | 2 ++ 3 files changed, 6 insertions(+), 3 deletions(-)