diff mbox series

[07/13] overlayfs: Make ovl_start_write() return error

Message ID 20240807183003.23562-7-jack@suse.cz (mailing list archive)
State New
Headers show
Series fs: generic filesystem shutdown handling | expand

Commit Message

Jan Kara Aug. 7, 2024, 6:29 p.m. UTC 
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@suse.cz>
---
 fs/overlayfs/copy_up.c   | 42 +++++++++++++++++++++++++++++++---------
 fs/overlayfs/overlayfs.h |  2 +-
 fs/overlayfs/util.c      |  3 ++-
 3 files changed, 36 insertions(+), 11 deletions(-)

Comments

Amir Goldstein Aug. 8, 2024, 12:01 p.m. UTC | #1 
On Wed, Aug 7, 2024 at 8:30 PM Jan Kara <jack@suse.cz> 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@suse.cz>
> ---
>  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.
diff mbox series

Patch

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;
 	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) {
+		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;
 }
 
 int ovl_want_write(struct dentry *dentry)