On Wed, Aug 7, 2024 at 8:30 PM Jan Kara <jack@xxxxxxx> wrote:
>
> sb_start_write() will be returning error for a shutdown filesystem.
> Teach all ovl_start_write() to handle the error and bail out.
>
> Signed-off-by: Jan Kara <jack@xxxxxxx>
> ---
> fs/overlayfs/copy_up.c | 42 +++++++++++++++++++++++++++++++---------
> fs/overlayfs/overlayfs.h | 2 +-
> fs/overlayfs/util.c | 3 ++-
> 3 files changed, 36 insertions(+), 11 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index a5ef2005a2cc..6ebfd9c7b260 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -584,7 +584,9 @@ static int ovl_link_up(struct ovl_copy_up_ctx *c)
> struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
> struct inode *udir = d_inode(upperdir);
>
> - ovl_start_write(c->dentry);
> + err = ovl_start_write(c->dentry);
> + if (err)
> + return err;
>
> /* Mark parent "impure" because it may now contain non-pure upper */
> err = ovl_set_impure(c->parent, upperdir);
> @@ -744,6 +746,7 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> struct path path = { .mnt = ovl_upper_mnt(ofs) };
> struct dentry *temp, *upper, *trap;
> struct ovl_cu_creds cc;
> + bool frozen = false;
frozen is not a descriptive name for taking sb_writers?
> int err;
> struct ovl_cattr cattr = {
> /* Can't properly set mode on creation because of the umask */
> @@ -756,7 +759,11 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> if (err)
> return err;
>
> - ovl_start_write(c->dentry);
> + err = ovl_start_write(c->dentry);
> + if (err) {
> + ovl_revert_cu_creds(&cc);
> + return err;
> + }
> inode_lock(wdir);
> temp = ovl_create_temp(ofs, c->workdir, &cattr);
> inode_unlock(wdir);
> @@ -778,7 +785,10 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> * ovl_copy_up_data(), so lock workdir and destdir and make sure that
> * temp wasn't moved before copy up completion or cleanup.
> */
> - ovl_start_write(c->dentry);
> + if (!err) {
This is confusing, I admit, but !err is not checked because ovl_cleanup()
needs sb_writers held.
Suggest something like:
err2 = ovl_start_write(c->dentry);
if (err2) {
dput(temp);
return err ?: err2;
}
> + err = ovl_start_write(c->dentry);
> + frozen = !err;
> + }
> trap = lock_rename(c->workdir, c->destdir);
> if (trap || temp->d_parent != c->workdir) {
> /* temp or workdir moved underneath us? abort without cleanup */
> @@ -827,7 +837,8 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> unlock:
> unlock_rename(c->workdir, c->destdir);
> out:
> - ovl_end_write(c->dentry);
> + if (frozen)
> + ovl_end_write(c->dentry);
>
> return err;
>
> @@ -851,7 +862,11 @@ static int ovl_copy_up_tmpfile(struct ovl_copy_up_ctx *c)
> if (err)
> return err;
>
> - ovl_start_write(c->dentry);
> + err = ovl_start_write(c->dentry);
> + if (err) {
> + ovl_revert_cu_creds(&cc);
> + return err;
> + }
> tmpfile = ovl_do_tmpfile(ofs, c->workdir, c->stat.mode);
> ovl_end_write(c->dentry);
> ovl_revert_cu_creds(&cc);
> @@ -865,7 +880,9 @@ static int ovl_copy_up_tmpfile(struct ovl_copy_up_ctx *c)
> goto out_fput;
> }
>
> - ovl_start_write(c->dentry);
> + err = ovl_start_write(c->dentry);
> + if (err)
> + goto out_fput;
>
> err = ovl_copy_up_metadata(c, temp);
> if (err)
> @@ -964,7 +981,9 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
> * Mark parent "impure" because it may now contain non-pure
> * upper
> */
> - ovl_start_write(c->dentry);
> + err = ovl_start_write(c->dentry);
> + if (err)
> + goto out_free_fh;
> err = ovl_set_impure(c->parent, c->destdir);
> ovl_end_write(c->dentry);
> if (err)
> @@ -982,7 +1001,9 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
> if (c->indexed)
> ovl_set_flag(OVL_INDEX, d_inode(c->dentry));
>
> - ovl_start_write(c->dentry);
> + err = ovl_start_write(c->dentry);
> + if (err)
> + goto out;
> if (to_index) {
> /* Initialize nlink for copy up of disconnected dentry */
> err = ovl_set_nlink_upper(c->dentry);
> @@ -1088,7 +1109,10 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
> * Writing to upper file will clear security.capability xattr. We
> * don't want that to happen for normal copy-up operation.
> */
> - ovl_start_write(c->dentry);
> + err = ovl_start_write(c->dentry);
> + if (err)
> + goto out_free;
> +
> if (capability) {
> err = ovl_do_setxattr(ofs, upperpath.dentry, XATTR_NAME_CAPS,
> capability, cap_size, 0);
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 0bfe35da4b7b..ee8f2b28159a 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -423,7 +423,7 @@ static inline int ovl_do_getattr(const struct path *path, struct kstat *stat,
> /* util.c */
> int ovl_get_write_access(struct dentry *dentry);
> void ovl_put_write_access(struct dentry *dentry);
> -void ovl_start_write(struct dentry *dentry);
> +int __must_check ovl_start_write(struct dentry *dentry);
> void ovl_end_write(struct dentry *dentry);
> int ovl_want_write(struct dentry *dentry);
> void ovl_drop_write(struct dentry *dentry);
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index edc9216f6e27..b53fa14506a9 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -25,10 +25,11 @@ int ovl_get_write_access(struct dentry *dentry)
> }
>
> /* Get write access to upper sb - may block if upper sb is frozen */
> -void ovl_start_write(struct dentry *dentry)
> +int __must_check ovl_start_write(struct dentry *dentry)
> {
> struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
> sb_start_write(ovl_upper_mnt(ofs)->mnt_sb);
> + return 0;
> }
Is this an unintentional omission of sb_start_write() return value or
fixed later on?
Thanks,
Amir.
