Re: Patch "ovl: fix missing upper fs freeze protection on copy up for ioctl" has been added to the 4.19-stable tree

Re: Patch "ovl: fix missing upper fs freeze protection on copy up for ioctl" has been added to the 4.19-stable tree

[Amir Goldstein]

On Thu, May 23, 2019 at 12:30 PM <gregkh@linuxfoundation.org> wrote:
>
>
> This is a note to let you know that I've just added the patch titled
>
>     ovl: fix missing upper fs freeze protection on copy up for ioctl
>
> to the 4.19-stable tree which can be found at:
>     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
>
> The filename of the patch is:
>      ovl-fix-missing-upper-fs-freeze-protection-on-copy-up-for-ioctl.patch
> and it can be found in the queue-4.19 subdirectory.
>
> If you, or anyone else, feels it should not be added to the stable tree,
> please let <stable@vger.kernel.org> know about it.
>
>
> From 3428030da004a1128cbdcf93dc03e16f184d845b Mon Sep 17 00:00:00 2001
> From: Amir Goldstein <amir73il@gmail.com>
> Date: Tue, 22 Jan 2019 07:01:39 +0200
> Subject: ovl: fix missing upper fs freeze protection on copy up for ioctl
>
> From: Amir Goldstein <amir73il@gmail.com>
>
> commit 3428030da004a1128cbdcf93dc03e16f184d845b upstream.
>
> Generalize the helper ovl_open_maybe_copy_up() and use it to copy up file
> with data before FS_IOC_SETFLAGS ioctl.
>
> The FS_IOC_SETFLAGS ioctl is a bit of an odd ball in vfs, which probably
> caused the confusion.  File may be open O_RDONLY, but ioctl modifies the
> file.  VFS does not call mnt_want_write_file() nor lock inode mutex, but
> fs-specific code for FS_IOC_SETFLAGS does.  So ovl_ioctl() calls
> mnt_want_write_file() for the overlay file, and fs-specific code calls
> mnt_want_write_file() for upper fs file, but there was no call for
> ovl_want_write() for copy up duration which prevents overlayfs from copying
> up on a frozen upper fs.
>
> Fixes: dab5ca8fd9dd ("ovl: add lsattr/chattr support")
> Cc: <stable@vger.kernel.org> # v4.19
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> Acked-by: Vivek Goyal <vgoyal@redhat.com>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> ---
>  fs/overlayfs/copy_up.c   |    6 +++---
>  fs/overlayfs/file.c      |    5 ++---
>  fs/overlayfs/overlayfs.h |    2 +-
>  3 files changed, 6 insertions(+), 7 deletions(-)
>
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -878,14 +878,14 @@ static bool ovl_open_need_copy_up(struct
>         return true;
>  }
>
> -int ovl_open_maybe_copy_up(struct dentry *dentry, unsigned int file_flags)
> +int ovl_maybe_copy_up(struct dentry *dentry, int flags)
>  {
>         int err = 0;
>
> -       if (ovl_open_need_copy_up(dentry, file_flags)) {
> +       if (ovl_open_need_copy_up(dentry, flags)) {
>                 err = ovl_want_write(dentry);
>                 if (!err) {
> -                       err = ovl_copy_up_flags(dentry, file_flags);
> +                       err = ovl_copy_up_flags(dentry, flags);
>                         ovl_drop_write(dentry);
>                 }
>         }
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -116,11 +116,10 @@ static int ovl_real_fdget(const struct f
>
>  static int ovl_open(struct inode *inode, struct file *file)
>  {
> -       struct dentry *dentry = file_dentry(file);
>         struct file *realfile;
>         int err;
>
> -       err = ovl_open_maybe_copy_up(dentry, file->f_flags);
> +       err = ovl_maybe_copy_up(file_dentry(file), file->f_flags);
>         if (err)
>                 return err;
>
> @@ -390,7 +389,7 @@ static long ovl_ioctl(struct file *file,
>                 if (ret)
>                         return ret;
>
> -               ret = ovl_copy_up_with_data(file_dentry(file));
> +               ret = ovl_maybe_copy_up(file_dentry(file), O_WRONLY);
>                 if (!ret) {
>                         ret = ovl_real_ioctl(file, cmd, arg);
>
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -411,7 +411,7 @@ extern const struct file_operations ovl_
>  int ovl_copy_up(struct dentry *dentry);
>  int ovl_copy_up_with_data(struct dentry *dentry);
>  int ovl_copy_up_flags(struct dentry *dentry, int flags);
> -int ovl_open_maybe_copy_up(struct dentry *dentry, unsigned int file_flags);
> +int ovl_maybe_copy_up(struct dentry *dentry, int flags);
>  int ovl_copy_xattr(struct dentry *old, struct dentry *new);
>  int ovl_set_attr(struct dentry *upper, struct kstat *stat);
>  struct ovl_fh *ovl_encode_real_fh(struct dentry *real, bool is_upper);
>
>
> Patches currently in stable-queue which might be from amir73il@gmail.com are
>
> queue-4.19/ovl-fix-missing-upper-fs-freeze-protection-on-copy-up-for-ioctl.patch

This patch is fine for stable, but I have a process question.
All these patches from overlayfs 5.2-rc1 are also v4.9 stable candidates:

1. acf3062a7e1c - ovl: relax WARN_ON() for overlapping layers use case
2. 98487de318a6 - ovl: check the capability before cred overridden
3. d989903058a8 - ovl: do not generate duplicate fsnotify events for "fake" path
4. 9e46b840c705 - ovl: support stacked SEEK_HOLE/SEEK_DATA

#2 wasn't properly marked for stable, but the other are marked with
Fixes: and Reported-by:

Are those marks not sufficient to get selected for stable trees these days?

I must admit that #1 in borderline stable. Not sure if eliminating an unjust
WARN_ON qualified, but syzbot did report a bug..

Just asking in order to improve the process, but in any case,
please pick those patches for v4.9+ (unless anyone objects?)
They all already have LTP/xfstests/syzkaller tests that cover them.

Thanks,
Amir.

Re: Patch "ovl: fix missing upper fs freeze protection on copy up for ioctl" has been added to the 4.19-stable tree

[Greg KH]

On Thu, May 23, 2019 at 10:51:58PM +0300, Amir Goldstein wrote:
> On Thu, May 23, 2019 at 12:30 PM <gregkh@linuxfoundation.org> wrote:
> >
> >
> > This is a note to let you know that I've just added the patch titled
> >
> >     ovl: fix missing upper fs freeze protection on copy up for ioctl
> >
> > to the 4.19-stable tree which can be found at:
> >     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
> >
> > The filename of the patch is:
> >      ovl-fix-missing-upper-fs-freeze-protection-on-copy-up-for-ioctl.patch
> > and it can be found in the queue-4.19 subdirectory.
> >
> > If you, or anyone else, feels it should not be added to the stable tree,
> > please let <stable@vger.kernel.org> know about it.
> >
> >
> > From 3428030da004a1128cbdcf93dc03e16f184d845b Mon Sep 17 00:00:00 2001
> > From: Amir Goldstein <amir73il@gmail.com>
> > Date: Tue, 22 Jan 2019 07:01:39 +0200
> > Subject: ovl: fix missing upper fs freeze protection on copy up for ioctl
> >
> > From: Amir Goldstein <amir73il@gmail.com>
> >
> > commit 3428030da004a1128cbdcf93dc03e16f184d845b upstream.
> >
> > Generalize the helper ovl_open_maybe_copy_up() and use it to copy up file
> > with data before FS_IOC_SETFLAGS ioctl.
> >
> > The FS_IOC_SETFLAGS ioctl is a bit of an odd ball in vfs, which probably
> > caused the confusion.  File may be open O_RDONLY, but ioctl modifies the
> > file.  VFS does not call mnt_want_write_file() nor lock inode mutex, but
> > fs-specific code for FS_IOC_SETFLAGS does.  So ovl_ioctl() calls
> > mnt_want_write_file() for the overlay file, and fs-specific code calls
> > mnt_want_write_file() for upper fs file, but there was no call for
> > ovl_want_write() for copy up duration which prevents overlayfs from copying
> > up on a frozen upper fs.
> >
> > Fixes: dab5ca8fd9dd ("ovl: add lsattr/chattr support")
> > Cc: <stable@vger.kernel.org> # v4.19
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > Acked-by: Vivek Goyal <vgoyal@redhat.com>
> > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >
> > ---
> >  fs/overlayfs/copy_up.c   |    6 +++---
> >  fs/overlayfs/file.c      |    5 ++---
> >  fs/overlayfs/overlayfs.h |    2 +-
> >  3 files changed, 6 insertions(+), 7 deletions(-)
> >
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -878,14 +878,14 @@ static bool ovl_open_need_copy_up(struct
> >         return true;
> >  }
> >
> > -int ovl_open_maybe_copy_up(struct dentry *dentry, unsigned int file_flags)
> > +int ovl_maybe_copy_up(struct dentry *dentry, int flags)
> >  {
> >         int err = 0;
> >
> > -       if (ovl_open_need_copy_up(dentry, file_flags)) {
> > +       if (ovl_open_need_copy_up(dentry, flags)) {
> >                 err = ovl_want_write(dentry);
> >                 if (!err) {
> > -                       err = ovl_copy_up_flags(dentry, file_flags);
> > +                       err = ovl_copy_up_flags(dentry, flags);
> >                         ovl_drop_write(dentry);
> >                 }
> >         }
> > --- a/fs/overlayfs/file.c
> > +++ b/fs/overlayfs/file.c
> > @@ -116,11 +116,10 @@ static int ovl_real_fdget(const struct f
> >
> >  static int ovl_open(struct inode *inode, struct file *file)
> >  {
> > -       struct dentry *dentry = file_dentry(file);
> >         struct file *realfile;
> >         int err;
> >
> > -       err = ovl_open_maybe_copy_up(dentry, file->f_flags);
> > +       err = ovl_maybe_copy_up(file_dentry(file), file->f_flags);
> >         if (err)
> >                 return err;
> >
> > @@ -390,7 +389,7 @@ static long ovl_ioctl(struct file *file,
> >                 if (ret)
> >                         return ret;
> >
> > -               ret = ovl_copy_up_with_data(file_dentry(file));
> > +               ret = ovl_maybe_copy_up(file_dentry(file), O_WRONLY);
> >                 if (!ret) {
> >                         ret = ovl_real_ioctl(file, cmd, arg);
> >
> > --- a/fs/overlayfs/overlayfs.h
> > +++ b/fs/overlayfs/overlayfs.h
> > @@ -411,7 +411,7 @@ extern const struct file_operations ovl_
> >  int ovl_copy_up(struct dentry *dentry);
> >  int ovl_copy_up_with_data(struct dentry *dentry);
> >  int ovl_copy_up_flags(struct dentry *dentry, int flags);
> > -int ovl_open_maybe_copy_up(struct dentry *dentry, unsigned int file_flags);
> > +int ovl_maybe_copy_up(struct dentry *dentry, int flags);
> >  int ovl_copy_xattr(struct dentry *old, struct dentry *new);
> >  int ovl_set_attr(struct dentry *upper, struct kstat *stat);
> >  struct ovl_fh *ovl_encode_real_fh(struct dentry *real, bool is_upper);
> >
> >
> > Patches currently in stable-queue which might be from amir73il@gmail.com are
> >
> > queue-4.19/ovl-fix-missing-upper-fs-freeze-protection-on-copy-up-for-ioctl.patch
> 
> This patch is fine for stable, but I have a process question.
> All these patches from overlayfs 5.2-rc1 are also v4.9 stable candidates:
> 
> 1. acf3062a7e1c - ovl: relax WARN_ON() for overlapping layers use case
> 2. 98487de318a6 - ovl: check the capability before cred overridden
> 3. d989903058a8 - ovl: do not generate duplicate fsnotify events for "fake" path
> 4. 9e46b840c705 - ovl: support stacked SEEK_HOLE/SEEK_DATA
> 
> #2 wasn't properly marked for stable, but the other are marked with
> Fixes: and Reported-by:
> 
> Are those marks not sufficient to get selected for stable trees these days?

Not by default, no.  Sometimes they might get picked up if we get bored,
or the auto-bot notices them.

> I must admit that #1 in borderline stable. Not sure if eliminating an unjust
> WARN_ON qualified, but syzbot did report a bug..

syzbot things are good to fix in stable kernels, so that syzbot can
continue to find real things in stable kernels.  So yes, that is fine to
backport.

> Just asking in order to improve the process, but in any case,
> please pick those patches for v4.9+ (unless anyone objects?)
> They all already have LTP/xfstests/syzkaller tests that cover them.

I'll queue them up for the next round after this, thanks.

greg k-h

Re: Patch "ovl: fix missing upper fs freeze protection on copy up for ioctl" has been added to the 4.19-stable tree

[Amir Goldstein]

> > This patch is fine for stable, but I have a process question.
> > All these patches from overlayfs 5.2-rc1 are also v4.9 stable candidates:
> >
> > 1. acf3062a7e1c - ovl: relax WARN_ON() for overlapping layers use case
> > 2. 98487de318a6 - ovl: check the capability before cred overridden
> > 3. d989903058a8 - ovl: do not generate duplicate fsnotify events for "fake" path
> > 4. 9e46b840c705 - ovl: support stacked SEEK_HOLE/SEEK_DATA
> >
> > #2 wasn't properly marked for stable, but the other are marked with
> > Fixes: and Reported-by:
> >
> > Are those marks not sufficient to get selected for stable trees these days?
>
> Not by default, no.  Sometimes they might get picked up if we get bored,
> or the auto-bot notices them.
>
> > I must admit that #1 in borderline stable. Not sure if eliminating an unjust
> > WARN_ON qualified, but syzbot did report a bug..
>
> syzbot things are good to fix in stable kernels, so that syzbot can
> continue to find real things in stable kernels.  So yes, that is fine to
> backport.
>
> > Just asking in order to improve the process, but in any case,
> > please pick those patches for v4.9+ (unless anyone objects?)
> > They all already have LTP/xfstests/syzkaller tests that cover them.
>
> I'll queue them up for the next round after this, thanks.
>

Hi Greg,

I forgot to follow up on those patches.
Now I look at linux-4.19.y, I only see patch #1 (ovl: relax WARN_ON()..)
and not the 3 other patches I listed as stable candidates.
Was there any issue with those patches?

Thanks,
Amir.

Re: Patch "ovl: fix missing upper fs freeze protection on copy up for ioctl" has been added to the 4.19-stable tree

[Greg KH]

On Mon, Jun 10, 2019 at 11:28:46PM +0300, Amir Goldstein wrote:
> > > This patch is fine for stable, but I have a process question.
> > > All these patches from overlayfs 5.2-rc1 are also v4.9 stable candidates:
> > >
> > > 1. acf3062a7e1c - ovl: relax WARN_ON() for overlapping layers use case
> > > 2. 98487de318a6 - ovl: check the capability before cred overridden
> > > 3. d989903058a8 - ovl: do not generate duplicate fsnotify events for "fake" path
> > > 4. 9e46b840c705 - ovl: support stacked SEEK_HOLE/SEEK_DATA
> > >
> > > #2 wasn't properly marked for stable, but the other are marked with
> > > Fixes: and Reported-by:
> > >
> > > Are those marks not sufficient to get selected for stable trees these days?
> >
> > Not by default, no.  Sometimes they might get picked up if we get bored,
> > or the auto-bot notices them.
> >
> > > I must admit that #1 in borderline stable. Not sure if eliminating an unjust
> > > WARN_ON qualified, but syzbot did report a bug..
> >
> > syzbot things are good to fix in stable kernels, so that syzbot can
> > continue to find real things in stable kernels.  So yes, that is fine to
> > backport.
> >
> > > Just asking in order to improve the process, but in any case,
> > > please pick those patches for v4.9+ (unless anyone objects?)
> > > They all already have LTP/xfstests/syzkaller tests that cover them.
> >
> > I'll queue them up for the next round after this, thanks.
> >
> 
> Hi Greg,
> 
> I forgot to follow up on those patches.
> Now I look at linux-4.19.y, I only see patch #1 (ovl: relax WARN_ON()..)
> and not the 3 other patches I listed as stable candidates.
> Was there any issue with those patches?

Sorry, didn't get to them.

I did now, and they all do not apply to all kernel versions.  Most of
them do not go back to 4.14 or 4.9 as the code just isn't there.

So, after this next round of kernel releases, can you send backported
versions of any missing patches so that we are sure to apply them
correctly?

thanks,

greg k-h

Re: Patch "ovl: fix missing upper fs freeze protection on copy up for ioctl" has been added to the 4.19-stable tree

[Amir Goldstein]

On Thu, Jun 13, 2019 at 10:48 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jun 10, 2019 at 11:28:46PM +0300, Amir Goldstein wrote:
> > > > This patch is fine for stable, but I have a process question.
> > > > All these patches from overlayfs 5.2-rc1 are also v4.9 stable candidates:
> > > >
> > > > 1. acf3062a7e1c - ovl: relax WARN_ON() for overlapping layers use case
> > > > 2. 98487de318a6 - ovl: check the capability before cred overridden
> > > > 3. d989903058a8 - ovl: do not generate duplicate fsnotify events for "fake" path
> > > > 4. 9e46b840c705 - ovl: support stacked SEEK_HOLE/SEEK_DATA
> > > >
> > > > #2 wasn't properly marked for stable, but the other are marked with
> > > > Fixes: and Reported-by:
> > > >
> > > > Are those marks not sufficient to get selected for stable trees these days?
> > >
> > > Not by default, no.  Sometimes they might get picked up if we get bored,
> > > or the auto-bot notices them.
> > >
> > > > I must admit that #1 in borderline stable. Not sure if eliminating an unjust
> > > > WARN_ON qualified, but syzbot did report a bug..
> > >
> > > syzbot things are good to fix in stable kernels, so that syzbot can
> > > continue to find real things in stable kernels.  So yes, that is fine to
> > > backport.
> > >
> > > > Just asking in order to improve the process, but in any case,
> > > > please pick those patches for v4.9+ (unless anyone objects?)
> > > > They all already have LTP/xfstests/syzkaller tests that cover them.
> > >
> > > I'll queue them up for the next round after this, thanks.
> > >
> >
> > Hi Greg,
> >
> > I forgot to follow up on those patches.
> > Now I look at linux-4.19.y, I only see patch #1 (ovl: relax WARN_ON()..)
> > and not the 3 other patches I listed as stable candidates.
> > Was there any issue with those patches?
>
> Sorry, didn't get to them.
>
> I did now, and they all do not apply to all kernel versions.  Most of
> them do not go back to 4.14 or 4.9 as the code just isn't there.
>
> So, after this next round of kernel releases, can you send backported
> versions of any missing patches so that we are sure to apply them
> correctly?
>

Is happens to be that all those patches you picked since 4.19
most of the, whether tagged properly or not are:
Fixes: d1d04ef8572b ("ovl: stack file ops")
Cc: <stable@vger.kernel.org> # v4.19

There was a very big change in v4.19 ("ovl: stack file ops")
removing lots of "VFS hacks" and resulting in more than
one behavior change. Things that used to work pre-4.19
with the "VFS hacks" that do not work with "ovl: stack file ops".

There have been very few overlayfs patches since v4.20,
only a trickle of v4.19 stable fixes.

Also, the code has changes quite considerably since 4.14, so
overlayfs patches that could be easily backported, even if
applicable to 4.14 are rare these days.

Thanks!
Amir.