diff mbox series

[v2,08/10] ovl: do not fail because of O_NOATIME

Message ID 20201207163255.564116-9-mszeredi@redhat.com (mailing list archive)
State New
Headers show
Series allow unprivileged overlay mounts | expand

Commit Message

Miklos Szeredi Dec. 7, 2020, 4:32 p.m. UTC
In case the file cannot be opened with O_NOATIME because of lack of
capabilities, then clear O_NOATIME instead of failing.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/file.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Amir Goldstein Dec. 8, 2020, 11:29 a.m. UTC | #1
On Mon, Dec 7, 2020 at 6:37 PM Miklos Szeredi <mszeredi@redhat.com> wrote:
>
> In case the file cannot be opened with O_NOATIME because of lack of
> capabilities, then clear O_NOATIME instead of failing.
>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/overlayfs/file.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index dc767034d37b..d6ac7ac66410 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -53,9 +53,10 @@ static struct file *ovl_open_realfile(const struct file *file,
>         err = inode_permission(realinode, MAY_OPEN | acc_mode);
>         if (err) {
>                 realfile = ERR_PTR(err);
> -       } else if (!inode_owner_or_capable(realinode)) {
> -               realfile = ERR_PTR(-EPERM);
>         } else {
> +               if (!inode_owner_or_capable(realinode))
> +                       flags &= ~O_NOATIME;
> +

Isn't that going to break:

        flags |= OVL_OPEN_FLAGS;

        /* If some flag changed that cannot be changed then something's amiss */
        if (WARN_ON((file->f_flags ^ flags) & ~OVL_SETFL_MASK))

IOW setting a flag that is allowed to change will fail because of
missing O_ATIME in file->f_flags.

I guess we need test coverage for SETFL.

Thanks,
Amir.
Miklos Szeredi Dec. 11, 2020, 2:44 p.m. UTC | #2
On Tue, Dec 8, 2020 at 12:32 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Mon, Dec 7, 2020 at 6:37 PM Miklos Szeredi <mszeredi@redhat.com> wrote:
> >
> > In case the file cannot be opened with O_NOATIME because of lack of
> > capabilities, then clear O_NOATIME instead of failing.
> >
> > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> > ---
> >  fs/overlayfs/file.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> > index dc767034d37b..d6ac7ac66410 100644
> > --- a/fs/overlayfs/file.c
> > +++ b/fs/overlayfs/file.c
> > @@ -53,9 +53,10 @@ static struct file *ovl_open_realfile(const struct file *file,
> >         err = inode_permission(realinode, MAY_OPEN | acc_mode);
> >         if (err) {
> >                 realfile = ERR_PTR(err);
> > -       } else if (!inode_owner_or_capable(realinode)) {
> > -               realfile = ERR_PTR(-EPERM);
> >         } else {
> > +               if (!inode_owner_or_capable(realinode))
> > +                       flags &= ~O_NOATIME;
> > +
>
> Isn't that going to break:
>
>         flags |= OVL_OPEN_FLAGS;
>
>         /* If some flag changed that cannot be changed then something's amiss */
>         if (WARN_ON((file->f_flags ^ flags) & ~OVL_SETFL_MASK))
>
> IOW setting a flag that is allowed to change will fail because of
> missing O_ATIME in file->f_flags.

Well spotted.  I just removed those lines as a fix.   The check never
triggered since its introduction in 4.19, so I guess it isn't worth
adding more complexity for.

>
> I guess we need test coverage for SETFL.

There might be some in ltp, haven't checked.  Would be nice if the fs
related ltp tests could be integrated into xfstests.


Thanks,
Miklos
Amir Goldstein Dec. 14, 2020, 5:49 a.m. UTC | #3
On Fri, Dec 11, 2020 at 4:44 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Tue, Dec 8, 2020 at 12:32 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Mon, Dec 7, 2020 at 6:37 PM Miklos Szeredi <mszeredi@redhat.com> wrote:
> > >
> > > In case the file cannot be opened with O_NOATIME because of lack of
> > > capabilities, then clear O_NOATIME instead of failing.
> > >
> > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> > > ---
> > >  fs/overlayfs/file.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> > > index dc767034d37b..d6ac7ac66410 100644
> > > --- a/fs/overlayfs/file.c
> > > +++ b/fs/overlayfs/file.c
> > > @@ -53,9 +53,10 @@ static struct file *ovl_open_realfile(const struct file *file,
> > >         err = inode_permission(realinode, MAY_OPEN | acc_mode);
> > >         if (err) {
> > >                 realfile = ERR_PTR(err);
> > > -       } else if (!inode_owner_or_capable(realinode)) {
> > > -               realfile = ERR_PTR(-EPERM);
> > >         } else {
> > > +               if (!inode_owner_or_capable(realinode))
> > > +                       flags &= ~O_NOATIME;
> > > +
> >
> > Isn't that going to break:
> >
> >         flags |= OVL_OPEN_FLAGS;
> >
> >         /* If some flag changed that cannot be changed then something's amiss */
> >         if (WARN_ON((file->f_flags ^ flags) & ~OVL_SETFL_MASK))
> >
> > IOW setting a flag that is allowed to change will fail because of
> > missing O_ATIME in file->f_flags.
>
> Well spotted.  I just removed those lines as a fix.   The check never
> triggered since its introduction in 4.19, so I guess it isn't worth
> adding more complexity for.
>
> >
> > I guess we need test coverage for SETFL.
>
> There might be some in ltp, haven't checked.  Would be nice if the fs
> related ltp tests could be integrated into xfstests.
>

There is some test coverage for SETFL in xfstests.

The t_immutable tests for one, but those would not run if the mounter
has no CAP_LINUX_IMMUTABLE, so would not have been useful to
detect the problem above.

fsstress also seems to have support for SETFL ops, but I am not sure
in how many tests it is exercises and perhaps the relevant problem
would have been covered by some stress test that is not in the 'quick'
tests group.

Thanks,
Amir.
diff mbox series

Patch

diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index dc767034d37b..d6ac7ac66410 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -53,9 +53,10 @@  static struct file *ovl_open_realfile(const struct file *file,
 	err = inode_permission(realinode, MAY_OPEN | acc_mode);
 	if (err) {
 		realfile = ERR_PTR(err);
-	} else if (!inode_owner_or_capable(realinode)) {
-		realfile = ERR_PTR(-EPERM);
 	} else {
+		if (!inode_owner_or_capable(realinode))
+			flags &= ~O_NOATIME;
+
 		realfile = open_with_fake_path(&file->f_path, flags, realinode,
 					       current_cred());
 	}