[PATCH v2 0/4] selinux: fix race when removing selinuxfs entries

[PATCH v2 0/4] selinux: fix race when removing selinuxfs entries

[Ondrej Mosnacek]

After hours and hours of getting familiar with dcache and debugging,
I think I finally found a solution that works and hopefully stands a
chance of being committed.

The series still doesn't address the lack of atomicity of the policy
reload transition, but this is part of a wider problem and can be
resolved later. Let's fix at least the userspace-triggered lockup
first.

Changes since v1:
 - switch to hopefully proper and actually working solution instead
   of the horrible mess I produced last time...
v1: https://lore.kernel.org/selinux/20181002111830.26342-1-omosnace@redhat.com/T/

Ondrej Mosnacek (4):
  d_walk: optionally lock also parent inode
  d_walk: add leave callback
  dcache: introduce d_genocide_safe()
  selinux: use d_genocide_safe() in selinuxfs

 fs/dcache.c                  | 87 +++++++++++++++++++++++++++++++-----
 include/linux/dcache.h       |  1 +
 security/selinux/selinuxfs.c |  2 +-
 3 files changed, 77 insertions(+), 13 deletions(-)

-- 
2.21.0

[PATCH v2 1/4] d_walk: optionally lock also parent inode

[Ondrej Mosnacek]

