| Message ID | 20190416162950.7845-1-christian.brauner@ubuntu.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,Disco] UBUNTU: SAUCE: shiftfs: use separate llseek method for directories | expand |
On Tue, Apr 16, 2019 at 06:29:50PM +0200, Christian Brauner wrote: > BugLink: https://bugs.launchpad.net/bugs/1824812 > > Give shiftfs it's own proper llseek method for directories. > > Before this commit we used to rely on an llseek method that was targeted > for regular files for both directories and regular files. Give > directories their own llseek operation. > This is required to have AppArmor function correctly with shiftfs. > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> Looks good, fixes reproducer provivded by tyhicks. Acked-by: Seth Forshee <seth.forshee@canonical.com>
On 2019-04-16 18:29:50, Christian Brauner wrote: > BugLink: https://bugs.launchpad.net/bugs/1824812 > > Give shiftfs it's own proper llseek method for directories. > > Before this commit we used to rely on an llseek method that was targeted > for regular files for both directories and regular files. Give > directories their own llseek operation. > This is required to have AppArmor function correctly with shiftfs. I think the commit message should be improved a little bit. This was a quick one that was thrown together while we were urgently debugging the bug report. The commit message is a bit misleading because the commit doesn't specifically fix AppArmor + shiftfs. It fixes shiftfs, in general, since it is common for userspace to expect lseek(2) to work on directory files. How about this: Give shiftfs it's own proper llseek method for directories. Before this commit we used to rely on an llseek method that was targeted for regular files for both directories and regular files. However, the realfile's f_pos was not correctly handled when userspace called lseek(2) on a shiftfs directory file. Give directories their own llseek operation so that seeking on a directory file is properly supported. I think that'll allow the commit message to age a little better once we've all forgotten the details of this bug. With that change, Acked-by: Tyler Hicks <tyhicks@canonical.com> Tyler > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> > --- > fs/shiftfs.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/fs/shiftfs.c b/fs/shiftfs.c > index 4c8a6ec2a617..9771165d1ce0 100644 > --- a/fs/shiftfs.c > +++ b/fs/shiftfs.c > @@ -1144,7 +1144,15 @@ static int shiftfs_release(struct inode *inode, struct file *file) > return 0; > } > > -static loff_t shiftfs_llseek(struct file *file, loff_t offset, int whence) > +static loff_t shiftfs_dir_llseek(struct file *file, loff_t offset, int whence) > +{ > + struct shiftfs_file_info *file_info = file->private_data; > + struct file *realfile = file_info->realfile; > + > + return vfs_llseek(realfile, offset, whence); > +} > + > +static loff_t shiftfs_file_llseek(struct file *file, loff_t offset, int whence) > { > struct inode *realinode = file_inode(file)->i_private; > > @@ -1653,7 +1661,7 @@ static int shiftfs_iterate_shared(struct file *file, struct dir_context *ctx) > const struct file_operations shiftfs_file_operations = { > .open = shiftfs_open, > .release = shiftfs_release, > - .llseek = shiftfs_llseek, > + .llseek = shiftfs_file_llseek, > .read_iter = shiftfs_read_iter, > .write_iter = shiftfs_write_iter, > .fsync = shiftfs_fsync, > @@ -1670,7 +1678,7 @@ const struct file_operations shiftfs_dir_operations = { > .compat_ioctl = shiftfs_compat_ioctl, > .fsync = shiftfs_fsync, > .iterate_shared = shiftfs_iterate_shared, > - .llseek = shiftfs_llseek, > + .llseek = shiftfs_dir_llseek, > .open = shiftfs_open, > .read = generic_read_dir, > .release = shiftfs_release, > -- > 2.21.0 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On Fri, Apr 19, 2019 at 11:34:45AM -0500, Tyler Hicks wrote: > On 2019-04-16 18:29:50, Christian Brauner wrote: > > BugLink: https://bugs.launchpad.net/bugs/1824812 > > > > Give shiftfs it's own proper llseek method for directories. > > > > Before this commit we used to rely on an llseek method that was targeted > > for regular files for both directories and regular files. Give > > directories their own llseek operation. > > This is required to have AppArmor function correctly with shiftfs. > > I think the commit message should be improved a little bit. This was a > quick one that was thrown together while we were urgently debugging the > bug report. > > The commit message is a bit misleading because the commit doesn't > specifically fix AppArmor + shiftfs. It fixes shiftfs, in general, > since it is common for userspace to expect lseek(2) to work on directory > files. How about this: > > Give shiftfs it's own proper llseek method for directories. > > Before this commit we used to rely on an llseek method that was > targeted for regular files for both directories and regular files. > However, the realfile's f_pos was not correctly handled when userspace > called lseek(2) on a shiftfs directory file. Give directories their > own llseek operation so that seeking on a directory file is properly > supported. > > I think that'll allow the commit message to age a little better once > we've all forgotten the details of this bug. With that change, > > Acked-by: Tyler Hicks <tyhicks@canonical.com> Sure, can you just drop-in your commit message instead of mine when you apply, please? Or do you want me to resend? > > Tyler > > > > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> > > --- > > fs/shiftfs.c | 14 +++++++++++--- > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/fs/shiftfs.c b/fs/shiftfs.c > > index 4c8a6ec2a617..9771165d1ce0 100644 > > --- a/fs/shiftfs.c > > +++ b/fs/shiftfs.c > > @@ -1144,7 +1144,15 @@ static int shiftfs_release(struct inode *inode, struct file *file) > > return 0; > > } > > > > -static loff_t shiftfs_llseek(struct file *file, loff_t offset, int whence) > > +static loff_t shiftfs_dir_llseek(struct file *file, loff_t offset, int whence) > > +{ > > + struct shiftfs_file_info *file_info = file->private_data; > > + struct file *realfile = file_info->realfile; > > + > > + return vfs_llseek(realfile, offset, whence); > > +} > > + > > +static loff_t shiftfs_file_llseek(struct file *file, loff_t offset, int whence) > > { > > struct inode *realinode = file_inode(file)->i_private; > > > > @@ -1653,7 +1661,7 @@ static int shiftfs_iterate_shared(struct file *file, struct dir_context *ctx) > > const struct file_operations shiftfs_file_operations = { > > .open = shiftfs_open, > > .release = shiftfs_release, > > - .llseek = shiftfs_llseek, > > + .llseek = shiftfs_file_llseek, > > .read_iter = shiftfs_read_iter, > > .write_iter = shiftfs_write_iter, > > .fsync = shiftfs_fsync, > > @@ -1670,7 +1678,7 @@ const struct file_operations shiftfs_dir_operations = { > > .compat_ioctl = shiftfs_compat_ioctl, > > .fsync = shiftfs_fsync, > > .iterate_shared = shiftfs_iterate_shared, > > - .llseek = shiftfs_llseek, > > + .llseek = shiftfs_dir_llseek, > > .open = shiftfs_open, > > .read = generic_read_dir, > > .release = shiftfs_release, > > -- > > 2.21.0 > > > > > > -- > > kernel-team mailing list > > kernel-team@lists.ubuntu.com > > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On 2019-04-19 18:39:46, Christian Brauner wrote: > On Fri, Apr 19, 2019 at 11:34:45AM -0500, Tyler Hicks wrote: > > On 2019-04-16 18:29:50, Christian Brauner wrote: > > > BugLink: https://bugs.launchpad.net/bugs/1824812 > > > > > > Give shiftfs it's own proper llseek method for directories. > > > > > > Before this commit we used to rely on an llseek method that was targeted > > > for regular files for both directories and regular files. Give > > > directories their own llseek operation. > > > This is required to have AppArmor function correctly with shiftfs. > > > > I think the commit message should be improved a little bit. This was a > > quick one that was thrown together while we were urgently debugging the > > bug report. > > > > The commit message is a bit misleading because the commit doesn't > > specifically fix AppArmor + shiftfs. It fixes shiftfs, in general, > > since it is common for userspace to expect lseek(2) to work on directory > > files. How about this: > > > > Give shiftfs it's own proper llseek method for directories. > > > > Before this commit we used to rely on an llseek method that was > > targeted for regular files for both directories and regular files. > > However, the realfile's f_pos was not correctly handled when userspace > > called lseek(2) on a shiftfs directory file. Give directories their > > own llseek operation so that seeking on a directory file is properly > > supported. > > > > I think that'll allow the commit message to age a little better once > > we've all forgotten the details of this bug. With that change, > > > > Acked-by: Tyler Hicks <tyhicks@canonical.com> > > Sure, can you just drop-in your commit message instead of mine when you > apply, please? Or do you want me to resend? I think the stable kernel team will drop-in my suggested commit message when they gather up and apply the acks. I don't think there's a need for you to resend. Tyler > > > > > Tyler > > > > > > > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> > > > --- > > > fs/shiftfs.c | 14 +++++++++++--- > > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > > > diff --git a/fs/shiftfs.c b/fs/shiftfs.c > > > index 4c8a6ec2a617..9771165d1ce0 100644 > > > --- a/fs/shiftfs.c > > > +++ b/fs/shiftfs.c > > > @@ -1144,7 +1144,15 @@ static int shiftfs_release(struct inode *inode, struct file *file) > > > return 0; > > > } > > > > > > -static loff_t shiftfs_llseek(struct file *file, loff_t offset, int whence) > > > +static loff_t shiftfs_dir_llseek(struct file *file, loff_t offset, int whence) > > > +{ > > > + struct shiftfs_file_info *file_info = file->private_data; > > > + struct file *realfile = file_info->realfile; > > > + > > > + return vfs_llseek(realfile, offset, whence); > > > +} > > > + > > > +static loff_t shiftfs_file_llseek(struct file *file, loff_t offset, int whence) > > > { > > > struct inode *realinode = file_inode(file)->i_private; > > > > > > @@ -1653,7 +1661,7 @@ static int shiftfs_iterate_shared(struct file *file, struct dir_context *ctx) > > > const struct file_operations shiftfs_file_operations = { > > > .open = shiftfs_open, > > > .release = shiftfs_release, > > > - .llseek = shiftfs_llseek, > > > + .llseek = shiftfs_file_llseek, > > > .read_iter = shiftfs_read_iter, > > > .write_iter = shiftfs_write_iter, > > > .fsync = shiftfs_fsync, > > > @@ -1670,7 +1678,7 @@ const struct file_operations shiftfs_dir_operations = { > > > .compat_ioctl = shiftfs_compat_ioctl, > > > .fsync = shiftfs_fsync, > > > .iterate_shared = shiftfs_iterate_shared, > > > - .llseek = shiftfs_llseek, > > > + .llseek = shiftfs_dir_llseek, > > > .open = shiftfs_open, > > > .read = generic_read_dir, > > > .release = shiftfs_release, > > > -- > > > 2.21.0 > > > > > > > > > -- > > > kernel-team mailing list > > > kernel-team@lists.ubuntu.com > > > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On 16.04.19 18:29, Christian Brauner wrote: > BugLink: https://bugs.launchpad.net/bugs/1824812 > > Give shiftfs it's own proper llseek method for directories. > > Before this commit we used to rely on an llseek method that was targeted > for regular files for both directories and regular files. Give > directories their own llseek operation. > This is required to have AppArmor function correctly with shiftfs. > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- With the improved commit message sent in another reply. > fs/shiftfs.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/fs/shiftfs.c b/fs/shiftfs.c > index 4c8a6ec2a617..9771165d1ce0 100644 > --- a/fs/shiftfs.c > +++ b/fs/shiftfs.c > @@ -1144,7 +1144,15 @@ static int shiftfs_release(struct inode *inode, struct file *file) > return 0; > } > > -static loff_t shiftfs_llseek(struct file *file, loff_t offset, int whence) > +static loff_t shiftfs_dir_llseek(struct file *file, loff_t offset, int whence) > +{ > + struct shiftfs_file_info *file_info = file->private_data; > + struct file *realfile = file_info->realfile; > + > + return vfs_llseek(realfile, offset, whence); > +} > + > +static loff_t shiftfs_file_llseek(struct file *file, loff_t offset, int whence) > { > struct inode *realinode = file_inode(file)->i_private; > > @@ -1653,7 +1661,7 @@ static int shiftfs_iterate_shared(struct file *file, struct dir_context *ctx) > const struct file_operations shiftfs_file_operations = { > .open = shiftfs_open, > .release = shiftfs_release, > - .llseek = shiftfs_llseek, > + .llseek = shiftfs_file_llseek, > .read_iter = shiftfs_read_iter, > .write_iter = shiftfs_write_iter, > .fsync = shiftfs_fsync, > @@ -1670,7 +1678,7 @@ const struct file_operations shiftfs_dir_operations = { > .compat_ioctl = shiftfs_compat_ioctl, > .fsync = shiftfs_fsync, > .iterate_shared = shiftfs_iterate_shared, > - .llseek = shiftfs_llseek, > + .llseek = shiftfs_dir_llseek, > .open = shiftfs_open, > .read = generic_read_dir, > .release = shiftfs_release, >
On 4/16/19 6:29 PM, Christian Brauner wrote: > BugLink: https://bugs.launchpad.net/bugs/1824812 > > Give shiftfs it's own proper llseek method for directories. > > Before this commit we used to rely on an llseek method that was targeted > for regular files for both directories and regular files. Give > directories their own llseek operation. > This is required to have AppArmor function correctly with shiftfs. > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> Applied to disco/master-next branch, changing the commit message to the one suggested by Tyler. Thanks, Kleber > --- > fs/shiftfs.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/fs/shiftfs.c b/fs/shiftfs.c > index 4c8a6ec2a617..9771165d1ce0 100644 > --- a/fs/shiftfs.c > +++ b/fs/shiftfs.c > @@ -1144,7 +1144,15 @@ static int shiftfs_release(struct inode *inode, struct file *file) > return 0; > } > > -static loff_t shiftfs_llseek(struct file *file, loff_t offset, int whence) > +static loff_t shiftfs_dir_llseek(struct file *file, loff_t offset, int whence) > +{ > + struct shiftfs_file_info *file_info = file->private_data; > + struct file *realfile = file_info->realfile; > + > + return vfs_llseek(realfile, offset, whence); > +} > + > +static loff_t shiftfs_file_llseek(struct file *file, loff_t offset, int whence) > { > struct inode *realinode = file_inode(file)->i_private; > > @@ -1653,7 +1661,7 @@ static int shiftfs_iterate_shared(struct file *file, struct dir_context *ctx) > const struct file_operations shiftfs_file_operations = { > .open = shiftfs_open, > .release = shiftfs_release, > - .llseek = shiftfs_llseek, > + .llseek = shiftfs_file_llseek, > .read_iter = shiftfs_read_iter, > .write_iter = shiftfs_write_iter, > .fsync = shiftfs_fsync, > @@ -1670,7 +1678,7 @@ const struct file_operations shiftfs_dir_operations = { > .compat_ioctl = shiftfs_compat_ioctl, > .fsync = shiftfs_fsync, > .iterate_shared = shiftfs_iterate_shared, > - .llseek = shiftfs_llseek, > + .llseek = shiftfs_dir_llseek, > .open = shiftfs_open, > .read = generic_read_dir, > .release = shiftfs_release, >
diff --git a/fs/shiftfs.c b/fs/shiftfs.c index 4c8a6ec2a617..9771165d1ce0 100644 --- a/fs/shiftfs.c +++ b/fs/shiftfs.c @@ -1144,7 +1144,15 @@ static int shiftfs_release(struct inode *inode, struct file *file) return 0; } -static loff_t shiftfs_llseek(struct file *file, loff_t offset, int whence) +static loff_t shiftfs_dir_llseek(struct file *file, loff_t offset, int whence) +{ + struct shiftfs_file_info *file_info = file->private_data; + struct file *realfile = file_info->realfile; + + return vfs_llseek(realfile, offset, whence); +} + +static loff_t shiftfs_file_llseek(struct file *file, loff_t offset, int whence) { struct inode *realinode = file_inode(file)->i_private; @@ -1653,7 +1661,7 @@ static int shiftfs_iterate_shared(struct file *file, struct dir_context *ctx) const struct file_operations shiftfs_file_operations = { .open = shiftfs_open, .release = shiftfs_release, - .llseek = shiftfs_llseek, + .llseek = shiftfs_file_llseek, .read_iter = shiftfs_read_iter, .write_iter = shiftfs_write_iter, .fsync = shiftfs_fsync, @@ -1670,7 +1678,7 @@ const struct file_operations shiftfs_dir_operations = { .compat_ioctl = shiftfs_compat_ioctl, .fsync = shiftfs_fsync, .iterate_shared = shiftfs_iterate_shared, - .llseek = shiftfs_llseek, + .llseek = shiftfs_dir_llseek, .open = shiftfs_open, .read = generic_read_dir, .release = shiftfs_release,
BugLink: https://bugs.launchpad.net/bugs/1824812 Give shiftfs it's own proper llseek method for directories. Before this commit we used to rely on an llseek method that was targeted for regular files for both directories and regular files. Give directories their own llseek operation. This is required to have AppArmor function correctly with shiftfs. Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> --- fs/shiftfs.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)