| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20191130034339.GI20752@bombadil.infradead.org>
Date: Fri, 29 Nov 2019 19:43:39 -0800
From: Matthew Wilcox <willy@...radead.org>
To: yu kuai <yukuai3@...wei.com>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org,
viro@...iv.linux.org.uk, rostedt@...dmis.org, oleg@...hat.com,
mchehab+samsung@...nel.org, corbet@....net, tytso@....edu,
jmorris@...ei.org, linux-kernel@...r.kernel.org,
linux-fsdevel@...r.kernel.org, zhengbin13@...wei.com,
yi.zhang@...wei.com, chenxiang66@...ilicon.com, xiexiuqi@...wei.com
Subject: Re: [PATCH V2 1/3] dcache: add a new enum type for
'dentry_d_lock_class'
On Sat, Nov 30, 2019 at 10:02:23AM +0800, yu kuai wrote:
> However, a single 'DENTRY_D_LOCK_NESTED' may not be enough if more than
> two dentry are involed. So, add in 'DENTRY_D_LOCK_NESTED_TWICE'.
No. These need meaningful names. Indeed, I think D_LOCK_NESTED is
a terrible name.
Looking at the calls:
$ git grep -n nested.*d_lock fs
fs/autofs/expire.c:82: spin_lock_nested(&child->d_lock, DENTRY_D_LOCK_NESTED);
fs/dcache.c:619: spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_NESTED);
fs/dcache.c:1086: spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_NESTED);
fs/dcache.c:1303: spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_NESTED);
fs/dcache.c:2822: spin_lock_nested(&old_parent->d_lock, DENTRY_D_LOCK_NESTED);
fs/dcache.c:2827: spin_lock_nested(&target->d_parent->d_lock,
fs/dcache.c:2830: spin_lock_nested(&dentry->d_lock, 2);
fs/dcache.c:2831: spin_lock_nested(&target->d_lock, 3);
fs/dcache.c:3121: spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_NESTED);
fs/libfs.c:112: spin_lock_nested(&d->d_lock, DENTRY_D_LOCK_NESTED);
fs/libfs.c:341: spin_lock_nested(&child->d_lock, DENTRY_D_LOCK_NESTED);
fs/notify/fsnotify.c:129: spin_lock_nested(&child->d_lock, DENTRY_D_LOCK_NESTED);
Most of these would be well-expressed by DENTRY_D_LOCK_CHILD.
The exception is __d_move() where I think we should actually name the
different lock classes instead of using a bare '2' and '3'. Something
like this, perhaps:
diff --git a/fs/autofs/expire.c b/fs/autofs/expire.c
index 2866fabf497f..f604175243eb 100644
--- a/fs/autofs/expire.c
+++ b/fs/autofs/expire.c
@@ -79,7 +79,7 @@ static struct dentry *positive_after(struct dentry *p, struct dentry *child)
child = list_first_entry(&p->d_subdirs, struct dentry, d_child);
list_for_each_entry_from(child, &p->d_subdirs, d_child) {
- spin_lock_nested(&child->d_lock, DENTRY_D_LOCK_NESTED);
+ spin_lock_nested(&child->d_lock, DENTRY_D_LOCK_CHILD);
if (simple_positive(child)) {
dget_dlock(child);
spin_unlock(&child->d_lock);
diff --git a/fs/dcache.c b/fs/dcache.c
index e88cf0554e65..c73b7d7bc785 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -616,7 +616,7 @@ static struct dentry *__lock_parent(struct dentry *dentry)
}
rcu_read_unlock();
if (parent != dentry)
- spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_NESTED);
+ spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_CHILD);
else
parent = NULL;
return parent;
@@ -1083,7 +1083,7 @@ static bool shrink_lock_dentry(struct dentry *dentry)
spin_lock(&dentry->d_lock);
goto out;
}
- spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_NESTED);
+ spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_CHILD);
if (likely(!dentry->d_lockref.count))
return true;
spin_unlock(&parent->d_lock);
@@ -1300,7 +1300,7 @@ static void d_walk(struct dentry *parent, void *data,
if (unlikely(dentry->d_flags & DCACHE_DENTRY_CURSOR))
continue;
- spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_NESTED);
+ spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_CHILD);
ret = enter(data, dentry);
switch (ret) {
@@ -2819,16 +2819,16 @@ static void __d_move(struct dentry *dentry, struct dentry *target,
} else if (!p) {
/* target is not a descendent of dentry->d_parent */
spin_lock(&target->d_parent->d_lock);
- spin_lock_nested(&old_parent->d_lock, DENTRY_D_LOCK_NESTED);
+ spin_lock_nested(&old_parent->d_lock, DENTRY_D_LOCK_PARENT_2);
} else {
BUG_ON(p == dentry);
spin_lock(&old_parent->d_lock);
if (p != target)
spin_lock_nested(&target->d_parent->d_lock,
- DENTRY_D_LOCK_NESTED);
+ DENTRY_D_LOCK_PARENT_2);
}
- spin_lock_nested(&dentry->d_lock, 2);
- spin_lock_nested(&target->d_lock, 3);
+ spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_CHILD);
+ spin_lock_nested(&target->d_lock, DENTRY_D_LOCK_CHILD_2);
if (unlikely(d_in_lookup(target))) {
dir = target->d_parent->d_inode;
@@ -2837,7 +2837,7 @@ static void __d_move(struct dentry *dentry, struct dentry *target,
}
write_seqcount_begin(&dentry->d_seq);
- write_seqcount_begin_nested(&target->d_seq, DENTRY_D_LOCK_NESTED);
+ write_seqcount_begin_nested(&target->d_seq, DENTRY_D_LOCK_CHILD);
/* unhash both */
if (!d_unhashed(dentry))
@@ -3118,7 +3118,7 @@ void d_tmpfile(struct dentry *dentry, struct inode *inode)
!hlist_unhashed(&dentry->d_u.d_alias) ||
!d_unlinked(dentry));
spin_lock(&dentry->d_parent->d_lock);
- spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_NESTED);
+ spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_CHILD);
dentry->d_name.len = sprintf(dentry->d_iname, "#%llu",
(unsigned long long)inode->i_ino);
spin_unlock(&dentry->d_lock);
diff --git a/fs/libfs.c b/fs/libfs.c
index 1463b038ffc4..c68dedbf4ad2 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -109,7 +109,7 @@ static struct dentry *scan_positives(struct dentry *cursor,
if (d->d_flags & DCACHE_DENTRY_CURSOR)
continue;
if (simple_positive(d) && !--count) {
- spin_lock_nested(&d->d_lock, DENTRY_D_LOCK_NESTED);
+ spin_lock_nested(&d->d_lock, DENTRY_D_LOCK_CHILD);
if (simple_positive(d))
found = dget_dlock(d);
spin_unlock(&d->d_lock);
@@ -336,9 +336,9 @@ int simple_empty(struct dentry *dentry)
struct dentry *child;
int ret = 0;
- spin_lock(&dentry->d_lock);
+ spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_PARENT_2);
list_for_each_entry(child, &dentry->d_subdirs, d_child) {
- spin_lock_nested(&child->d_lock, DENTRY_D_LOCK_NESTED);
+ spin_lock_nested(&child->d_lock, DENTRY_D_LOCK_CHILD_2);
if (simple_positive(child)) {
spin_unlock(&child->d_lock);
goto out;
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 2ecef6155fc0..23492f2e4915 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -126,7 +126,7 @@ void __fsnotify_update_child_dentry_flags(struct inode *inode)
if (!child->d_inode)
continue;
- spin_lock_nested(&child->d_lock, DENTRY_D_LOCK_NESTED);
+ spin_lock_nested(&child->d_lock, DENTRY_D_LOCK_CHILD);
if (watched)
child->d_flags |= DCACHE_FSNOTIFY_PARENT_WATCHED;
else
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 10090f11ab95..6a497c63bd38 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -121,15 +121,20 @@ struct dentry {
} __randomize_layout;
/*
- * dentry->d_lock spinlock nesting subclasses:
+ * dentry->d_lock spinlock nesting subclasses. Always taken in increasing
+ * order although some subclasses may be skipped.
*
* 0: normal
- * 1: nested
+ * 1: either a descendent of "normal" or a cousin.
+ * 2: child of the "normal" dentry
+ * 3: child of the "parent2" dentry
*/
enum dentry_d_lock_class
{
- DENTRY_D_LOCK_NORMAL, /* implicitly used by plain spin_lock() APIs. */
- DENTRY_D_LOCK_NESTED
+ DENTRY_D_LOCK_NORMAL, /* implicitly used by plain spin_lock() APIs */
+ DENTRY_D_LOCK_PARENT_2, /* not an ancestor of parent */
+ DENTRY_D_LOCK_CHILD, /* nests under parent's lock */
+ DENTRY_D_LOCK_CHILD_2, /* PARENT_2's child */
};
struct dentry_operations {
Powered by blists - more mailing lists