This will be used in a later patch to provide a function to safely
perform d_genocide on live trees.

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 fs/dcache.c | 43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index e88cf0554e65..9ed4c0f99e57 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1259,12 +1259,13 @@ enum d_walk_ret {
 /**
  * d_walk - walk the dentry tree
  * @parent:	start of walk
+ * @lock_inode	whether to lock also parent inode
  * @data:	data passed to @enter() and @finish()
  * @enter:	callback when first entering the dentry
  *
  * The @enter() callbacks are called with d_lock held.
  */
-static void d_walk(struct dentry *parent, void *data,
+static void d_walk(struct dentry *parent, bool lock_inode, void *data,
 		   enum d_walk_ret (*enter)(void *, struct dentry *))
 {
 	struct dentry *this_parent;
@@ -1276,6 +1277,8 @@ static void d_walk(struct dentry *parent, void *data,
 again:
 	read_seqbegin_or_lock(&rename_lock, &seq);
 	this_parent = parent;
+	if (lock_inode)
+		inode_lock(this_parent->d_inode);
 	spin_lock(&this_parent->d_lock);
 
 	ret = enter(data, this_parent);
@@ -1319,9 +1322,21 @@ resume:
 
 		if (!list_empty(&dentry->d_subdirs)) {
 			spin_unlock(&this_parent->d_lock);
-			spin_release(&dentry->d_lock.dep_map, 1, _RET_IP_);
+			if (lock_inode) {
+				spin_unlock(&dentry->d_lock);
+				inode_unlock(this_parent->d_inode);
+			} else {
+				spin_release(&dentry->d_lock.dep_map,
+					     1, _RET_IP_);
+			}
 			this_parent = dentry;
-			spin_acquire(&this_parent->d_lock.dep_map, 0, 1, _RET_IP_);
+			if (lock_inode) {
+				inode_lock(this_parent->d_inode);
+				spin_lock(&this_parent->d_lock);
+			} else {
+				spin_acquire(&this_parent->d_lock.dep_map,
+					     0, 1, _RET_IP_);
+			}
 			goto repeat;
 		}
 		spin_unlock(&dentry->d_lock);
@@ -1336,6 +1351,10 @@ ascend:
 		this_parent = child->d_parent;
 
 		spin_unlock(&child->d_lock);
+		if (lock_inode) {
+			inode_unlock(child->d_inode);
+			inode_lock(this_parent->d_inode);
+		}
 		spin_lock(&this_parent->d_lock);
 
 		/* might go back up the wrong parent if we have had a rename. */
@@ -1357,12 +1376,16 @@ ascend:
 
 out_unlock:
 	spin_unlock(&this_parent->d_lock);
+	if (lock_inode)
+		inode_unlock(this_parent->d_inode);
 	done_seqretry(&rename_lock, seq);
 	return;
 
 rename_retry:
-	spin_unlock(&this_parent->d_lock);
 	rcu_read_unlock();
+	spin_unlock(&this_parent->d_lock);
+	if (lock_inode)
+		inode_unlock(this_parent->d_inode);
 	BUG_ON(seq & 1);
 	if (!retry)
 		return;
@@ -1402,7 +1425,7 @@ int path_has_submounts(const struct path *parent)
 	struct check_mount data = { .mnt = parent->mnt, .mounted = 0 };
 
 	read_seqlock_excl(&mount_lock);
-	d_walk(parent->dentry, &data, path_check_mount);
+	d_walk(parent->dentry, false, &data, path_check_mount);
 	read_sequnlock_excl(&mount_lock);
 
 	return data.mounted;
@@ -1541,7 +1564,7 @@ void shrink_dcache_parent(struct dentry *parent)
 		struct select_data data = {.start = parent};
 
 		INIT_LIST_HEAD(&data.dispose);
-		d_walk(parent, &data, select_collect);
+		d_walk(parent, false, &data, select_collect);
 
 		if (!list_empty(&data.dispose)) {
 			shrink_dentry_list(&data.dispose);
@@ -1552,7 +1575,7 @@ void shrink_dcache_parent(struct dentry *parent)
 		if (!data.found)
 			break;
 		data.victim = NULL;
-		d_walk(parent, &data, select_collect2);
+		d_walk(parent, false, &data, select_collect2);
 		if (data.victim) {
 			struct dentry *parent;
 			spin_lock(&data.victim->d_lock);
@@ -1599,7 +1622,7 @@ static enum d_walk_ret umount_check(void *_data, struct dentry *dentry)
 static void do_one_tree(struct dentry *dentry)
 {
 	shrink_dcache_parent(dentry);
-	d_walk(dentry, dentry, umount_check);
+	d_walk(dentry, false, dentry, umount_check);
 	d_drop(dentry);
 	dput(dentry);
 }
@@ -1656,7 +1679,7 @@ void d_invalidate(struct dentry *dentry)
 	shrink_dcache_parent(dentry);
 	for (;;) {
 		struct dentry *victim = NULL;
-		d_walk(dentry, &victim, find_submount);
+		d_walk(dentry, false, &victim, find_submount);
 		if (!victim) {
 			if (had_submounts)
 				shrink_dcache_parent(dentry);
@@ -3106,7 +3129,7 @@ static enum d_walk_ret d_genocide_kill(void *data, struct dentry *dentry)
 
 void d_genocide(struct dentry *parent)
 {
-	d_walk(parent, parent, d_genocide_kill);
+	d_walk(parent, false, parent, d_genocide_kill);
 }
 
 EXPORT_SYMBOL(d_genocide);
-- 
2.21.0

Re: [PATCH v2 1/4] d_walk: optionally lock also parent inode

[Al Viro]

On Thu, Aug 01, 2019 at 04:02:40PM +0200, Ondrej Mosnacek wrote:
> @@ -1276,6 +1277,8 @@ static void d_walk(struct dentry *parent, void *data,
>  again:
>  	read_seqbegin_or_lock(&rename_lock, &seq);
>  	this_parent = parent;
> +	if (lock_inode)
> +		inode_lock(this_parent->d_inode);

Suppose we are on the second pass through that thing - with rename_lock held.
What will happen to that inode_lock?

Re: [PATCH v2 1/4] d_walk: optionally lock also parent inode

[Al Viro]

On Thu, Aug 01, 2019 at 04:02:40PM +0200, Ondrej Mosnacek wrote:
>  rename_retry:
> -	spin_unlock(&this_parent->d_lock);
>  	rcu_read_unlock();
> +	spin_unlock(&this_parent->d_lock);
> +	if (lock_inode)
> +		inode_unlock(this_parent->d_inode);

... and while we are at it, what's to keep this_parent positive here,
now that you've dropped ->d_lock on it?  Or to prevent it becoming
negative, then posiive _again_, with an unrelated inode?

[PATCH v2 2/4] d_walk: add leave callback

[Ondrej Mosnacek]

Add an optional callback that gets called when d_walk is *leaving* a
dentry. This will be used in a later patch to provide a function to
safely perform d_genocide on live trees.

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 fs/dcache.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 9ed4c0f99e57..70afcb6e6892 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1260,13 +1260,15 @@ enum d_walk_ret {
  * d_walk - walk the dentry tree
  * @parent:	start of walk
  * @lock_inode	whether to lock also parent inode
- * @data:	data passed to @enter() and @finish()
+ * @data:	data passed to @enter() and @leave()
  * @enter:	callback when first entering the dentry
+ * @leave:	callback when leaving the dentry
  *
  * The @enter() callbacks are called with d_lock held.
  */
 static void d_walk(struct dentry *parent, bool lock_inode, void *data,
-		   enum d_walk_ret (*enter)(void *, struct dentry *))
+		   enum d_walk_ret (*enter)(void *, struct dentry *),
+		   void (*leave)(void *, struct dentry *))
 {
 	struct dentry *this_parent;
 	struct list_head *next;
@@ -1339,6 +1341,8 @@ resume:
 			}
 			goto repeat;
 		}
+		if (leave)
+			leave(data, dentry);
 		spin_unlock(&dentry->d_lock);
 	}
 	/*
@@ -1350,6 +1354,8 @@ ascend:
 		struct dentry *child = this_parent;
 		this_parent = child->d_parent;
 
+		if (leave)
+			leave(data, child);
 		spin_unlock(&child->d_lock);
 		if (lock_inode) {
 			inode_unlock(child->d_inode);
@@ -1370,6 +1376,8 @@ ascend:
 		rcu_read_unlock();
 		goto resume;
 	}
+	if (leave)
+		leave(data, parent);
 	if (need_seqretry(&rename_lock, seq))
 		goto rename_retry;
 	rcu_read_unlock();
@@ -1425,7 +1433,7 @@ int path_has_submounts(const struct path *parent)
 	struct check_mount data = { .mnt = parent->mnt, .mounted = 0 };
 
 	read_seqlock_excl(&mount_lock);
-	d_walk(parent->dentry, false, &data, path_check_mount);
+	d_walk(parent->dentry, false, &data, path_check_mount, NULL);
 	read_sequnlock_excl(&mount_lock);
 
 	return data.mounted;
@@ -1564,7 +1572,7 @@ void shrink_dcache_parent(struct dentry *parent)
 		struct select_data data = {.start = parent};
 
 		INIT_LIST_HEAD(&data.dispose);
-		d_walk(parent, false, &data, select_collect);
+		d_walk(parent, false, &data, select_collect, NULL);
 
 		if (!list_empty(&data.dispose)) {
 			shrink_dentry_list(&data.dispose);
@@ -1575,7 +1583,7 @@ void shrink_dcache_parent(struct dentry *parent)
 		if (!data.found)
 			break;
 		data.victim = NULL;
-		d_walk(parent, false, &data, select_collect2);
+		d_walk(parent, false, &data, select_collect2, NULL);
 		if (data.victim) {
 			struct dentry *parent;
 			spin_lock(&data.victim->d_lock);
@@ -1622,7 +1630,7 @@ static enum d_walk_ret umount_check(void *_data, struct dentry *dentry)
 static void do_one_tree(struct dentry *dentry)
 {
 	shrink_dcache_parent(dentry);
-	d_walk(dentry, false, dentry, umount_check);
+	d_walk(dentry, false, dentry, umount_check, NULL);
 	d_drop(dentry);
 	dput(dentry);
 }
@@ -1679,7 +1687,7 @@ void d_invalidate(struct dentry *dentry)
 	shrink_dcache_parent(dentry);
 	for (;;) {
 		struct dentry *victim = NULL;
-		d_walk(dentry, false, &victim, find_submount);
+		d_walk(dentry, false, &victim, find_submount, NULL);
 		if (!victim) {
 			if (had_submounts)
 				shrink_dcache_parent(dentry);
@@ -3129,7 +3137,7 @@ static enum d_walk_ret d_genocide_kill(void *data, struct dentry *dentry)
 
 void d_genocide(struct dentry *parent)
 {
-	d_walk(parent, false, parent, d_genocide_kill);
+	d_walk(parent, false, parent, d_genocide_kill, NULL);
 }
 
 EXPORT_SYMBOL(d_genocide);
-- 
2.21.0

[PATCH v2 3/4] dcache: introduce d_genocide_safe()

[Ondrej Mosnacek]

This patch adds a slightly modified variant of d_genocide() that works
safely on live (ramfs-like) trees. This function is needed for a safe
implementation of sel_remove_entries() in selinuxfs.

This new function differs from the original d_genocide in the following:
1. It locks the parent inode when traversing the dentries.
2. It first unhashes the dentry using __d_drop() before dropping the
   refcount and marking the dentry.
3. It does its business in the leave callback so that each dentry is
   unhashed after its children -- otherwise some dentries might never
   get traversed when d_walk() is restarted internally.

The combination of (1.) and (2.) is needed to avoid racing with
dcache_readdir(), which relies on the assumption that any
simple_positive() child dentry will not turn negative without locking
the parent inode for writing.

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 fs/dcache.c            | 32 ++++++++++++++++++++++++++++++++
 include/linux/dcache.h |  1 +
 2 files changed, 33 insertions(+)

diff --git a/fs/dcache.c b/fs/dcache.c
index 70afcb6e6892..f6d667120c1e 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -3142,6 +3142,38 @@ void d_genocide(struct dentry *parent)
 
 EXPORT_SYMBOL(d_genocide);
 
+static enum d_walk_ret d_genocide_safe_enter(void *data, struct dentry *dentry)
+{
+	struct dentry *root = data;
+
+	if (dentry != root && !simple_positive(dentry))
+		return D_WALK_SKIP;
+
+	return D_WALK_CONTINUE;
+}
+
+static void d_genocide_safe_leave(void *data, struct dentry *dentry)
+{
+	struct dentry *root = data;
+
+	if (dentry != root) {
+		__d_drop(dentry);
+
+		if (!(dentry->d_flags & DCACHE_GENOCIDE)) {
+			dentry->d_flags |= DCACHE_GENOCIDE;
+			dentry->d_lockref.count--;
+		}
+	}
+}
+
+void d_genocide_safe(struct dentry *parent)
+{
+	d_walk(parent, true, parent, d_genocide_safe_enter,
+	       d_genocide_safe_leave);
+}
+
+EXPORT_SYMBOL(d_genocide_safe);
+
 void d_tmpfile(struct dentry *dentry, struct inode *inode)
 {
 	inode_dec_link_count(inode);
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 9451011ac014..6d787c26e901 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -253,6 +253,7 @@ extern struct dentry * d_make_root(struct inode *);
 
 /* <clickety>-<click> the ramfs-type tree */
 extern void d_genocide(struct dentry *);
+extern void d_genocide_safe(struct dentry *parent);
 
 extern void d_tmpfile(struct dentry *, struct inode *);
 
-- 
2.21.0

[PATCH v2 4/4] selinux: use d_genocide_safe() in selinuxfs

[Ondrej Mosnacek]

Letting the following set of commands run long enough on a machine with
at least 3 CPU threads causes soft lockups in the kernel:

    (cd /sys/fs/selinux/; while true; do find >/dev/null 2>&1; done) &
    (cd /sys/fs/selinux/; while true; do find >/dev/null 2>&1; done) &
    (cd /sys/fs/selinux/; while true; do find >/dev/null 2>&1; done) &

    while true; do load_policy; echo -n .; sleep 0.1; done

The problem is that sel_remove_entries() removes the old selinuxfs
entries using d_genocide() + shrink_dcache_parent(), which is not safe
to do on live trees that are still exposed to userspace.

Specifically, it races with dcache_readdir(), which expects that while a
dentry's inode is locked, its (positive) children cannot get unlisted,
because both unlink() and rmdir() lock the parent inode first.

Therefore, use the newly introduced d_genocide_safe() instead of
d_genocide(), which fixes this issue.

Bug tracker links:
 * SELinux GitHub: https://github.com/SELinuxProject/selinux-kernel/issues/42
 * Red Hat Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1510603

Fixes: ad52184b705c ("selinuxfs: don't open-code d_genocide()")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/selinuxfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index e6c7643c3fc0..58d1949e5faf 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -1317,7 +1317,7 @@ static const struct file_operations sel_commit_bools_ops = {
 
 static void sel_remove_entries(struct dentry *de)
 {
-	d_genocide(de);
+	d_genocide_safe(de);
 	shrink_dcache_parent(de);
 }
 
-- 
2.21.0

Re: [PATCH v2 0/4] selinux: fix race when removing selinuxfs entries

[Al Viro]

On Thu, Aug 01, 2019 at 04:02:39PM +0200, Ondrej Mosnacek wrote:
> After hours and hours of getting familiar with dcache and debugging,
> I think I finally found a solution that works and hopefully stands a
> chance of being committed.
> 
> The series still doesn't address the lack of atomicity of the policy
> reload transition, but this is part of a wider problem and can be
> resolved later. Let's fix at least the userspace-triggered lockup
> first.

I don't think this is the right approach.  Consider the related problem:
what happens if somebody has mounted something upon a selinuxfs file?
That is the hard part here, and AFAICS your variant doesn't help it
at all...

Re: [PATCH v2 0/4] selinux: fix race when removing selinuxfs entries

[Ondrej Mosnacek]

On Thu, Aug 1, 2019 at 6:09 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, Aug 01, 2019 at 04:02:39PM +0200, Ondrej Mosnacek wrote:
> > After hours and hours of getting familiar with dcache and debugging,
> > I think I finally found a solution that works and hopefully stands a
> > chance of being committed.
> >
> > The series still doesn't address the lack of atomicity of the policy
> > reload transition, but this is part of a wider problem and can be
> > resolved later. Let's fix at least the userspace-triggered lockup
> > first.
>
> I don't think this is the right approach.  Consider the related problem:
> what happens if somebody has mounted something upon a selinuxfs file?
> That is the hard part here, and AFAICS your variant doesn't help it
> at all...

But that's another independent problem and it's not even fixed in
debugfs, which for now I'm treating as the baseline as I don't know of
any other filesystem that needs to remove its own directory trees in a
similar way.

I get that you don't want me to add a new function to the dcache API
that isn't bulletproof (and what I wrote here is apparently still far
from it), but you also previously said that I shouldn't open-code this
stuff in selinuxfs.c... I don't think I have the wits to write a
common function that handles all the possible issues, but I still want
to fix at least this one scenario (dcache_readdir() vs.
sel_remove_entries()).

Is there some way I could do this without getting a NACK from you? For
example, I thought of taking what is now debugfs_remove[_recursive]()
out of debugfs into, say, fs/libfs.c (providing some optional callback
to allow debugfs to do its __debugfs_file_removed() business) and use
this function(s) from both debugfs and selinuxfs. This way we could
later fix the leftover mount issue in one place and both filesystems
would (hopefully) immediately benefit from it. Would that be a
feasible way forward?

Thanks,
-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.

Re: [PATCH v2 0/4] selinux: fix race when removing selinuxfs entries

[Ondrej Mosnacek]

On Thu, Aug 8, 2019 at 9:59 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Thu, Aug 1, 2019 at 6:09 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Thu, Aug 01, 2019 at 04:02:39PM +0200, Ondrej Mosnacek wrote:
> > > After hours and hours of getting familiar with dcache and debugging,
> > > I think I finally found a solution that works and hopefully stands a
> > > chance of being committed.
> > >
> > > The series still doesn't address the lack of atomicity of the policy
> > > reload transition, but this is part of a wider problem and can be
> > > resolved later. Let's fix at least the userspace-triggered lockup
> > > first.
> >
> > I don't think this is the right approach.  Consider the related problem:
> > what happens if somebody has mounted something upon a selinuxfs file?
> > That is the hard part here, and AFAICS your variant doesn't help it
> > at all...
>
> But that's another independent problem and it's not even fixed in
> debugfs, which for now I'm treating as the baseline as I don't know of
> any other filesystem that needs to remove its own directory trees in a
> similar way.
>
> I get that you don't want me to add a new function to the dcache API
> that isn't bulletproof (and what I wrote here is apparently still far
> from it), but you also previously said that I shouldn't open-code this
> stuff in selinuxfs.c... I don't think I have the wits to write a
> common function that handles all the possible issues, but I still want
> to fix at least this one scenario (dcache_readdir() vs.
> sel_remove_entries()).
>
> Is there some way I could do this without getting a NACK from you? For
> example, I thought of taking what is now debugfs_remove[_recursive]()
> out of debugfs into, say, fs/libfs.c (providing some optional callback
> to allow debugfs to do its __debugfs_file_removed() business) and use
> this function(s) from both debugfs and selinuxfs. This way we could
> later fix the leftover mount issue in one place and both filesystems
> would (hopefully) immediately benefit from it. Would that be a
> feasible way forward?

Ping?

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.