* [PATCH v2 0/3] overlayfs lock ordering changes
@ 2023-08-14 14:05 Amir Goldstein
2023-08-14 14:05 ` [PATCH v2 1/3] ovl: reorder ovl_want_write() after ovl_inode_lock() Amir Goldstein
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Amir Goldstein @ 2023-08-14 14:05 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Jan Kara, linux-unionfs
Hi Miklos,
These patches are needed for my start-write-safe series [1].
I think that at least patch 3 fixes a real bug.
Thanks,
Amir.
Changes since v1:
- Breakup the large ovl_want_write() transaction in copy up
- Add fix to possible deadlock with encode lower ovl fh
[1] https://github.com/amir73il/linux/commits/start-write-safe
Amir Goldstein (3):
ovl: reorder ovl_want_write() after ovl_inode_lock()
ovl: do not open/llseek lower file with upper sb_writers held
ovl: do not encode lower fh with upper sb_writers held
fs/overlayfs/copy_up.c | 195 ++++++++++++++++++++++++++-------------
fs/overlayfs/dir.c | 71 +++++++-------
fs/overlayfs/export.c | 7 +-
fs/overlayfs/inode.c | 56 ++++++-----
fs/overlayfs/namei.c | 37 ++++++--
fs/overlayfs/overlayfs.h | 26 ++++--
fs/overlayfs/super.c | 20 ++--
fs/overlayfs/util.c | 16 ++++
8 files changed, 268 insertions(+), 160 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 1/3] ovl: reorder ovl_want_write() after ovl_inode_lock()
2023-08-14 14:05 [PATCH v2 0/3] overlayfs lock ordering changes Amir Goldstein
@ 2023-08-14 14:05 ` Amir Goldstein
2023-08-15 10:50 ` Miklos Szeredi
2023-08-14 14:05 ` [PATCH v2 2/3] ovl: do not open/llseek lower file with upper sb_writers held Amir Goldstein
2023-08-14 14:05 ` [PATCH v2 3/3] ovl: do not encode lower fh " Amir Goldstein
2 siblings, 1 reply; 15+ messages in thread
From: Amir Goldstein @ 2023-08-14 14:05 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Jan Kara, linux-unionfs
Make the locking order of ovl_inode_lock() strictly between the two
vfs stacked layers, i.e.:
- ovl vfs locks: sb_writers, inode_lock, ...
- ovl_inode_lock
- upper vfs locks: sb_writers, inode_lock, ...
To that effect, move ovl_want_write() into the helpers ovl_nlink_start()
and ovl_copy_up_one() which currently take the ovl_inode_lock() after
ovl_want_write().
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
fs/overlayfs/copy_up.c | 36 ++++++++++-----------
fs/overlayfs/dir.c | 71 ++++++++++++++++++------------------------
fs/overlayfs/export.c | 7 +----
fs/overlayfs/inode.c | 56 ++++++++++++++++-----------------
fs/overlayfs/util.c | 7 +++++
5 files changed, 83 insertions(+), 94 deletions(-)
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index bae404a1bad4..c998dab440f8 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -1085,15 +1085,22 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
if (unlikely(err)) {
if (err > 0)
err = 0;
- } else {
- if (!ovl_dentry_upper(dentry))
- err = ovl_do_copy_up(&ctx);
- if (!err && parent && !ovl_dentry_has_upper_alias(dentry))
- err = ovl_link_up(&ctx);
- if (!err && ovl_dentry_needs_data_copy_up_locked(dentry, flags))
- err = ovl_copy_up_meta_inode_data(&ctx);
- ovl_copy_up_end(dentry);
+ goto out;
}
+
+ err = ovl_want_write(dentry);
+ if (err)
+ goto out;
+
+ if (!ovl_dentry_upper(dentry))
+ err = ovl_do_copy_up(&ctx);
+ if (!err && parent && !ovl_dentry_has_upper_alias(dentry))
+ err = ovl_link_up(&ctx);
+ if (!err && ovl_dentry_needs_data_copy_up_locked(dentry, flags))
+ err = ovl_copy_up_meta_inode_data(&ctx);
+ ovl_drop_write(dentry);
+ ovl_copy_up_end(dentry);
+out:
do_delayed_call(&done);
return err;
@@ -1169,17 +1176,10 @@ static bool ovl_open_need_copy_up(struct dentry *dentry, int flags)
int ovl_maybe_copy_up(struct dentry *dentry, int flags)
{
- int err = 0;
-
- if (ovl_open_need_copy_up(dentry, flags)) {
- err = ovl_want_write(dentry);
- if (!err) {
- err = ovl_copy_up_flags(dentry, flags);
- ovl_drop_write(dentry);
- }
- }
+ if (!ovl_open_need_copy_up(dentry, flags))
+ return 0;
- return err;
+ return ovl_copy_up_flags(dentry, flags);
}
int ovl_copy_up_with_data(struct dentry *dentry)
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 033fc0458a3d..f01031fe7b97 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -559,10 +559,6 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
struct cred *override_cred;
struct dentry *parent = dentry->d_parent;
- err = ovl_copy_up(parent);
- if (err)
- return err;
-
old_cred = ovl_override_creds(dentry->d_sb);
/*
@@ -626,15 +622,11 @@ static int ovl_create_object(struct dentry *dentry, int mode, dev_t rdev,
.link = link,
};
- err = ovl_want_write(dentry);
- if (err)
- goto out;
-
/* Preallocate inode to be used by ovl_get_inode() */
err = -ENOMEM;
inode = ovl_new_inode(dentry->d_sb, mode, rdev);
if (!inode)
- goto out_drop_write;
+ goto out;
spin_lock(&inode->i_lock);
inode->i_state |= I_CREATING;
@@ -643,12 +635,19 @@ static int ovl_create_object(struct dentry *dentry, int mode, dev_t rdev,
inode_init_owner(&nop_mnt_idmap, inode, dentry->d_parent->d_inode, mode);
attr.mode = inode->i_mode;
+ err = ovl_copy_up(dentry->d_parent);
+ if (err)
+ return err;
+
+ err = ovl_want_write(dentry);
+ if (err)
+ goto out;
+
err = ovl_create_or_link(dentry, inode, &attr, false);
/* Did we end up using the preallocated inode? */
if (inode != d_inode(dentry))
iput(inode);
-out_drop_write:
ovl_drop_write(dentry);
out:
return err;
@@ -700,28 +699,24 @@ static int ovl_link(struct dentry *old, struct inode *newdir,
int err;
struct inode *inode;
- err = ovl_want_write(old);
+ err = ovl_copy_up(old);
if (err)
goto out;
- err = ovl_copy_up(old);
+ err = ovl_copy_up(new->d_parent);
if (err)
- goto out_drop_write;
+ goto out;
- err = ovl_copy_up(new->d_parent);
+ err = ovl_nlink_start(old);
if (err)
- goto out_drop_write;
+ goto out;
if (ovl_is_metacopy_dentry(old)) {
err = ovl_set_link_redirect(old);
if (err)
- goto out_drop_write;
+ goto out_nlink_end;
}
- err = ovl_nlink_start(old);
- if (err)
- goto out_drop_write;
-
inode = d_inode(old);
ihold(inode);
@@ -731,9 +726,8 @@ static int ovl_link(struct dentry *old, struct inode *newdir,
if (err)
iput(inode);
+out_nlink_end:
ovl_nlink_end(old);
-out_drop_write:
- ovl_drop_write(old);
out:
return err;
}
@@ -891,17 +885,13 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir)
goto out;
}
- err = ovl_want_write(dentry);
- if (err)
- goto out;
-
err = ovl_copy_up(dentry->d_parent);
if (err)
- goto out_drop_write;
+ goto out;
err = ovl_nlink_start(dentry);
if (err)
- goto out_drop_write;
+ goto out;
old_cred = ovl_override_creds(dentry->d_sb);
if (!lower_positive)
@@ -926,8 +916,6 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir)
if (ovl_dentry_upper(dentry))
ovl_copyattr(d_inode(dentry));
-out_drop_write:
- ovl_drop_write(dentry);
out:
ovl_cache_free(&list);
return err;
@@ -1131,29 +1119,32 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
}
}
- err = ovl_want_write(old);
- if (err)
- goto out;
-
err = ovl_copy_up(old);
if (err)
- goto out_drop_write;
+ goto out;
err = ovl_copy_up(new->d_parent);
if (err)
- goto out_drop_write;
+ goto out;
if (!overwrite) {
err = ovl_copy_up(new);
if (err)
- goto out_drop_write;
+ goto out;
} else if (d_inode(new)) {
err = ovl_nlink_start(new);
if (err)
- goto out_drop_write;
+ goto out;
update_nlink = true;
}
+ if (!update_nlink) {
+ /* ovl_nlink_start() took ovl_want_write() */
+ err = ovl_want_write(old);
+ if (err)
+ goto out;
+ }
+
old_cred = ovl_override_creds(old->d_sb);
if (!list_empty(&list)) {
@@ -1286,8 +1277,8 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
revert_creds(old_cred);
if (update_nlink)
ovl_nlink_end(new);
-out_drop_write:
- ovl_drop_write(old);
+ else
+ ovl_drop_write(old);
out:
dput(opaquedir);
ovl_cache_free(&list);
diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index c8c8588bd98c..4a79c479c971 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -23,12 +23,7 @@ static int ovl_encode_maybe_copy_up(struct dentry *dentry)
if (ovl_dentry_upper(dentry))
return 0;
- err = ovl_want_write(dentry);
- if (!err) {
- err = ovl_copy_up(dentry);
- ovl_drop_write(dentry);
- }
-
+ err = ovl_copy_up(dentry);
if (err) {
pr_warn_ratelimited("failed to copy up on encode (%pd2, err=%i)\n",
dentry, err);
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index b395cd84bfce..f5638cfe8f6d 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -32,10 +32,6 @@ int ovl_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
if (err)
return err;
- err = ovl_want_write(dentry);
- if (err)
- goto out;
-
if (attr->ia_valid & ATTR_SIZE) {
/* Truncate should trigger data copy up as well */
full_copy_up = true;
@@ -54,7 +50,7 @@ int ovl_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
winode = d_inode(upperdentry);
err = get_write_access(winode);
if (err)
- goto out_drop_write;
+ goto out;
}
if (attr->ia_valid & (ATTR_KILL_SUID|ATTR_KILL_SGID))
@@ -78,6 +74,10 @@ int ovl_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
*/
attr->ia_valid &= ~ATTR_OPEN;
+ err = ovl_want_write(dentry);
+ if (err)
+ goto out;
+
inode_lock(upperdentry->d_inode);
old_cred = ovl_override_creds(dentry->d_sb);
err = ovl_do_notify_change(ofs, upperdentry, attr);
@@ -85,12 +85,11 @@ int ovl_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
if (!err)
ovl_copyattr(dentry->d_inode);
inode_unlock(upperdentry->d_inode);
+ ovl_drop_write(dentry);
if (winode)
put_write_access(winode);
}
-out_drop_write:
- ovl_drop_write(dentry);
out:
return err;
}
@@ -361,27 +360,27 @@ int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
struct path realpath;
const struct cred *old_cred;
- err = ovl_want_write(dentry);
- if (err)
- goto out;
-
if (!value && !upperdentry) {
ovl_path_lower(dentry, &realpath);
old_cred = ovl_override_creds(dentry->d_sb);
err = vfs_getxattr(mnt_idmap(realpath.mnt), realdentry, name, NULL, 0);
revert_creds(old_cred);
if (err < 0)
- goto out_drop_write;
+ goto out;
}
if (!upperdentry) {
err = ovl_copy_up(dentry);
if (err)
- goto out_drop_write;
+ goto out;
realdentry = ovl_dentry_upper(dentry);
}
+ err = ovl_want_write(dentry);
+ if (err)
+ goto out;
+
old_cred = ovl_override_creds(dentry->d_sb);
if (value) {
err = ovl_do_setxattr(ofs, realdentry, name, value, size,
@@ -391,12 +390,10 @@ int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
err = ovl_do_removexattr(ofs, realdentry, name);
}
revert_creds(old_cred);
+ ovl_drop_write(dentry);
/* copy c/mtime */
ovl_copyattr(inode);
-
-out_drop_write:
- ovl_drop_write(dentry);
out:
return err;
}
@@ -611,10 +608,6 @@ static int ovl_set_or_remove_acl(struct dentry *dentry, struct inode *inode,
struct dentry *upperdentry = ovl_dentry_upper(dentry);
struct dentry *realdentry = upperdentry ?: ovl_dentry_lower(dentry);
- err = ovl_want_write(dentry);
- if (err)
- return err;
-
/*
* If ACL is to be removed from a lower file, check if it exists in
* the first place before copying it up.
@@ -630,7 +623,7 @@ static int ovl_set_or_remove_acl(struct dentry *dentry, struct inode *inode,
revert_creds(old_cred);
if (IS_ERR(real_acl)) {
err = PTR_ERR(real_acl);
- goto out_drop_write;
+ goto out;
}
posix_acl_release(real_acl);
}
@@ -638,23 +631,26 @@ static int ovl_set_or_remove_acl(struct dentry *dentry, struct inode *inode,
if (!upperdentry) {
err = ovl_copy_up(dentry);
if (err)
- goto out_drop_write;
+ goto out;
realdentry = ovl_dentry_upper(dentry);
}
+ err = ovl_want_write(dentry);
+ if (err)
+ goto out;
+
old_cred = ovl_override_creds(dentry->d_sb);
if (acl)
err = ovl_do_set_acl(ofs, realdentry, acl_name, acl);
else
err = ovl_do_remove_acl(ofs, realdentry, acl_name);
revert_creds(old_cred);
+ ovl_drop_write(dentry);
/* copy c/mtime */
ovl_copyattr(inode);
-
-out_drop_write:
- ovl_drop_write(dentry);
+out:
return err;
}
@@ -777,14 +773,14 @@ int ovl_fileattr_set(struct mnt_idmap *idmap,
unsigned int flags;
int err;
- err = ovl_want_write(dentry);
- if (err)
- goto out;
-
err = ovl_copy_up(dentry);
if (!err) {
ovl_path_real(dentry, &upperpath);
+ err = ovl_want_write(dentry);
+ if (err)
+ goto out;
+
old_cred = ovl_override_creds(inode->i_sb);
/*
* Store immutable/append-only flags in xattr and clear them
@@ -797,6 +793,7 @@ int ovl_fileattr_set(struct mnt_idmap *idmap,
if (!err)
err = ovl_real_fileattr_set(&upperpath, fa);
revert_creds(old_cred);
+ ovl_drop_write(dentry);
/*
* Merge real inode flags with inode flags read from
@@ -811,7 +808,6 @@ int ovl_fileattr_set(struct mnt_idmap *idmap,
/* Update ctime */
ovl_copyattr(inode);
}
- ovl_drop_write(dentry);
out:
return err;
}
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 0f387092450e..4deed8a2a112 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -1062,6 +1062,10 @@ int ovl_nlink_start(struct dentry *dentry)
if (err)
return err;
+ err = ovl_want_write(dentry);
+ if (err)
+ goto out;
+
if (d_is_dir(dentry) || !ovl_test_flag(OVL_INDEX, inode))
goto out;
@@ -1074,6 +1078,8 @@ int ovl_nlink_start(struct dentry *dentry)
*/
err = ovl_set_nlink_upper(dentry);
revert_creds(old_cred);
+ if (err)
+ ovl_drop_write(dentry);
out:
if (err)
@@ -1094,6 +1100,7 @@ void ovl_nlink_end(struct dentry *dentry)
revert_creds(old_cred);
}
+ ovl_drop_write(dentry);
ovl_inode_unlock(inode);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 2/3] ovl: do not open/llseek lower file with upper sb_writers held
2023-08-14 14:05 [PATCH v2 0/3] overlayfs lock ordering changes Amir Goldstein
2023-08-14 14:05 ` [PATCH v2 1/3] ovl: reorder ovl_want_write() after ovl_inode_lock() Amir Goldstein
@ 2023-08-14 14:05 ` Amir Goldstein
2023-08-15 15:12 ` Miklos Szeredi
2023-08-14 14:05 ` [PATCH v2 3/3] ovl: do not encode lower fh " Amir Goldstein
2 siblings, 1 reply; 15+ messages in thread
From: Amir Goldstein @ 2023-08-14 14:05 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Jan Kara, linux-unionfs
overlayfs file open (ovl_maybe_lookup_lowerdata) and overlay file llseek
take the ovl_inode_lock, without holding upper sb_writers.
In case of nested lower overlay that uses same upper fs as this overlay,
lockdep will warn about (possibly false positive) circular lock
dependency when doing open/llseek of lower ovl file during copy up with
our upper sb_writers held, because the locking ordering seems reverse to
the locking order in ovl_copy_up_start():
- lower ovl_inode_lock
- upper sb_writers
Take upper sb_writers only when we actually need it, so we won't hold it
during lower file open and lower file llseek to avoid the lockdep warning.
Minimizing the scope of ovl_want_write() during copy up is also needed
for fixing other possible deadlocks by following patches.
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
fs/overlayfs/copy_up.c | 117 +++++++++++++++++++++++++++++++----------
1 file changed, 88 insertions(+), 29 deletions(-)
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index c998dab440f8..f2a31ff790fb 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -251,8 +251,13 @@ static int ovl_copy_up_file(struct ovl_fs *ofs, struct dentry *dentry,
if (IS_ERR(old_file))
return PTR_ERR(old_file);
+ error = ovl_want_write(dentry);
+ if (error)
+ goto out_fput;
+
/* Try to use clone_file_range to clone up within the same fs */
cloned = do_clone_file_range(old_file, 0, new_file, 0, len, 0);
+ ovl_drop_write(dentry);
if (cloned == len)
goto out_fput;
/* Couldn't clone, so now we try to copy the data */
@@ -287,8 +292,12 @@ static int ovl_copy_up_file(struct ovl_fs *ofs, struct dentry *dentry,
* it may not recognize all kind of holes and sometimes
* only skips partial of hole area. However, it will be
* enough for most of the use cases.
+ *
+ * We do not hold upper sb_writers throughout the loop to avert
+ * lockdep warning with llseek of lower file in nested overlay:
+ * - upper sb_writers
+ * -- lower ovl_inode_lock (ovl_llseek)
*/
-
if (skip_hole && data_pos < old_pos) {
data_pos = vfs_llseek(old_file, old_pos, SEEK_DATA);
if (data_pos > old_pos) {
@@ -303,9 +312,14 @@ static int ovl_copy_up_file(struct ovl_fs *ofs, struct dentry *dentry,
}
}
+ error = ovl_want_write(dentry);
+ if (error)
+ break;
+
bytes = do_splice_direct(old_file, &old_pos,
new_file, &new_pos,
this_len, SPLICE_F_MOVE);
+ ovl_drop_write(dentry);
if (bytes <= 0) {
error = bytes;
break;
@@ -555,14 +569,18 @@ 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);
+ err = ovl_want_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);
if (err)
- return err;
+ goto out_drop_write;
err = ovl_set_nlink_lower(c->dentry);
if (err)
- return err;
+ goto out_drop_write;
inode_lock_nested(udir, I_MUTEX_PARENT);
upper = ovl_lookup_upper(ofs, c->dentry->d_name.name, upperdir,
@@ -581,10 +599,12 @@ static int ovl_link_up(struct ovl_copy_up_ctx *c)
}
inode_unlock(udir);
if (err)
- return err;
+ goto out_drop_write;
err = ovl_set_nlink_upper(c->dentry);
+out_drop_write:
+ ovl_drop_write(c->dentry);
return err;
}
@@ -710,7 +730,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;
struct ovl_cu_creds cc;
- int err;
+ int err, err2;
struct ovl_cattr cattr = {
/* Can't properly set mode on creation because of the umask */
.mode = c->stat.mode & S_IFMT,
@@ -718,21 +738,22 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
.link = c->link
};
- /* workdir and destdir could be the same when copying up to indexdir */
- err = -EIO;
- if (lock_rename(c->workdir, c->destdir) != NULL)
- goto unlock;
-
err = ovl_prep_cu_creds(c->dentry, &cc);
if (err)
- goto unlock;
+ return err;
- temp = ovl_create_temp(ofs, c->workdir, &cattr);
+ err = ovl_want_write(c->dentry);
+ if (!err) {
+ inode_lock(d_inode(c->workdir));
+ temp = ovl_create_temp(ofs, c->workdir, &cattr);
+ inode_unlock(d_inode(c->workdir));
+ ovl_drop_write(c->dentry);
+ if (IS_ERR(temp))
+ err = PTR_ERR(temp);
+ }
ovl_revert_cu_creds(&cc);
-
- err = PTR_ERR(temp);
- if (IS_ERR(temp))
- goto unlock;
+ if (err)
+ return err;
/*
* Copy up data first and then xattrs. Writing data after
@@ -740,6 +761,21 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
*/
path.dentry = temp;
err = ovl_copy_up_data(c, &path);
+ /*
+ * Request write access, lock workdir and destdir and make sure that
+ * temp wasn't moved before copy up completion or cleanup.
+ * workdir and destdir could be the same when copying up to indexdir.
+ */
+ err2 = ovl_want_write(c->dentry);
+ if (err2)
+ return err ?: err2;
+
+ if (lock_rename(c->workdir, c->destdir) != NULL ||
+ temp->d_parent != c->workdir) {
+ err = err ?: -EIO;
+ goto unlock;
+ }
+
if (err)
goto cleanup;
@@ -778,6 +814,7 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
ovl_set_flag(OVL_WHITEOUTS, inode);
unlock:
unlock_rename(c->workdir, c->destdir);
+ ovl_drop_write(c->dentry);
return err;
@@ -801,11 +838,16 @@ static int ovl_copy_up_tmpfile(struct ovl_copy_up_ctx *c)
if (err)
return err;
- tmpfile = ovl_do_tmpfile(ofs, c->workdir, c->stat.mode);
+ err = ovl_want_write(c->dentry);
+ if (!err) {
+ tmpfile = ovl_do_tmpfile(ofs, c->workdir, c->stat.mode);
+ ovl_drop_write(c->dentry);
+ if (IS_ERR(tmpfile))
+ err = PTR_ERR(tmpfile);
+ }
ovl_revert_cu_creds(&cc);
-
- if (IS_ERR(tmpfile))
- return PTR_ERR(tmpfile);
+ if (err)
+ return err;
temp = tmpfile->f_path.dentry;
if (!c->metacopy && c->stat.size) {
@@ -814,10 +856,14 @@ static int ovl_copy_up_tmpfile(struct ovl_copy_up_ctx *c)
goto out_fput;
}
- err = ovl_copy_up_metadata(c, temp);
+ err = ovl_want_write(c->dentry);
if (err)
goto out_fput;
+ err = ovl_copy_up_metadata(c, temp);
+ if (err)
+ goto out_drop_write;
+
inode_lock_nested(udir, I_MUTEX_PARENT);
upper = ovl_lookup_upper(ofs, c->destname.name, c->destdir,
@@ -830,7 +876,7 @@ static int ovl_copy_up_tmpfile(struct ovl_copy_up_ctx *c)
inode_unlock(udir);
if (err)
- goto out_fput;
+ goto out_drop_write;
if (c->metacopy_digest)
ovl_set_flag(OVL_HAS_DIGEST, d_inode(c->dentry));
@@ -842,6 +888,8 @@ static int ovl_copy_up_tmpfile(struct ovl_copy_up_ctx *c)
ovl_set_upperdata(d_inode(c->dentry));
ovl_inode_update(d_inode(c->dentry), dget(temp));
+out_drop_write:
+ ovl_drop_write(c->dentry);
out_fput:
fput(tmpfile);
return err;
@@ -892,7 +940,12 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
* Mark parent "impure" because it may now contain non-pure
* upper
*/
+ err = ovl_want_write(c->dentry);
+ if (err)
+ return err;
+
err = ovl_set_impure(c->parent, c->destdir);
+ ovl_drop_write(c->dentry);
if (err)
return err;
}
@@ -908,6 +961,10 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
if (c->indexed)
ovl_set_flag(OVL_INDEX, d_inode(c->dentry));
+ err = ovl_want_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);
@@ -923,6 +980,7 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
ovl_dentry_update_reval(c->dentry, ovl_dentry_upper(c->dentry));
}
+ ovl_drop_write(c->dentry);
out:
if (to_index)
kfree(c->destname.name);
@@ -1006,6 +1064,10 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
if (err)
goto out_free;
+ err = ovl_want_write(c->dentry);
+ if (err)
+ goto out_free;
+
/*
* Writing to upper file will clear security.capability xattr. We
* don't want that to happen for normal copy-up operation.
@@ -1014,17 +1076,19 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
err = ovl_do_setxattr(ofs, upperpath.dentry, XATTR_NAME_CAPS,
capability, cap_size, 0);
if (err)
- goto out_free;
+ goto out_drop_write;
}
err = ovl_removexattr(ofs, upperpath.dentry, OVL_XATTR_METACOPY);
if (err)
- goto out_free;
+ goto out_drop_write;
ovl_clear_flag(OVL_HAS_DIGEST, d_inode(c->dentry));
ovl_clear_flag(OVL_VERIFIED_DIGEST, d_inode(c->dentry));
ovl_set_upperdata(d_inode(c->dentry));
+out_drop_write:
+ ovl_drop_write(c->dentry);
out_free:
kfree(capability);
out:
@@ -1088,17 +1152,12 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
goto out;
}
- err = ovl_want_write(dentry);
- if (err)
- goto out;
-
if (!ovl_dentry_upper(dentry))
err = ovl_do_copy_up(&ctx);
if (!err && parent && !ovl_dentry_has_upper_alias(dentry))
err = ovl_link_up(&ctx);
if (!err && ovl_dentry_needs_data_copy_up_locked(dentry, flags))
err = ovl_copy_up_meta_inode_data(&ctx);
- ovl_drop_write(dentry);
ovl_copy_up_end(dentry);
out:
do_delayed_call(&done);
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 3/3] ovl: do not encode lower fh with upper sb_writers held
2023-08-14 14:05 [PATCH v2 0/3] overlayfs lock ordering changes Amir Goldstein
2023-08-14 14:05 ` [PATCH v2 1/3] ovl: reorder ovl_want_write() after ovl_inode_lock() Amir Goldstein
2023-08-14 14:05 ` [PATCH v2 2/3] ovl: do not open/llseek lower file with upper sb_writers held Amir Goldstein
@ 2023-08-14 14:05 ` Amir Goldstein
2 siblings, 0 replies; 15+ messages in thread
From: Amir Goldstein @ 2023-08-14 14:05 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Jan Kara, linux-unionfs
When lower fs is a nested overlayfs, calling encode_fh() on a lower
directory dentry may trigger copy up and take sb_writers on the upper fs
of the lower nested overlayfs.
The lower nested overlayfs may have the same upper fs as this overlayfs,
so nested sb_writers lock is illegal.
Move all the callers that encode lower fh to before ovl_want_write().
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
fs/overlayfs/copy_up.c | 54 ++++++++++++++++++++++++----------------
fs/overlayfs/namei.c | 37 ++++++++++++++++++++-------
fs/overlayfs/overlayfs.h | 26 +++++++++++++------
fs/overlayfs/super.c | 20 ++++++++++-----
fs/overlayfs/util.c | 11 +++++++-
5 files changed, 104 insertions(+), 44 deletions(-)
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index f2a31ff790fb..d9d925b96f37 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -440,29 +440,29 @@ struct ovl_fh *ovl_encode_real_fh(struct ovl_fs *ofs, struct dentry *real,
return ERR_PTR(err);
}
-int ovl_set_origin(struct ovl_fs *ofs, struct dentry *lower,
- struct dentry *upper)
+struct ovl_fh *ovl_get_origin_fh(struct ovl_fs *ofs, struct dentry *origin)
{
- const struct ovl_fh *fh = NULL;
- int err;
-
/*
* When lower layer doesn't support export operations store a 'null' fh,
* so we can use the overlay.origin xattr to distignuish between a copy
* up and a pure upper inode.
*/
- if (ovl_can_decode_fh(lower->d_sb)) {
- fh = ovl_encode_real_fh(ofs, lower, false);
- if (IS_ERR(fh))
- return PTR_ERR(fh);
- }
+ if (!ovl_can_decode_fh(origin->d_sb))
+ return NULL;
+
+ return ovl_encode_real_fh(ofs, origin, false);
+}
+
+int ovl_set_origin_fh(struct ovl_fs *ofs, const struct ovl_fh *fh,
+ struct dentry *upper)
+{
+ int err;
/*
* Do not fail when upper doesn't support xattrs.
*/
err = ovl_check_setxattr(ofs, upper, OVL_XATTR_ORIGIN, fh->buf,
fh ? fh->fb.len : 0, 0);
- kfree(fh);
/* Ignore -EPERM from setting "user.*" on symlink/special */
return err == -EPERM ? 0 : err;
@@ -490,7 +490,7 @@ static int ovl_set_upper_fh(struct ovl_fs *ofs, struct dentry *upper,
*
* Caller must hold i_mutex on indexdir.
*/
-static int ovl_create_index(struct dentry *dentry, struct dentry *origin,
+static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
struct dentry *upper)
{
struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
@@ -516,7 +516,7 @@ static int ovl_create_index(struct dentry *dentry, struct dentry *origin,
if (WARN_ON(ovl_test_flag(OVL_INDEX, d_inode(dentry))))
return -EIO;
- err = ovl_get_index_name(ofs, origin, &name);
+ err = ovl_get_index_name_fh(fh, &name);
if (err)
return err;
@@ -555,6 +555,7 @@ struct ovl_copy_up_ctx {
struct dentry *destdir;
struct qstr destname;
struct dentry *workdir;
+ const struct ovl_fh *origin_fh;
bool origin;
bool indexed;
bool metacopy;
@@ -656,7 +657,7 @@ static int ovl_copy_up_metadata(struct ovl_copy_up_ctx *c, struct dentry *temp)
* hard link.
*/
if (c->origin) {
- err = ovl_set_origin(ofs, c->lowerpath.dentry, temp);
+ err = ovl_set_origin_fh(ofs, c->origin_fh, temp);
if (err)
return err;
}
@@ -784,7 +785,7 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
goto cleanup;
if (S_ISDIR(c->stat.mode) && c->indexed) {
- err = ovl_create_index(c->dentry, c->lowerpath.dentry, temp);
+ err = ovl_create_index(c->dentry, c->origin_fh, temp);
if (err)
goto cleanup;
}
@@ -908,6 +909,8 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
{
int err;
struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
+ struct dentry *origin = c->lowerpath.dentry;
+ struct ovl_fh *fh = NULL;
bool to_index = false;
/*
@@ -924,17 +927,25 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
to_index = true;
}
- if (S_ISDIR(c->stat.mode) || c->stat.nlink == 1 || to_index)
+ if (S_ISDIR(c->stat.mode) || c->stat.nlink == 1 || to_index) {
+ fh = ovl_get_origin_fh(ofs, origin);
+ if (IS_ERR(fh))
+ return PTR_ERR(fh);
+
+ /* origin_fh may be NULL */
+ c->origin_fh = fh;
c->origin = true;
+ }
if (to_index) {
c->destdir = ovl_indexdir(c->dentry->d_sb);
- err = ovl_get_index_name(ofs, c->lowerpath.dentry, &c->destname);
+ err = ovl_get_index_name(ofs, origin, &c->destname);
if (err)
- return err;
+ goto out;
} else if (WARN_ON(!c->parent)) {
/* Disconnected dentry must be copied up to index dir */
- return -EIO;
+ err = -EIO;
+ goto out;
} else {
/*
* Mark parent "impure" because it may now contain non-pure
@@ -942,12 +953,12 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
*/
err = ovl_want_write(c->dentry);
if (err)
- return err;
+ goto out;
err = ovl_set_impure(c->parent, c->destdir);
ovl_drop_write(c->dentry);
if (err)
- return err;
+ goto out;
}
/* Should we copyup with O_TMPFILE or with workdir? */
@@ -984,6 +995,7 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
out:
if (to_index)
kfree(c->destname.name);
+ kfree(fh);
return err;
}
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 80391c687c2a..f10ac4ae35f0 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -507,6 +507,19 @@ static int ovl_verify_fh(struct ovl_fs *ofs, struct dentry *dentry,
return err;
}
+int ovl_verify_set_fh(struct ovl_fs *ofs, struct dentry *dentry,
+ enum ovl_xattr ox, const struct ovl_fh *fh,
+ bool is_upper, bool set)
+{
+ int err;
+
+ err = ovl_verify_fh(ofs, dentry, ox, fh);
+ if (set && err == -ENODATA)
+ err = ovl_setxattr(ofs, dentry, ox, fh->buf, fh->fb.len);
+
+ return err;
+}
+
/*
* Verify that @real dentry matches the file handle stored in xattr @name.
*
@@ -515,9 +528,9 @@ static int ovl_verify_fh(struct ovl_fs *ofs, struct dentry *dentry,
*
* Return 0 on match, -ESTALE on mismatch, -ENODATA on no xattr, < 0 on error.
*/
-int ovl_verify_set_fh(struct ovl_fs *ofs, struct dentry *dentry,
- enum ovl_xattr ox, struct dentry *real, bool is_upper,
- bool set)
+int ovl_verify_origin_xattr(struct ovl_fs *ofs, struct dentry *dentry,
+ enum ovl_xattr ox, struct dentry *real,
+ bool is_upper, bool set)
{
struct inode *inode;
struct ovl_fh *fh;
@@ -530,9 +543,7 @@ int ovl_verify_set_fh(struct ovl_fs *ofs, struct dentry *dentry,
goto fail;
}
- err = ovl_verify_fh(ofs, dentry, ox, fh);
- if (set && err == -ENODATA)
- err = ovl_setxattr(ofs, dentry, ox, fh->buf, fh->fb.len);
+ err = ovl_verify_set_fh(ofs, dentry, ox, fh, is_upper, set);
if (err)
goto fail;
@@ -548,6 +559,7 @@ int ovl_verify_set_fh(struct ovl_fs *ofs, struct dentry *dentry,
goto out;
}
+
/* Get upper dentry from index */
struct dentry *ovl_index_upper(struct ovl_fs *ofs, struct dentry *index,
bool connected)
@@ -684,7 +696,7 @@ int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index)
goto out;
}
-static int ovl_get_index_name_fh(struct ovl_fh *fh, struct qstr *name)
+int ovl_get_index_name_fh(const struct ovl_fh *fh, struct qstr *name)
{
char *n, *s;
@@ -873,20 +885,27 @@ int ovl_path_next(int idx, struct dentry *dentry, struct path *path)
static int ovl_fix_origin(struct ovl_fs *ofs, struct dentry *dentry,
struct dentry *lower, struct dentry *upper)
{
+ const struct ovl_fh *fh;
int err;
if (ovl_check_origin_xattr(ofs, upper))
return 0;
+ fh = ovl_get_origin_fh(ofs, lower);
+ if (IS_ERR(fh))
+ return PTR_ERR(fh);
+
err = ovl_want_write(dentry);
if (err)
- return err;
+ goto out;
- err = ovl_set_origin(ofs, lower, upper);
+ err = ovl_set_origin_fh(ofs, fh, upper);
if (!err)
err = ovl_set_impure(dentry->d_parent, upper->d_parent);
ovl_drop_write(dentry);
+out:
+ kfree(fh);
return err;
}
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 72f57d919aa9..715afef4804d 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -624,11 +624,15 @@ struct dentry *ovl_decode_real_fh(struct ovl_fs *ofs, struct ovl_fh *fh,
int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
struct dentry *upperdentry, struct ovl_path **stackp);
int ovl_verify_set_fh(struct ovl_fs *ofs, struct dentry *dentry,
- enum ovl_xattr ox, struct dentry *real, bool is_upper,
- bool set);
+ enum ovl_xattr ox, const struct ovl_fh *fh,
+ bool is_upper, bool set);
+int ovl_verify_origin_xattr(struct ovl_fs *ofs, struct dentry *dentry,
+ enum ovl_xattr ox, struct dentry *real,
+ bool is_upper, bool set);
struct dentry *ovl_index_upper(struct ovl_fs *ofs, struct dentry *index,
bool connected);
int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index);
+int ovl_get_index_name_fh(const struct ovl_fh *fh, struct qstr *name);
int ovl_get_index_name(struct ovl_fs *ofs, struct dentry *origin,
struct qstr *name);
struct dentry *ovl_get_index_fh(struct ovl_fs *ofs, struct ovl_fh *fh);
@@ -640,17 +644,24 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
unsigned int flags);
bool ovl_lower_positive(struct dentry *dentry);
+static inline int ovl_verify_origin_fh(struct ovl_fs *ofs, struct dentry *upper,
+ const struct ovl_fh *fh, bool set)
+{
+ return ovl_verify_set_fh(ofs, upper, OVL_XATTR_ORIGIN, fh, false, set);
+}
+
static inline int ovl_verify_origin(struct ovl_fs *ofs, struct dentry *upper,
struct dentry *origin, bool set)
{
- return ovl_verify_set_fh(ofs, upper, OVL_XATTR_ORIGIN, origin,
- false, set);
+ return ovl_verify_origin_xattr(ofs, upper, OVL_XATTR_ORIGIN, origin,
+ false, set);
}
static inline int ovl_verify_upper(struct ovl_fs *ofs, struct dentry *index,
struct dentry *upper, bool set)
{
- return ovl_verify_set_fh(ofs, index, OVL_XATTR_UPPER, upper, true, set);
+ return ovl_verify_origin_xattr(ofs, index, OVL_XATTR_UPPER, upper,
+ true, set);
}
/* readdir.c */
@@ -815,8 +826,9 @@ int ovl_copy_xattr(struct super_block *sb, const struct path *path, struct dentr
int ovl_set_attr(struct ovl_fs *ofs, struct dentry *upper, struct kstat *stat);
struct ovl_fh *ovl_encode_real_fh(struct ovl_fs *ofs, struct dentry *real,
bool is_upper);
-int ovl_set_origin(struct ovl_fs *ofs, struct dentry *lower,
- struct dentry *upper);
+struct ovl_fh *ovl_get_origin_fh(struct ovl_fs *ofs, struct dentry *origin);
+int ovl_set_origin_fh(struct ovl_fs *ofs, const struct ovl_fh *fh,
+ struct dentry *upper);
/* export.c */
extern const struct export_operations ovl_export_operations;
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index def266b5e2a3..93d500d4fda9 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -881,15 +881,20 @@ static int ovl_get_indexdir(struct super_block *sb, struct ovl_fs *ofs,
{
struct vfsmount *mnt = ovl_upper_mnt(ofs);
struct dentry *indexdir;
+ struct dentry *origin = ovl_lowerstack(oe)->dentry;
+ const struct ovl_fh *fh;
int err;
+ fh = ovl_get_origin_fh(ofs, origin);
+ if (IS_ERR(fh))
+ return PTR_ERR(fh);
+
err = mnt_want_write(mnt);
if (err)
- return err;
+ goto out_free_fh;
/* Verify lower root is upper root origin */
- err = ovl_verify_origin(ofs, upperpath->dentry,
- ovl_lowerstack(oe)->dentry, true);
+ err = ovl_verify_origin_fh(ofs, upperpath->dentry, fh, true);
if (err) {
pr_err("failed to verify upper root origin\n");
goto out;
@@ -921,9 +926,10 @@ static int ovl_get_indexdir(struct super_block *sb, struct ovl_fs *ofs,
* directory entries.
*/
if (ovl_check_origin_xattr(ofs, ofs->indexdir)) {
- err = ovl_verify_set_fh(ofs, ofs->indexdir,
- OVL_XATTR_ORIGIN,
- upperpath->dentry, true, false);
+ err = ovl_verify_origin_xattr(ofs, ofs->indexdir,
+ OVL_XATTR_ORIGIN,
+ upperpath->dentry, true,
+ false);
if (err)
pr_err("failed to verify index dir 'origin' xattr\n");
}
@@ -941,6 +947,8 @@ static int ovl_get_indexdir(struct super_block *sb, struct ovl_fs *ofs,
out:
mnt_drop_write(mnt);
+out_free_fh:
+ kfree(fh);
return err;
}
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 4deed8a2a112..bf2a6b69af67 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -973,12 +973,18 @@ static void ovl_cleanup_index(struct dentry *dentry)
struct dentry *index = NULL;
struct inode *inode;
struct qstr name = { };
+ bool got_write = false;
int err;
err = ovl_get_index_name(ofs, lowerdentry, &name);
if (err)
goto fail;
+ err = ovl_want_write(dentry);
+ if (err)
+ goto fail;
+
+ got_write = true;
inode = d_inode(upperdentry);
if (!S_ISDIR(inode->i_mode) && inode->i_nlink != 1) {
pr_warn_ratelimited("cleanup linked index (%pd2, ino=%lu, nlink=%u)\n",
@@ -1016,6 +1022,8 @@ static void ovl_cleanup_index(struct dentry *dentry)
goto fail;
out:
+ if (got_write)
+ ovl_drop_write(dentry);
kfree(name.name);
dput(index);
return;
@@ -1092,6 +1100,8 @@ void ovl_nlink_end(struct dentry *dentry)
{
struct inode *inode = d_inode(dentry);
+ ovl_drop_write(dentry);
+
if (ovl_test_flag(OVL_INDEX, inode) && inode->i_nlink == 0) {
const struct cred *old_cred;
@@ -1100,7 +1110,6 @@ void ovl_nlink_end(struct dentry *dentry)
revert_creds(old_cred);
}
- ovl_drop_write(dentry);
ovl_inode_unlock(inode);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/3] ovl: reorder ovl_want_write() after ovl_inode_lock()
2023-08-14 14:05 ` [PATCH v2 1/3] ovl: reorder ovl_want_write() after ovl_inode_lock() Amir Goldstein
@ 2023-08-15 10:50 ` Miklos Szeredi
2023-08-15 15:12 ` Amir Goldstein
0 siblings, 1 reply; 15+ messages in thread
From: Miklos Szeredi @ 2023-08-15 10:50 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Jan Kara, linux-unionfs
On Mon, 14 Aug 2023 at 16:05, Amir Goldstein <amir73il@gmail.com> wrote:
>
> Make the locking order of ovl_inode_lock() strictly between the two
> vfs stacked layers, i.e.:
> - ovl vfs locks: sb_writers, inode_lock, ...
> - ovl_inode_lock
> - upper vfs locks: sb_writers, inode_lock, ...
>
> To that effect, move ovl_want_write() into the helpers ovl_nlink_start()
> and ovl_copy_up_one() which currently take the ovl_inode_lock() after
> ovl_want_write().
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
> fs/overlayfs/copy_up.c | 36 ++++++++++-----------
> fs/overlayfs/dir.c | 71 ++++++++++++++++++------------------------
> fs/overlayfs/export.c | 7 +----
> fs/overlayfs/inode.c | 56 ++++++++++++++++-----------------
> fs/overlayfs/util.c | 7 +++++
> 5 files changed, 83 insertions(+), 94 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index bae404a1bad4..c998dab440f8 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -1085,15 +1085,22 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
> if (unlikely(err)) {
> if (err > 0)
> err = 0;
> - } else {
> - if (!ovl_dentry_upper(dentry))
> - err = ovl_do_copy_up(&ctx);
> - if (!err && parent && !ovl_dentry_has_upper_alias(dentry))
> - err = ovl_link_up(&ctx);
> - if (!err && ovl_dentry_needs_data_copy_up_locked(dentry, flags))
> - err = ovl_copy_up_meta_inode_data(&ctx);
> - ovl_copy_up_end(dentry);
> + goto out;
> }
> +
> + err = ovl_want_write(dentry);
> + if (err)
> + goto out;
Needs ovl_copy_up_end.
> +
> + if (!ovl_dentry_upper(dentry))
> + err = ovl_do_copy_up(&ctx);
> + if (!err && parent && !ovl_dentry_has_upper_alias(dentry))
> + err = ovl_link_up(&ctx);
> + if (!err && ovl_dentry_needs_data_copy_up_locked(dentry, flags))
> + err = ovl_copy_up_meta_inode_data(&ctx);
> + ovl_drop_write(dentry);
> + ovl_copy_up_end(dentry);
> +out:
> do_delayed_call(&done);
>
> return err;
> @@ -1169,17 +1176,10 @@ static bool ovl_open_need_copy_up(struct dentry *dentry, int flags)
>
> int ovl_maybe_copy_up(struct dentry *dentry, int flags)
> {
> - int err = 0;
> -
> - if (ovl_open_need_copy_up(dentry, flags)) {
> - err = ovl_want_write(dentry);
> - if (!err) {
> - err = ovl_copy_up_flags(dentry, flags);
> - ovl_drop_write(dentry);
> - }
> - }
> + if (!ovl_open_need_copy_up(dentry, flags))
> + return 0;
>
> - return err;
> + return ovl_copy_up_flags(dentry, flags);
> }
>
> int ovl_copy_up_with_data(struct dentry *dentry)
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 033fc0458a3d..f01031fe7b97 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -559,10 +559,6 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
> struct cred *override_cred;
> struct dentry *parent = dentry->d_parent;
>
> - err = ovl_copy_up(parent);
> - if (err)
> - return err;
> -
> old_cred = ovl_override_creds(dentry->d_sb);
>
> /*
> @@ -626,15 +622,11 @@ static int ovl_create_object(struct dentry *dentry, int mode, dev_t rdev,
> .link = link,
> };
>
> - err = ovl_want_write(dentry);
> - if (err)
> - goto out;
> -
> /* Preallocate inode to be used by ovl_get_inode() */
> err = -ENOMEM;
> inode = ovl_new_inode(dentry->d_sb, mode, rdev);
> if (!inode)
> - goto out_drop_write;
> + goto out;
>
> spin_lock(&inode->i_lock);
> inode->i_state |= I_CREATING;
> @@ -643,12 +635,19 @@ static int ovl_create_object(struct dentry *dentry, int mode, dev_t rdev,
> inode_init_owner(&nop_mnt_idmap, inode, dentry->d_parent->d_inode, mode);
> attr.mode = inode->i_mode;
>
> + err = ovl_copy_up(dentry->d_parent);
> + if (err)
> + return err;
Needs iput().
> +
> + err = ovl_want_write(dentry);
> + if (err)
> + goto out;
This as well.
Also I don't understand the reason behind moving ovl_want_write().
I'd just put the copy_up(dentry->parent) above ovl_mnt_write().
> +
> err = ovl_create_or_link(dentry, inode, &attr, false);
> /* Did we end up using the preallocated inode? */
> if (inode != d_inode(dentry))
> iput(inode);
>
> -out_drop_write:
> ovl_drop_write(dentry);
> out:
> return err;
> @@ -700,28 +699,24 @@ static int ovl_link(struct dentry *old, struct inode *newdir,
> int err;
> struct inode *inode;
>
> - err = ovl_want_write(old);
> + err = ovl_copy_up(old);
> if (err)
> goto out;
>
> - err = ovl_copy_up(old);
> + err = ovl_copy_up(new->d_parent);
> if (err)
> - goto out_drop_write;
> + goto out;
>
> - err = ovl_copy_up(new->d_parent);
> + err = ovl_nlink_start(old);
> if (err)
> - goto out_drop_write;
> + goto out;
>
> if (ovl_is_metacopy_dentry(old)) {
> err = ovl_set_link_redirect(old);
> if (err)
> - goto out_drop_write;
> + goto out_nlink_end;
> }
>
> - err = ovl_nlink_start(old);
> - if (err)
> - goto out_drop_write;
> -
> inode = d_inode(old);
> ihold(inode);
>
> @@ -731,9 +726,8 @@ static int ovl_link(struct dentry *old, struct inode *newdir,
> if (err)
> iput(inode);
>
> +out_nlink_end:
> ovl_nlink_end(old);
> -out_drop_write:
> - ovl_drop_write(old);
> out:
> return err;
> }
> @@ -891,17 +885,13 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir)
> goto out;
> }
>
> - err = ovl_want_write(dentry);
> - if (err)
> - goto out;
> -
> err = ovl_copy_up(dentry->d_parent);
> if (err)
> - goto out_drop_write;
> + goto out;
>
> err = ovl_nlink_start(dentry);
> if (err)
> - goto out_drop_write;
> + goto out;
>
> old_cred = ovl_override_creds(dentry->d_sb);
> if (!lower_positive)
> @@ -926,8 +916,6 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir)
> if (ovl_dentry_upper(dentry))
> ovl_copyattr(d_inode(dentry));
>
> -out_drop_write:
> - ovl_drop_write(dentry);
> out:
> ovl_cache_free(&list);
> return err;
> @@ -1131,29 +1119,32 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
> }
> }
>
> - err = ovl_want_write(old);
> - if (err)
> - goto out;
> -
> err = ovl_copy_up(old);
> if (err)
> - goto out_drop_write;
> + goto out;
>
> err = ovl_copy_up(new->d_parent);
> if (err)
> - goto out_drop_write;
> + goto out;
> if (!overwrite) {
> err = ovl_copy_up(new);
> if (err)
> - goto out_drop_write;
> + goto out;
> } else if (d_inode(new)) {
> err = ovl_nlink_start(new);
> if (err)
> - goto out_drop_write;
> + goto out;
>
> update_nlink = true;
> }
>
> + if (!update_nlink) {
> + /* ovl_nlink_start() took ovl_want_write() */
> + err = ovl_want_write(old);
> + if (err)
> + goto out;
> + }
> +
> old_cred = ovl_override_creds(old->d_sb);
>
> if (!list_empty(&list)) {
> @@ -1286,8 +1277,8 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
> revert_creds(old_cred);
> if (update_nlink)
> ovl_nlink_end(new);
> -out_drop_write:
> - ovl_drop_write(old);
> + else
> + ovl_drop_write(old);
> out:
> dput(opaquedir);
> ovl_cache_free(&list);
> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> index c8c8588bd98c..4a79c479c971 100644
> --- a/fs/overlayfs/export.c
> +++ b/fs/overlayfs/export.c
> @@ -23,12 +23,7 @@ static int ovl_encode_maybe_copy_up(struct dentry *dentry)
> if (ovl_dentry_upper(dentry))
> return 0;
>
> - err = ovl_want_write(dentry);
> - if (!err) {
> - err = ovl_copy_up(dentry);
> - ovl_drop_write(dentry);
> - }
> -
> + err = ovl_copy_up(dentry);
> if (err) {
> pr_warn_ratelimited("failed to copy up on encode (%pd2, err=%i)\n",
> dentry, err);
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index b395cd84bfce..f5638cfe8f6d 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -32,10 +32,6 @@ int ovl_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> if (err)
> return err;
>
> - err = ovl_want_write(dentry);
> - if (err)
> - goto out;
> -
> if (attr->ia_valid & ATTR_SIZE) {
> /* Truncate should trigger data copy up as well */
> full_copy_up = true;
> @@ -54,7 +50,7 @@ int ovl_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> winode = d_inode(upperdentry);
> err = get_write_access(winode);
> if (err)
> - goto out_drop_write;
> + goto out;
> }
>
> if (attr->ia_valid & (ATTR_KILL_SUID|ATTR_KILL_SGID))
> @@ -78,6 +74,10 @@ int ovl_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> */
> attr->ia_valid &= ~ATTR_OPEN;
>
> + err = ovl_want_write(dentry);
> + if (err)
> + goto out;
Need to put write access.
> +
> inode_lock(upperdentry->d_inode);
> old_cred = ovl_override_creds(dentry->d_sb);
> err = ovl_do_notify_change(ofs, upperdentry, attr);
> @@ -85,12 +85,11 @@ int ovl_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> if (!err)
> ovl_copyattr(dentry->d_inode);
> inode_unlock(upperdentry->d_inode);
> + ovl_drop_write(dentry);
>
> if (winode)
> put_write_access(winode);
> }
> -out_drop_write:
> - ovl_drop_write(dentry);
> out:
> return err;
> }
> @@ -361,27 +360,27 @@ int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
> struct path realpath;
> const struct cred *old_cred;
>
> - err = ovl_want_write(dentry);
> - if (err)
> - goto out;
> -
> if (!value && !upperdentry) {
> ovl_path_lower(dentry, &realpath);
> old_cred = ovl_override_creds(dentry->d_sb);
> err = vfs_getxattr(mnt_idmap(realpath.mnt), realdentry, name, NULL, 0);
> revert_creds(old_cred);
> if (err < 0)
> - goto out_drop_write;
> + goto out;
> }
>
> if (!upperdentry) {
> err = ovl_copy_up(dentry);
> if (err)
> - goto out_drop_write;
> + goto out;
>
> realdentry = ovl_dentry_upper(dentry);
> }
>
> + err = ovl_want_write(dentry);
> + if (err)
> + goto out;
> +
> old_cred = ovl_override_creds(dentry->d_sb);
> if (value) {
> err = ovl_do_setxattr(ofs, realdentry, name, value, size,
> @@ -391,12 +390,10 @@ int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
> err = ovl_do_removexattr(ofs, realdentry, name);
> }
> revert_creds(old_cred);
> + ovl_drop_write(dentry);
>
> /* copy c/mtime */
> ovl_copyattr(inode);
> -
> -out_drop_write:
> - ovl_drop_write(dentry);
> out:
> return err;
> }
> @@ -611,10 +608,6 @@ static int ovl_set_or_remove_acl(struct dentry *dentry, struct inode *inode,
> struct dentry *upperdentry = ovl_dentry_upper(dentry);
> struct dentry *realdentry = upperdentry ?: ovl_dentry_lower(dentry);
>
> - err = ovl_want_write(dentry);
> - if (err)
> - return err;
> -
> /*
> * If ACL is to be removed from a lower file, check if it exists in
> * the first place before copying it up.
> @@ -630,7 +623,7 @@ static int ovl_set_or_remove_acl(struct dentry *dentry, struct inode *inode,
> revert_creds(old_cred);
> if (IS_ERR(real_acl)) {
> err = PTR_ERR(real_acl);
> - goto out_drop_write;
> + goto out;
> }
> posix_acl_release(real_acl);
> }
> @@ -638,23 +631,26 @@ static int ovl_set_or_remove_acl(struct dentry *dentry, struct inode *inode,
> if (!upperdentry) {
> err = ovl_copy_up(dentry);
> if (err)
> - goto out_drop_write;
> + goto out;
>
> realdentry = ovl_dentry_upper(dentry);
> }
>
> + err = ovl_want_write(dentry);
> + if (err)
> + goto out;
> +
> old_cred = ovl_override_creds(dentry->d_sb);
> if (acl)
> err = ovl_do_set_acl(ofs, realdentry, acl_name, acl);
> else
> err = ovl_do_remove_acl(ofs, realdentry, acl_name);
> revert_creds(old_cred);
> + ovl_drop_write(dentry);
>
> /* copy c/mtime */
> ovl_copyattr(inode);
> -
> -out_drop_write:
> - ovl_drop_write(dentry);
> +out:
> return err;
> }
>
> @@ -777,14 +773,14 @@ int ovl_fileattr_set(struct mnt_idmap *idmap,
> unsigned int flags;
> int err;
>
> - err = ovl_want_write(dentry);
> - if (err)
> - goto out;
> -
> err = ovl_copy_up(dentry);
> if (!err) {
> ovl_path_real(dentry, &upperpath);
>
> + err = ovl_want_write(dentry);
> + if (err)
> + goto out;
> +
> old_cred = ovl_override_creds(inode->i_sb);
> /*
> * Store immutable/append-only flags in xattr and clear them
> @@ -797,6 +793,7 @@ int ovl_fileattr_set(struct mnt_idmap *idmap,
> if (!err)
> err = ovl_real_fileattr_set(&upperpath, fa);
> revert_creds(old_cred);
> + ovl_drop_write(dentry);
>
> /*
> * Merge real inode flags with inode flags read from
> @@ -811,7 +808,6 @@ int ovl_fileattr_set(struct mnt_idmap *idmap,
> /* Update ctime */
> ovl_copyattr(inode);
> }
> - ovl_drop_write(dentry);
> out:
> return err;
> }
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 0f387092450e..4deed8a2a112 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -1062,6 +1062,10 @@ int ovl_nlink_start(struct dentry *dentry)
> if (err)
> return err;
>
> + err = ovl_want_write(dentry);
> + if (err)
> + goto out;
Need to unlock.
> +
> if (d_is_dir(dentry) || !ovl_test_flag(OVL_INDEX, inode))
> goto out;
Need to drop write.
>
> @@ -1074,6 +1078,8 @@ int ovl_nlink_start(struct dentry *dentry)
> */
> err = ovl_set_nlink_upper(dentry);
> revert_creds(old_cred);
> + if (err)
> + ovl_drop_write(dentry);
>
> out:
> if (err)
I'd just separate out error handling into separate labels.
> @@ -1094,6 +1100,7 @@ void ovl_nlink_end(struct dentry *dentry)
> revert_creds(old_cred);
> }
>
> + ovl_drop_write(dentry);
> ovl_inode_unlock(inode);
> }
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/3] ovl: reorder ovl_want_write() after ovl_inode_lock()
2023-08-15 10:50 ` Miklos Szeredi
@ 2023-08-15 15:12 ` Amir Goldstein
0 siblings, 0 replies; 15+ messages in thread
From: Amir Goldstein @ 2023-08-15 15:12 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Jan Kara, linux-unionfs
On Tue, Aug 15, 2023 at 1:50 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, 14 Aug 2023 at 16:05, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > Make the locking order of ovl_inode_lock() strictly between the two
> > vfs stacked layers, i.e.:
> > - ovl vfs locks: sb_writers, inode_lock, ...
> > - ovl_inode_lock
> > - upper vfs locks: sb_writers, inode_lock, ...
> >
> > To that effect, move ovl_want_write() into the helpers ovl_nlink_start()
> > and ovl_copy_up_one() which currently take the ovl_inode_lock() after
> > ovl_want_write().
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> > fs/overlayfs/copy_up.c | 36 ++++++++++-----------
> > fs/overlayfs/dir.c | 71 ++++++++++++++++++------------------------
> > fs/overlayfs/export.c | 7 +----
> > fs/overlayfs/inode.c | 56 ++++++++++++++++-----------------
> > fs/overlayfs/util.c | 7 +++++
> > 5 files changed, 83 insertions(+), 94 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index bae404a1bad4..c998dab440f8 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -1085,15 +1085,22 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
> > if (unlikely(err)) {
> > if (err > 0)
> > err = 0;
> > - } else {
> > - if (!ovl_dentry_upper(dentry))
> > - err = ovl_do_copy_up(&ctx);
> > - if (!err && parent && !ovl_dentry_has_upper_alias(dentry))
> > - err = ovl_link_up(&ctx);
> > - if (!err && ovl_dentry_needs_data_copy_up_locked(dentry, flags))
> > - err = ovl_copy_up_meta_inode_data(&ctx);
> > - ovl_copy_up_end(dentry);
> > + goto out;
> > }
> > +
> > + err = ovl_want_write(dentry);
> > + if (err)
> > + goto out;
>
> Needs ovl_copy_up_end.
Right. though those lines are removed in the next patch..
>
> > +
> > + if (!ovl_dentry_upper(dentry))
> > + err = ovl_do_copy_up(&ctx);
> > + if (!err && parent && !ovl_dentry_has_upper_alias(dentry))
> > + err = ovl_link_up(&ctx);
> > + if (!err && ovl_dentry_needs_data_copy_up_locked(dentry, flags))
> > + err = ovl_copy_up_meta_inode_data(&ctx);
> > + ovl_drop_write(dentry);
> > + ovl_copy_up_end(dentry);
> > +out:
> > do_delayed_call(&done);
> >
> > return err;
> > @@ -1169,17 +1176,10 @@ static bool ovl_open_need_copy_up(struct dentry *dentry, int flags)
> >
> > int ovl_maybe_copy_up(struct dentry *dentry, int flags)
> > {
> > - int err = 0;
> > -
> > - if (ovl_open_need_copy_up(dentry, flags)) {
> > - err = ovl_want_write(dentry);
> > - if (!err) {
> > - err = ovl_copy_up_flags(dentry, flags);
> > - ovl_drop_write(dentry);
> > - }
> > - }
> > + if (!ovl_open_need_copy_up(dentry, flags))
> > + return 0;
> >
> > - return err;
> > + return ovl_copy_up_flags(dentry, flags);
> > }
> >
> > int ovl_copy_up_with_data(struct dentry *dentry)
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index 033fc0458a3d..f01031fe7b97 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -559,10 +559,6 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
> > struct cred *override_cred;
> > struct dentry *parent = dentry->d_parent;
> >
> > - err = ovl_copy_up(parent);
> > - if (err)
> > - return err;
> > -
> > old_cred = ovl_override_creds(dentry->d_sb);
> >
> > /*
> > @@ -626,15 +622,11 @@ static int ovl_create_object(struct dentry *dentry, int mode, dev_t rdev,
> > .link = link,
> > };
> >
> > - err = ovl_want_write(dentry);
> > - if (err)
> > - goto out;
> > -
> > /* Preallocate inode to be used by ovl_get_inode() */
> > err = -ENOMEM;
> > inode = ovl_new_inode(dentry->d_sb, mode, rdev);
> > if (!inode)
> > - goto out_drop_write;
> > + goto out;
> >
> > spin_lock(&inode->i_lock);
> > inode->i_state |= I_CREATING;
> > @@ -643,12 +635,19 @@ static int ovl_create_object(struct dentry *dentry, int mode, dev_t rdev,
> > inode_init_owner(&nop_mnt_idmap, inode, dentry->d_parent->d_inode, mode);
> > attr.mode = inode->i_mode;
> >
> > + err = ovl_copy_up(dentry->d_parent);
> > + if (err)
> > + return err;
>
> Needs iput().
>
> > +
> > + err = ovl_want_write(dentry);
> > + if (err)
> > + goto out;
>
> This as well.
>
> Also I don't understand the reason behind moving ovl_want_write().
> I'd just put the copy_up(dentry->parent) above ovl_mnt_write().
>
You're right. not sure why I did that.
> > +
> > err = ovl_create_or_link(dentry, inode, &attr, false);
> > /* Did we end up using the preallocated inode? */
> > if (inode != d_inode(dentry))
> > iput(inode);
> >
> > -out_drop_write:
> > ovl_drop_write(dentry);
> > out:
> > return err;
> > @@ -700,28 +699,24 @@ static int ovl_link(struct dentry *old, struct inode *newdir,
> > int err;
> > struct inode *inode;
> >
> > - err = ovl_want_write(old);
> > + err = ovl_copy_up(old);
> > if (err)
> > goto out;
> >
> > - err = ovl_copy_up(old);
> > + err = ovl_copy_up(new->d_parent);
> > if (err)
> > - goto out_drop_write;
> > + goto out;
> >
> > - err = ovl_copy_up(new->d_parent);
> > + err = ovl_nlink_start(old);
> > if (err)
> > - goto out_drop_write;
> > + goto out;
> >
> > if (ovl_is_metacopy_dentry(old)) {
> > err = ovl_set_link_redirect(old);
> > if (err)
> > - goto out_drop_write;
> > + goto out_nlink_end;
> > }
> >
> > - err = ovl_nlink_start(old);
> > - if (err)
> > - goto out_drop_write;
> > -
> > inode = d_inode(old);
> > ihold(inode);
> >
> > @@ -731,9 +726,8 @@ static int ovl_link(struct dentry *old, struct inode *newdir,
> > if (err)
> > iput(inode);
> >
> > +out_nlink_end:
> > ovl_nlink_end(old);
> > -out_drop_write:
> > - ovl_drop_write(old);
> > out:
> > return err;
> > }
> > @@ -891,17 +885,13 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir)
> > goto out;
> > }
> >
> > - err = ovl_want_write(dentry);
> > - if (err)
> > - goto out;
> > -
> > err = ovl_copy_up(dentry->d_parent);
> > if (err)
> > - goto out_drop_write;
> > + goto out;
> >
> > err = ovl_nlink_start(dentry);
> > if (err)
> > - goto out_drop_write;
> > + goto out;
> >
> > old_cred = ovl_override_creds(dentry->d_sb);
> > if (!lower_positive)
> > @@ -926,8 +916,6 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir)
> > if (ovl_dentry_upper(dentry))
> > ovl_copyattr(d_inode(dentry));
> >
> > -out_drop_write:
> > - ovl_drop_write(dentry);
> > out:
> > ovl_cache_free(&list);
> > return err;
> > @@ -1131,29 +1119,32 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
> > }
> > }
> >
> > - err = ovl_want_write(old);
> > - if (err)
> > - goto out;
> > -
> > err = ovl_copy_up(old);
> > if (err)
> > - goto out_drop_write;
> > + goto out;
> >
> > err = ovl_copy_up(new->d_parent);
> > if (err)
> > - goto out_drop_write;
> > + goto out;
> > if (!overwrite) {
> > err = ovl_copy_up(new);
> > if (err)
> > - goto out_drop_write;
> > + goto out;
> > } else if (d_inode(new)) {
> > err = ovl_nlink_start(new);
> > if (err)
> > - goto out_drop_write;
> > + goto out;
> >
> > update_nlink = true;
> > }
> >
> > + if (!update_nlink) {
> > + /* ovl_nlink_start() took ovl_want_write() */
> > + err = ovl_want_write(old);
> > + if (err)
> > + goto out;
> > + }
> > +
> > old_cred = ovl_override_creds(old->d_sb);
> >
> > if (!list_empty(&list)) {
> > @@ -1286,8 +1277,8 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
> > revert_creds(old_cred);
> > if (update_nlink)
> > ovl_nlink_end(new);
> > -out_drop_write:
> > - ovl_drop_write(old);
> > + else
> > + ovl_drop_write(old);
> > out:
> > dput(opaquedir);
> > ovl_cache_free(&list);
> > diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> > index c8c8588bd98c..4a79c479c971 100644
> > --- a/fs/overlayfs/export.c
> > +++ b/fs/overlayfs/export.c
> > @@ -23,12 +23,7 @@ static int ovl_encode_maybe_copy_up(struct dentry *dentry)
> > if (ovl_dentry_upper(dentry))
> > return 0;
> >
> > - err = ovl_want_write(dentry);
> > - if (!err) {
> > - err = ovl_copy_up(dentry);
> > - ovl_drop_write(dentry);
> > - }
> > -
> > + err = ovl_copy_up(dentry);
> > if (err) {
> > pr_warn_ratelimited("failed to copy up on encode (%pd2, err=%i)\n",
> > dentry, err);
> > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> > index b395cd84bfce..f5638cfe8f6d 100644
> > --- a/fs/overlayfs/inode.c
> > +++ b/fs/overlayfs/inode.c
> > @@ -32,10 +32,6 @@ int ovl_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> > if (err)
> > return err;
> >
> > - err = ovl_want_write(dentry);
> > - if (err)
> > - goto out;
> > -
> > if (attr->ia_valid & ATTR_SIZE) {
> > /* Truncate should trigger data copy up as well */
> > full_copy_up = true;
> > @@ -54,7 +50,7 @@ int ovl_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> > winode = d_inode(upperdentry);
> > err = get_write_access(winode);
> > if (err)
> > - goto out_drop_write;
> > + goto out;
> > }
> >
> > if (attr->ia_valid & (ATTR_KILL_SUID|ATTR_KILL_SGID))
> > @@ -78,6 +74,10 @@ int ovl_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> > */
> > attr->ia_valid &= ~ATTR_OPEN;
> >
> > + err = ovl_want_write(dentry);
> > + if (err)
> > + goto out;
>
> Need to put write access.
>
ok.
> > +
> > inode_lock(upperdentry->d_inode);
> > old_cred = ovl_override_creds(dentry->d_sb);
> > err = ovl_do_notify_change(ofs, upperdentry, attr);
> > @@ -85,12 +85,11 @@ int ovl_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> > if (!err)
> > ovl_copyattr(dentry->d_inode);
> > inode_unlock(upperdentry->d_inode);
> > + ovl_drop_write(dentry);
> >
> > if (winode)
> > put_write_access(winode);
> > }
> > -out_drop_write:
> > - ovl_drop_write(dentry);
> > out:
> > return err;
> > }
> > @@ -361,27 +360,27 @@ int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
> > struct path realpath;
> > const struct cred *old_cred;
> >
> > - err = ovl_want_write(dentry);
> > - if (err)
> > - goto out;
> > -
> > if (!value && !upperdentry) {
> > ovl_path_lower(dentry, &realpath);
> > old_cred = ovl_override_creds(dentry->d_sb);
> > err = vfs_getxattr(mnt_idmap(realpath.mnt), realdentry, name, NULL, 0);
> > revert_creds(old_cred);
> > if (err < 0)
> > - goto out_drop_write;
> > + goto out;
> > }
> >
> > if (!upperdentry) {
> > err = ovl_copy_up(dentry);
> > if (err)
> > - goto out_drop_write;
> > + goto out;
> >
> > realdentry = ovl_dentry_upper(dentry);
> > }
> >
> > + err = ovl_want_write(dentry);
> > + if (err)
> > + goto out;
> > +
> > old_cred = ovl_override_creds(dentry->d_sb);
> > if (value) {
> > err = ovl_do_setxattr(ofs, realdentry, name, value, size,
> > @@ -391,12 +390,10 @@ int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
> > err = ovl_do_removexattr(ofs, realdentry, name);
> > }
> > revert_creds(old_cred);
> > + ovl_drop_write(dentry);
> >
> > /* copy c/mtime */
> > ovl_copyattr(inode);
> > -
> > -out_drop_write:
> > - ovl_drop_write(dentry);
> > out:
> > return err;
> > }
> > @@ -611,10 +608,6 @@ static int ovl_set_or_remove_acl(struct dentry *dentry, struct inode *inode,
> > struct dentry *upperdentry = ovl_dentry_upper(dentry);
> > struct dentry *realdentry = upperdentry ?: ovl_dentry_lower(dentry);
> >
> > - err = ovl_want_write(dentry);
> > - if (err)
> > - return err;
> > -
> > /*
> > * If ACL is to be removed from a lower file, check if it exists in
> > * the first place before copying it up.
> > @@ -630,7 +623,7 @@ static int ovl_set_or_remove_acl(struct dentry *dentry, struct inode *inode,
> > revert_creds(old_cred);
> > if (IS_ERR(real_acl)) {
> > err = PTR_ERR(real_acl);
> > - goto out_drop_write;
> > + goto out;
> > }
> > posix_acl_release(real_acl);
> > }
> > @@ -638,23 +631,26 @@ static int ovl_set_or_remove_acl(struct dentry *dentry, struct inode *inode,
> > if (!upperdentry) {
> > err = ovl_copy_up(dentry);
> > if (err)
> > - goto out_drop_write;
> > + goto out;
> >
> > realdentry = ovl_dentry_upper(dentry);
> > }
> >
> > + err = ovl_want_write(dentry);
> > + if (err)
> > + goto out;
> > +
> > old_cred = ovl_override_creds(dentry->d_sb);
> > if (acl)
> > err = ovl_do_set_acl(ofs, realdentry, acl_name, acl);
> > else
> > err = ovl_do_remove_acl(ofs, realdentry, acl_name);
> > revert_creds(old_cred);
> > + ovl_drop_write(dentry);
> >
> > /* copy c/mtime */
> > ovl_copyattr(inode);
> > -
> > -out_drop_write:
> > - ovl_drop_write(dentry);
> > +out:
> > return err;
> > }
> >
> > @@ -777,14 +773,14 @@ int ovl_fileattr_set(struct mnt_idmap *idmap,
> > unsigned int flags;
> > int err;
> >
> > - err = ovl_want_write(dentry);
> > - if (err)
> > - goto out;
> > -
> > err = ovl_copy_up(dentry);
> > if (!err) {
> > ovl_path_real(dentry, &upperpath);
> >
> > + err = ovl_want_write(dentry);
> > + if (err)
> > + goto out;
> > +
> > old_cred = ovl_override_creds(inode->i_sb);
> > /*
> > * Store immutable/append-only flags in xattr and clear them
> > @@ -797,6 +793,7 @@ int ovl_fileattr_set(struct mnt_idmap *idmap,
> > if (!err)
> > err = ovl_real_fileattr_set(&upperpath, fa);
> > revert_creds(old_cred);
> > + ovl_drop_write(dentry);
> >
> > /*
> > * Merge real inode flags with inode flags read from
> > @@ -811,7 +808,6 @@ int ovl_fileattr_set(struct mnt_idmap *idmap,
> > /* Update ctime */
> > ovl_copyattr(inode);
> > }
> > - ovl_drop_write(dentry);
> > out:
> > return err;
> > }
> > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> > index 0f387092450e..4deed8a2a112 100644
> > --- a/fs/overlayfs/util.c
> > +++ b/fs/overlayfs/util.c
> > @@ -1062,6 +1062,10 @@ int ovl_nlink_start(struct dentry *dentry)
> > if (err)
> > return err;
> >
> > + err = ovl_want_write(dentry);
> > + if (err)
> > + goto out;
>
> Need to unlock.
goto out does unlock, but I will make this more explicit
with out_unlock label.
>
>
> > +
> > if (d_is_dir(dentry) || !ovl_test_flag(OVL_INDEX, inode))
> > goto out;
>
> Need to drop write.
This one is not an error case, it is a success and it returns with
both inode lock and sb write held, so it can just be return 0;
>
> >
> > @@ -1074,6 +1078,8 @@ int ovl_nlink_start(struct dentry *dentry)
> > */
> > err = ovl_set_nlink_upper(dentry);
> > revert_creds(old_cred);
> > + if (err)
> > + ovl_drop_write(dentry);
> >
> > out:
> > if (err)
>
> I'd just separate out error handling into separate labels.
>
OK.
I hope the two other patches have less mistakes :-/
Thanks,
Amir.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] ovl: do not open/llseek lower file with upper sb_writers held
2023-08-14 14:05 ` [PATCH v2 2/3] ovl: do not open/llseek lower file with upper sb_writers held Amir Goldstein
@ 2023-08-15 15:12 ` Miklos Szeredi
2023-08-15 15:59 ` Amir Goldstein
0 siblings, 1 reply; 15+ messages in thread
From: Miklos Szeredi @ 2023-08-15 15:12 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Jan Kara, linux-unionfs
On Mon, 14 Aug 2023 at 16:05, Amir Goldstein <amir73il@gmail.com> wrote:
>
> overlayfs file open (ovl_maybe_lookup_lowerdata) and overlay file llseek
> take the ovl_inode_lock, without holding upper sb_writers.
>
> In case of nested lower overlay that uses same upper fs as this overlay,
> lockdep will warn about (possibly false positive) circular lock
> dependency when doing open/llseek of lower ovl file during copy up with
> our upper sb_writers held, because the locking ordering seems reverse to
> the locking order in ovl_copy_up_start():
>
> - lower ovl_inode_lock
> - upper sb_writers
>
> Take upper sb_writers only when we actually need it, so we won't hold it
> during lower file open and lower file llseek to avoid the lockdep warning.
>
> Minimizing the scope of ovl_want_write() during copy up is also needed
> for fixing other possible deadlocks by following patches.
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
> fs/overlayfs/copy_up.c | 117 +++++++++++++++++++++++++++++++----------
> 1 file changed, 88 insertions(+), 29 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index c998dab440f8..f2a31ff790fb 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -251,8 +251,13 @@ static int ovl_copy_up_file(struct ovl_fs *ofs, struct dentry *dentry,
> if (IS_ERR(old_file))
> return PTR_ERR(old_file);
>
> + error = ovl_want_write(dentry);
> + if (error)
> + goto out_fput;
What occurs to me is why are we bothering with getting write access on
the internal upper mnt each time. Seems to me it's a historical thing
without a good reason. Upper mnt is never changed from R/W to R/O.
So the only thing we need to do is grab the upper mount write access
on superblock creation and do the sb_start_write/end_write() thing
which can't fail. If upper mnt is read-only, we effectively have a
read-only filesystem, and can handle it that way (sb->s_flags |=
SB_RDONLY).
There's still the possibility that we do some changes to upper even
for non-modify operations. But with careful review we can remove a
most (possibly all) error handling cases from ovl_want_write()
callsites when we do know that we have write access on upper. And
WARN_ON(__mnt_is_readonly(ovl_upper_mnt(ofs))) should ensure that we
catch any mistakes.
Hmm?
Thanks,
Miklos
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] ovl: do not open/llseek lower file with upper sb_writers held
2023-08-15 15:12 ` Miklos Szeredi
@ 2023-08-15 15:59 ` Amir Goldstein
2023-08-15 19:07 ` Miklos Szeredi
2023-08-16 10:34 ` Christian Brauner
0 siblings, 2 replies; 15+ messages in thread
From: Amir Goldstein @ 2023-08-15 15:59 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Jan Kara, linux-unionfs, Christian Brauner
[cc Christian]
On Tue, Aug 15, 2023 at 6:12 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, 14 Aug 2023 at 16:05, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > overlayfs file open (ovl_maybe_lookup_lowerdata) and overlay file llseek
> > take the ovl_inode_lock, without holding upper sb_writers.
> >
> > In case of nested lower overlay that uses same upper fs as this overlay,
> > lockdep will warn about (possibly false positive) circular lock
> > dependency when doing open/llseek of lower ovl file during copy up with
> > our upper sb_writers held, because the locking ordering seems reverse to
> > the locking order in ovl_copy_up_start():
> >
> > - lower ovl_inode_lock
> > - upper sb_writers
> >
> > Take upper sb_writers only when we actually need it, so we won't hold it
> > during lower file open and lower file llseek to avoid the lockdep warning.
> >
> > Minimizing the scope of ovl_want_write() during copy up is also needed
> > for fixing other possible deadlocks by following patches.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> > fs/overlayfs/copy_up.c | 117 +++++++++++++++++++++++++++++++----------
> > 1 file changed, 88 insertions(+), 29 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index c998dab440f8..f2a31ff790fb 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -251,8 +251,13 @@ static int ovl_copy_up_file(struct ovl_fs *ofs, struct dentry *dentry,
> > if (IS_ERR(old_file))
> > return PTR_ERR(old_file);
> >
> > + error = ovl_want_write(dentry);
> > + if (error)
> > + goto out_fput;
>
> What occurs to me is why are we bothering with getting write access on
> the internal upper mnt each time. Seems to me it's a historical thing
> without a good reason. Upper mnt is never changed from R/W to R/O.
>
> So the only thing we need to do is grab the upper mount write access
> on superblock creation and do the sb_start_write/end_write() thing
> which can't fail. If upper mnt is read-only, we effectively have a
> read-only filesystem, and can handle it that way (sb->s_flags |=
> SB_RDONLY).
>
> There's still the possibility that we do some changes to upper even
> for non-modify operations. But with careful review we can remove a
> most (possibly all) error handling cases from ovl_want_write()
> callsites when we do know that we have write access on upper. And
> WARN_ON(__mnt_is_readonly(ovl_upper_mnt(ofs))) should ensure that we
> catch any mistakes.
>
> Hmm?
>
I was thinking the same thing myself, before I went on this journey.
I reached the conclusion that doing only sb_start_write() would not be
safe against emergency remount rdonly of the upper sb.
I guess if upper sb is emergency mounted rdonly, then overlayfs
sb would also be emergency remounted rdonly, but for example
ext4 sb can become rdonly on internal errors.
But maybe that is not the responsibility of vfs or ovl to care about?
Christian, is there also an API to set the sb rdonly when private
writable mounts (i.e. ovl_upper_mnt) exist?
Thanks,
Amir.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] ovl: do not open/llseek lower file with upper sb_writers held
2023-08-15 15:59 ` Amir Goldstein
@ 2023-08-15 19:07 ` Miklos Szeredi
2023-08-15 19:51 ` Amir Goldstein
2023-08-16 15:02 ` Amir Goldstein
2023-08-16 10:34 ` Christian Brauner
1 sibling, 2 replies; 15+ messages in thread
From: Miklos Szeredi @ 2023-08-15 19:07 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Jan Kara, linux-unionfs, Christian Brauner
On Tue, 15 Aug 2023 at 17:59, Amir Goldstein <amir73il@gmail.com> wrote:
> > What occurs to me is why are we bothering with getting write access on
> > the internal upper mnt each time. Seems to me it's a historical thing
> > without a good reason. Upper mnt is never changed from R/W to R/O.
> >
> > So the only thing we need to do is grab the upper mount write access
> > on superblock creation and do the sb_start_write/end_write() thing
> > which can't fail. If upper mnt is read-only, we effectively have a
> > read-only filesystem, and can handle it that way (sb->s_flags |=
> > SB_RDONLY).
> >
> > There's still the possibility that we do some changes to upper even
> > for non-modify operations. But with careful review we can remove a
> > most (possibly all) error handling cases from ovl_want_write()
> > callsites when we do know that we have write access on upper. And
> > WARN_ON(__mnt_is_readonly(ovl_upper_mnt(ofs))) should ensure that we
> > catch any mistakes.
> >
> > Hmm?
> >
>
> I was thinking the same thing myself, before I went on this journey.
> I reached the conclusion that doing only sb_start_write() would not be
> safe against emergency remount rdonly of the upper sb.
>
> I guess if upper sb is emergency mounted rdonly, then overlayfs
> sb would also be emergency remounted rdonly, but for example
> ext4 sb can become rdonly on internal errors.
> But maybe that is not the responsibility of vfs or ovl to care about?
Consider the case of a writable open file: the mount write access is
only checked on open. So not having fine grained mnt write access
checks is not without precedent.
I'm not sure, but the number of added lines in this particular patch
makes me think that at least during copy-up we could separate the mnt
and the sb write locks.
> Christian, is there also an API to set the sb rdonly when private
> writable mounts (i.e. ovl_upper_mnt) exist?
You mean notifying overlayfs about rdonly remount of upper mnt? No,
that doesn't exist today.
Thanks,
Miklos
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] ovl: do not open/llseek lower file with upper sb_writers held
2023-08-15 19:07 ` Miklos Szeredi
@ 2023-08-15 19:51 ` Amir Goldstein
2023-08-15 20:36 ` Miklos Szeredi
2023-08-16 15:02 ` Amir Goldstein
1 sibling, 1 reply; 15+ messages in thread
From: Amir Goldstein @ 2023-08-15 19:51 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Jan Kara, linux-unionfs, Christian Brauner
On Tue, Aug 15, 2023 at 10:07 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Tue, 15 Aug 2023 at 17:59, Amir Goldstein <amir73il@gmail.com> wrote:
>
> > > What occurs to me is why are we bothering with getting write access on
> > > the internal upper mnt each time. Seems to me it's a historical thing
> > > without a good reason. Upper mnt is never changed from R/W to R/O.
> > >
> > > So the only thing we need to do is grab the upper mount write access
> > > on superblock creation and do the sb_start_write/end_write() thing
> > > which can't fail. If upper mnt is read-only, we effectively have a
> > > read-only filesystem, and can handle it that way (sb->s_flags |=
> > > SB_RDONLY).
> > >
> > > There's still the possibility that we do some changes to upper even
> > > for non-modify operations. But with careful review we can remove a
> > > most (possibly all) error handling cases from ovl_want_write()
> > > callsites when we do know that we have write access on upper. And
> > > WARN_ON(__mnt_is_readonly(ovl_upper_mnt(ofs))) should ensure that we
> > > catch any mistakes.
> > >
> > > Hmm?
> > >
> >
> > I was thinking the same thing myself, before I went on this journey.
> > I reached the conclusion that doing only sb_start_write() would not be
> > safe against emergency remount rdonly of the upper sb.
> >
> > I guess if upper sb is emergency mounted rdonly, then overlayfs
> > sb would also be emergency remounted rdonly, but for example
> > ext4 sb can become rdonly on internal errors.
> > But maybe that is not the responsibility of vfs or ovl to care about?
>
> Consider the case of a writable open file: the mount write access is
> only checked on open. So not having fine grained mnt write access
> checks is not without precedent.
>
That's true.
I see that at least ext4_sync_file() and ext4_do_writepages()
test the EXT4_MF_FS_ABORTED case specifically to return EROFS.
> I'm not sure, but the number of added lines in this particular patch
> makes me think that at least during copy-up we could separate the mnt
> and the sb write locks.
>
Yeh, I think that makes a lot of sense.
> > Christian, is there also an API to set the sb rdonly when private
> > writable mounts (i.e. ovl_upper_mnt) exist?
>
> You mean notifying overlayfs about rdonly remount of upper mnt? No,
> that doesn't exist today.
>
What I meant is, except from emergency remount rdonly and fs specific
cases like ext4 remount-ro on error, is there a way via new mount API
that users can request remount of the upper SB rdonly, despite the
fact that this sb has private writable mount clones?
even if ovl has elevated mnt_writers of upper_mnt?
Thanks,
Amir.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] ovl: do not open/llseek lower file with upper sb_writers held
2023-08-15 19:51 ` Amir Goldstein
@ 2023-08-15 20:36 ` Miklos Szeredi
2023-08-16 10:28 ` Christian Brauner
0 siblings, 1 reply; 15+ messages in thread
From: Miklos Szeredi @ 2023-08-15 20:36 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Jan Kara, linux-unionfs, Christian Brauner
On Tue, 15 Aug 2023 at 21:51, Amir Goldstein <amir73il@gmail.com> wrote:
> What I meant is, except from emergency remount rdonly and fs specific
> cases like ext4 remount-ro on error, is there a way via new mount API
> that users can request remount of the upper SB rdonly, despite the
> fact that this sb has private writable mount clones?
> even if ovl has elevated mnt_writers of upper_mnt?
Private and public mounts are completely equal in this regard. So no,
you can't remount rdonly if upper mnt has an elevated mnt_writers.
Thanks,
Miklos
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] ovl: do not open/llseek lower file with upper sb_writers held
2023-08-15 20:36 ` Miklos Szeredi
@ 2023-08-16 10:28 ` Christian Brauner
0 siblings, 0 replies; 15+ messages in thread
From: Christian Brauner @ 2023-08-16 10:28 UTC (permalink / raw)
To: Miklos Szeredi, Amir Goldstein; +Cc: Jan Kara, linux-unionfs
On Tue, Aug 15, 2023 at 10:36:52PM +0200, Miklos Szeredi wrote:
> On Tue, 15 Aug 2023 at 21:51, Amir Goldstein <amir73il@gmail.com> wrote:
>
> > What I meant is, except from emergency remount rdonly and fs specific
> > cases like ext4 remount-ro on error, is there a way via new mount API
> > that users can request remount of the upper SB rdonly, despite the
> > fact that this sb has private writable mount clones?
> > even if ovl has elevated mnt_writers of upper_mnt?
>
> Private and public mounts are completely equal in this regard. So no,
> you can't remount rdonly if upper mnt has an elevated mnt_writers.
It's worth noting that during _emergency_ rw->ro remount writers on
mounts are ignored.
IOW, if SB_FORCE is set then we don't call sb_prepare_remount_readonly()
which would otherwise walk all mounts of the superblock marking them as
about to be made ro and would fail the rw->ro if there are any active
writers.
And a real emergency ro remount would make all active superblocks ro
including any overlayfs ones.
But for a regular rw->ro any writable mount with active writers would
prevent the sb from going ro.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] ovl: do not open/llseek lower file with upper sb_writers held
2023-08-15 15:59 ` Amir Goldstein
2023-08-15 19:07 ` Miklos Szeredi
@ 2023-08-16 10:34 ` Christian Brauner
1 sibling, 0 replies; 15+ messages in thread
From: Christian Brauner @ 2023-08-16 10:34 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Miklos Szeredi, Jan Kara, linux-unionfs
On Tue, Aug 15, 2023 at 06:59:44PM +0300, Amir Goldstein wrote:
> [cc Christian]
>
> On Tue, Aug 15, 2023 at 6:12 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Mon, 14 Aug 2023 at 16:05, Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > overlayfs file open (ovl_maybe_lookup_lowerdata) and overlay file llseek
> > > take the ovl_inode_lock, without holding upper sb_writers.
> > >
> > > In case of nested lower overlay that uses same upper fs as this overlay,
> > > lockdep will warn about (possibly false positive) circular lock
> > > dependency when doing open/llseek of lower ovl file during copy up with
> > > our upper sb_writers held, because the locking ordering seems reverse to
> > > the locking order in ovl_copy_up_start():
> > >
> > > - lower ovl_inode_lock
> > > - upper sb_writers
> > >
> > > Take upper sb_writers only when we actually need it, so we won't hold it
> > > during lower file open and lower file llseek to avoid the lockdep warning.
> > >
> > > Minimizing the scope of ovl_want_write() during copy up is also needed
> > > for fixing other possible deadlocks by following patches.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > ---
> > > fs/overlayfs/copy_up.c | 117 +++++++++++++++++++++++++++++++----------
> > > 1 file changed, 88 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > > index c998dab440f8..f2a31ff790fb 100644
> > > --- a/fs/overlayfs/copy_up.c
> > > +++ b/fs/overlayfs/copy_up.c
> > > @@ -251,8 +251,13 @@ static int ovl_copy_up_file(struct ovl_fs *ofs, struct dentry *dentry,
> > > if (IS_ERR(old_file))
> > > return PTR_ERR(old_file);
> > >
> > > + error = ovl_want_write(dentry);
> > > + if (error)
> > > + goto out_fput;
> >
> > What occurs to me is why are we bothering with getting write access on
> > the internal upper mnt each time. Seems to me it's a historical thing
> > without a good reason. Upper mnt is never changed from R/W to R/O.
> >
> > So the only thing we need to do is grab the upper mount write access
> > on superblock creation and do the sb_start_write/end_write() thing
Yes, that should work for fine afaict. I think that overlayfs
conceptually is equivalent to a permanent writer on that mount where
write access is granted during mount.
(I guess overlayfs could yield write access to the underlying mounts
when it gets an SB_FORCE/emergency remount request.)
> > which can't fail. If upper mnt is read-only, we effectively have a
> > read-only filesystem, and can handle it that way (sb->s_flags |=
> > SB_RDONLY).
> >
> > There's still the possibility that we do some changes to upper even
> > for non-modify operations. But with careful review we can remove a
> > most (possibly all) error handling cases from ovl_want_write()
> > callsites when we do know that we have write access on upper. And
> > WARN_ON(__mnt_is_readonly(ovl_upper_mnt(ofs))) should ensure that we
> > catch any mistakes.
> >
> > Hmm?
> >
>
> I was thinking the same thing myself, before I went on this journey.
> I reached the conclusion that doing only sb_start_write() would not be
> safe against emergency remount rdonly of the upper sb.
>
> I guess if upper sb is emergency mounted rdonly, then overlayfs
> sb would also be emergency remounted rdonly, but for example
> ext4 sb can become rdonly on internal errors.
> But maybe that is not the responsibility of vfs or ovl to care about?
>
> Christian, is there also an API to set the sb rdonly when private
> writable mounts (i.e. ovl_upper_mnt) exist?
No, I don't think so (see my other mail for emergency remount). There's
definitely no public one as private mounts are "invisible" to userspace
and can't be interacted with.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] ovl: do not open/llseek lower file with upper sb_writers held
2023-08-15 19:07 ` Miklos Szeredi
2023-08-15 19:51 ` Amir Goldstein
@ 2023-08-16 15:02 ` Amir Goldstein
2023-09-08 12:29 ` Amir Goldstein
1 sibling, 1 reply; 15+ messages in thread
From: Amir Goldstein @ 2023-08-16 15:02 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Jan Kara, linux-unionfs, Christian Brauner
On Tue, Aug 15, 2023 at 10:07 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Tue, 15 Aug 2023 at 17:59, Amir Goldstein <amir73il@gmail.com> wrote:
>
> > > What occurs to me is why are we bothering with getting write access on
> > > the internal upper mnt each time. Seems to me it's a historical thing
> > > without a good reason. Upper mnt is never changed from R/W to R/O.
> > >
> > > So the only thing we need to do is grab the upper mount write access
> > > on superblock creation and do the sb_start_write/end_write() thing
> > > which can't fail. If upper mnt is read-only, we effectively have a
> > > read-only filesystem, and can handle it that way (sb->s_flags |=
> > > SB_RDONLY).
> > >
> > > There's still the possibility that we do some changes to upper even
> > > for non-modify operations. But with careful review we can remove a
> > > most (possibly all) error handling cases from ovl_want_write()
> > > callsites when we do know that we have write access on upper. And
> > > WARN_ON(__mnt_is_readonly(ovl_upper_mnt(ofs))) should ensure that we
> > > catch any mistakes.
> > >
> > > Hmm?
> > >
> >
> > I was thinking the same thing myself, before I went on this journey.
> > I reached the conclusion that doing only sb_start_write() would not be
> > safe against emergency remount rdonly of the upper sb.
> >
> > I guess if upper sb is emergency mounted rdonly, then overlayfs
> > sb would also be emergency remounted rdonly, but for example
> > ext4 sb can become rdonly on internal errors.
> > But maybe that is not the responsibility of vfs or ovl to care about?
>
> Consider the case of a writable open file: the mount write access is
> only checked on open. So not having fine grained mnt write access
> checks is not without precedent.
>
> I'm not sure, but the number of added lines in this particular patch
> makes me think that at least during copy-up we could separate the mnt
> and the sb write locks.
>
The patch with separate locks during copy-up is not much smaller
but it is a lot nicer IMO:
https://github.com/amir73il/linux/commits/ovl_want_write-v3
I shall post these shortly after tests are complete.
Thanks,
Amir.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] ovl: do not open/llseek lower file with upper sb_writers held
2023-08-16 15:02 ` Amir Goldstein
@ 2023-09-08 12:29 ` Amir Goldstein
0 siblings, 0 replies; 15+ messages in thread
From: Amir Goldstein @ 2023-09-08 12:29 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Jan Kara, linux-unionfs, Christian Brauner
On Wed, Aug 16, 2023 at 6:02 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Aug 15, 2023 at 10:07 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Tue, 15 Aug 2023 at 17:59, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > > > What occurs to me is why are we bothering with getting write access on
> > > > the internal upper mnt each time. Seems to me it's a historical thing
> > > > without a good reason. Upper mnt is never changed from R/W to R/O.
> > > >
> > > > So the only thing we need to do is grab the upper mount write access
> > > > on superblock creation and do the sb_start_write/end_write() thing
> > > > which can't fail. If upper mnt is read-only, we effectively have a
> > > > read-only filesystem, and can handle it that way (sb->s_flags |=
> > > > SB_RDONLY).
> > > >
> > > > There's still the possibility that we do some changes to upper even
> > > > for non-modify operations. But with careful review we can remove a
> > > > most (possibly all) error handling cases from ovl_want_write()
> > > > callsites when we do know that we have write access on upper. And
> > > > WARN_ON(__mnt_is_readonly(ovl_upper_mnt(ofs))) should ensure that we
> > > > catch any mistakes.
> > > >
> > > > Hmm?
> > > >
> > >
> > > I was thinking the same thing myself, before I went on this journey.
> > > I reached the conclusion that doing only sb_start_write() would not be
> > > safe against emergency remount rdonly of the upper sb.
> > >
> > > I guess if upper sb is emergency mounted rdonly, then overlayfs
> > > sb would also be emergency remounted rdonly, but for example
> > > ext4 sb can become rdonly on internal errors.
> > > But maybe that is not the responsibility of vfs or ovl to care about?
> >
> > Consider the case of a writable open file: the mount write access is
> > only checked on open. So not having fine grained mnt write access
> > checks is not without precedent.
> >
> > I'm not sure, but the number of added lines in this particular patch
> > makes me think that at least during copy-up we could separate the mnt
> > and the sb write locks.
> >
>
> The patch with separate locks during copy-up is not much smaller
> but it is a lot nicer IMO:
>
> https://github.com/amir73il/linux/commits/ovl_want_write-v3
>
> I shall post these shortly after tests are complete.
>
Hi Miklos,
Did you get a change to review v3 patches [1] with the split of
ovl_want_write() to ovl_get_mnt_write() and ovl_start_write()?
I would like to queue this lock ordering change for 6.7.
Thanks,
Amir.
[1] https://lore.kernel.org/linux-unionfs/20230816152334.924960-1-amir73il@gmail.com/
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2023-09-08 12:29 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-14 14:05 [PATCH v2 0/3] overlayfs lock ordering changes Amir Goldstein
2023-08-14 14:05 ` [PATCH v2 1/3] ovl: reorder ovl_want_write() after ovl_inode_lock() Amir Goldstein
2023-08-15 10:50 ` Miklos Szeredi
2023-08-15 15:12 ` Amir Goldstein
2023-08-14 14:05 ` [PATCH v2 2/3] ovl: do not open/llseek lower file with upper sb_writers held Amir Goldstein
2023-08-15 15:12 ` Miklos Szeredi
2023-08-15 15:59 ` Amir Goldstein
2023-08-15 19:07 ` Miklos Szeredi
2023-08-15 19:51 ` Amir Goldstein
2023-08-15 20:36 ` Miklos Szeredi
2023-08-16 10:28 ` Christian Brauner
2023-08-16 15:02 ` Amir Goldstein
2023-09-08 12:29 ` Amir Goldstein
2023-08-16 10:34 ` Christian Brauner
2023-08-14 14:05 ` [PATCH v2 3/3] ovl: do not encode lower fh " Amir Goldstein
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox