Received-SPF: pass (google.com: domain of linux-kernel+bounces-83455-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 27 Feb 2024 08:57:51 -0600
From: "Seth Forshee (DigitalOcean)" <sforshee@kernel.org>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Christian Brauner <brauner@kernel.org>, Serge Hallyn <serge@hallyn.com>,
	Paul Moore <paul@paul-moore.com>, Eric Paris <eparis@redhat.com>,
	James Morris <jmorris@namei.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>, Jan Kara <jack@suse.cz>,
	Stephen Smalley <stephen.smalley.work@gmail.com>,
	Ondrej Mosnacek <omosnace@redhat.com>,
	Casey Schaufler <casey@schaufler-ca.com>,
	Mimi Zohar <zohar@linux.ibm.com>,
	Roberto Sassu <roberto.sassu@huawei.com>,
	Dmitry Kasatkin <dmitry.kasatkin@gmail.com>,
	Eric Snowberg <eric.snowberg@oracle.com>,
	"Matthew Wilcox (Oracle)" <willy@infradead.org>,
	Jonathan Corbet <corbet@lwn.net>,
	Miklos Szeredi <miklos@szeredi.hu>, linux-kernel@vger.kernel.org,
	linux-fsdevel@vger.kernel.org,
	linux-security-module@vger.kernel.org, audit@vger.kernel.org,
	selinux@vger.kernel.org, linux-integrity@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-unionfs@vger.kernel.org
Subject: Re: [PATCH v2 20/25] ovl: add fscaps handlers
Message-ID: <Zd34b4Lw9hOHJYr2@do-x1extreme>
References: <20240221-idmap-fscap-refactor-v2-0-3039364623bd@kernel.org>
 <20240221-idmap-fscap-refactor-v2-20-3039364623bd@kernel.org>
 <CAOQ4uxjvrFuz2iCiO9dsOnear+qN=M+GFW-eEOZU5uCzBkTwLQ@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAOQ4uxjvrFuz2iCiO9dsOnear+qN=M+GFW-eEOZU5uCzBkTwLQ@mail.gmail.com>

On Tue, Feb 27, 2024 at 03:28:18PM +0200, Amir Goldstein wrote:
> > +int ovl_set_fscaps(struct mnt_idmap *idmap, struct dentry *dentry,
> > +                  const struct vfs_caps *caps, int setxattr_flags)
> > +{
> > +       int err;
> > +       struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
> > +       struct dentry *upperdentry = ovl_dentry_upper(dentry);
> > +       struct dentry *realdentry = upperdentry ?: ovl_dentry_lower(dentry);
> > +       const struct cred *old_cred;
> > +
> > +       /*
> > +        * If the fscaps are to be remove from a lower file, check that they
> > +        * exist before copying up.
> > +        */
> 
> Don't you need to convert -ENODATA to 0 return value in this case?

Do you mean that trying to remove an xattr that does not exist should
return 0? Standard behavior is to return -ENODATA in this situation.

> 
> > +       if (!caps && !upperdentry) {
> > +               struct path realpath;
> > +               struct vfs_caps lower_caps;
> > +
> > +               ovl_path_lower(dentry, &realpath);
> > +               old_cred = ovl_override_creds(dentry->d_sb);
> > +               err = vfs_get_fscaps(mnt_idmap(realpath.mnt), realdentry,
> > +                                    &lower_caps);
> > +               revert_creds(old_cred);
> > +               if (err)
> > +                       goto out;
> > +       }
> > +
> > +       err = ovl_want_write(dentry);
> > +       if (err)
> > +               goto out;
> > +
> 
> ovl_want_write() should after ovl_copy_up(), see:
> 162d06444070 ("ovl: reorder ovl_want_write() after ovl_inode_lock()")

Fixed.

> > @@ -758,6 +826,8 @@ static const struct inode_operations ovl_symlink_inode_operations = {
> >         .get_link       = ovl_get_link,
> >         .getattr        = ovl_getattr,
> >         .listxattr      = ovl_listxattr,
> > +       .get_fscaps     = ovl_get_fscaps,
> > +       .set_fscaps     = ovl_set_fscaps,
> >         .update_time    = ovl_update_time,
> >  };
> >
> > @@ -769,6 +839,8 @@ static const struct inode_operations ovl_special_inode_operations = {
> >         .get_inode_acl  = ovl_get_inode_acl,
> >         .get_acl        = ovl_get_acl,
> >         .set_acl        = ovl_set_acl,
> > +       .get_fscaps     = ovl_get_fscaps,
> > +       .set_fscaps     = ovl_set_fscaps,
> >         .update_time    = ovl_update_time,
> >  };
> >
> 
> 
> Sorry, I did not understand the explanation why fscaps ops are needed
> for non regular files. It does not look right to me.

The kernel does not forbid XATTR_NAME_CAPS for non-regular files and
will internally even try to read them from non-regular files during
killpriv checks. If we do not add handlers then we will end up using the
normal ovl xattr handlers, which call vfs_*xattr(). These will return an
error for fscaps xattrs after this series, which would be a change in
behavior for overlayfs and make it behave differently from other
filesystems.