On Sat, 27 Apr 2024 at 13:58, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> There are some tests in src/vfs/vfstest that run sgid tests
> with O_TMPFILE if it is supported.
> I identified generic/696 and generic/697, but only the latter
> currently runs on overlayfs.
Yes, I ran full xfstests, but now I also verified that they do use
O_TMPFILE on overlayfs and it works.
The only one that didn't run from these was:
generic/509 [not run] require /scratch to be valid block disk
> > + path_get(user_path);
> > + *backing_file_user_path(f) = *user_path;
> > + error = vfs_tmpfile(real_idmap, real_parentpath, f, mode);
>
> We do not have a custom idmap in other backing_file helpers
> don't see why we need real_idmap in this helper.
> I think that should be:
> mnt_idmap(real_parentpath.mnt)
Yes.
> > + realfile = backing_tmpfile_open(&file->f_path, flags,
> > + &nop_mnt_idmap, &realparentpath, mode,
> > + current_cred());
>
> Using &nop_mnt_idmap here is not only unneeded but also looks wrong.
Yep, no need to pass idmap down this helper, since it can be extracted
form realparentpath.
> Any reason not to reuse ovl_instantiate() to avoid duplicating some
> of the subtlety? for example:
>
> + /* ovl_instantiate() consumes the .upperdentry reference on success */
> + dget(realfile->f_path.dentry)
> + err = ovl_instantiate(dentry, inode, realfile->f_path.dentry, 0, 1);
> + if (err)
> + goto out_err;
>
> [...]
>
> static int ovl_instantiate(struct dentry *dentry, struct inode *inode,
> - struct dentry *newdentry, bool hardlink)
> + struct dentry *newdentry, bool hardlink,
> bool tmpfile)
> {
> struct ovl_inode_params oip = {
> .upperdentry = newdentry,
> @@ -295,6 +295,9 @@ static int ovl_instantiate(struct dentry *dentry,
> struct inode *inode,
> inc_nlink(inode);
> }
>
> + if (tmpfile)
> + d_mark_tmpfile(dentry);
> +
> d_instantiate(dentry, inode);
>
Okay, makes sense.
> > +static int ovl_create_tmpfile(struct file *file, struct dentry *dentry,
> > + struct inode *inode, umode_t mode)
> > +{
> > + int err;
> > + const struct cred *old_cred;
> > + struct cred *override_cred;
> > +
> > + err = ovl_copy_up(dentry->d_parent);
> > + if (err)
> > + return err;
> > +
> > + old_cred = ovl_override_creds(dentry->d_sb);
> > +
> > + err = -ENOMEM;
> > + override_cred = prepare_creds();
> > + if (override_cred) {
> > + override_cred->fsuid = inode->i_uid;
> > + override_cred->fsgid = inode->i_gid;
> > + err = security_dentry_create_files_as(dentry, mode,
> > + &dentry->d_name, old_cred,
> > + override_cred);
> > + if (err) {
> > + put_cred(override_cred);
> > + goto out_revert_creds;
> > + }
> > + put_cred(override_creds(override_cred));
> > + put_cred(override_cred);
> > +
> > + err = ovl_create_upper_tmpfile(file, dentry, inode, mode);
> > + }
> > +out_revert_creds:
> > + revert_creds(old_cred);
> > + return err;
> > +}
>
> This also shouts unneeded and subtle code duplication to me:
>
> static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
> - struct ovl_cattr *attr, bool origin)
> + struct ovl_cattr *attr, bool origin,
> + struct file *tmpfile)
> {
> int err;
> const struct cred *old_cred;
> @@ -602,7 +606,9 @@ static int ovl_create_or_link(struct dentry
> *dentry, struct inode *inode,
> put_cred(override_cred);
> }
>
> - if (!ovl_dentry_is_whiteout(dentry))
> + if (tmpfile)
> + err = ovl_create_upper_tmpfile(tmpfile, dentry, inode,
> attr->mode);
> + else if (!ovl_dentry_is_whiteout(dentry))
> err = ovl_create_upper(dentry, inode, attr);
> else
> err = ovl_create_over_whiteout(dentry, inode, attr);
Instead I opted to extract the creds preparation into a separate helper.
> > + /* inode reference was transferred to dentry */
> > + inode = NULL;
> > + err = finish_open(file, dentry, ovl_dummy_open);
> > +put_realfile:
> > + if (!(file->f_mode & FMODE_OPENED))
> > + fput(file->private_data);
>
> This cleanup bit is very subtle and hard for me to review.
> I wonder if there is a way to improve this subtlety?
The reason I did it this way is because fput() code checks
FMODE_OPENED and calls ->release() only if it's set. So essentially
it's an indication whether the VFS will initiate the cleanup or we
need to do that ourselves.
> Would it be possible to write this cleanup as:
> + if (err && file->private_data)
> + fput(file->private_data);
>
> With a comment explaining where file->private_data is set?
If you look at do_dentry_open() there's an error condition after
setting FMODE_OPENED, that would result in the above being wrong. In
fact that error condition cannot happen in overlayfs, due to aops
being set just for this reason. So checking the error works in our
case, but I think that's more subtle than the FMODE_OPENED check. A
comment would make sense, though.
>
> Overall, I did not find any bugs, but I am hoping that the code could be
> a bit easier to review and maintain.
Thanks for the review, will post a new version shortly.
Thanks,
Miklos
