SELinux Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 0/4] selinux: fix race when removing selinuxfs entries
@ 2019-08-01 14:02 Ondrej Mosnacek
  2019-08-01 14:02 ` [PATCH v2 1/4] d_walk: optionally lock also parent inode Ondrej Mosnacek
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Ondrej Mosnacek @ 2019-08-01 14:02 UTC (permalink / raw)
  To: selinux, Paul Moore; +Cc: Al Viro, linux-fsdevel

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/4] d_walk: optionally lock also parent inode
  2019-08-01 14:02 [PATCH v2 0/4] selinux: fix race when removing selinuxfs entries Ondrej Mosnacek
@ 2019-08-01 14:02 ` Ondrej Mosnacek
  2019-08-01 16:10   ` Al Viro
  2019-08-01 16:12   ` Al Viro
  2019-08-01 14:02 ` [PATCH v2 2/4] d_walk: add leave callback Ondrej Mosnacek
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 9+ messages in thread
From: Ondrej Mosnacek @ 2019-08-01 14:02 UTC (permalink / raw)
  To: selinux, Paul Moore; +Cc: Al Viro, linux-fsdevel

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 2/4] d_walk: add leave callback
  2019-08-01 14:02 [PATCH v2 0/4] selinux: fix race when removing selinuxfs entries Ondrej Mosnacek
  2019-08-01 14:02 ` [PATCH v2 1/4] d_walk: optionally lock also parent inode Ondrej Mosnacek
@ 2019-08-01 14:02 ` Ondrej Mosnacek
  2019-08-01 14:02 ` [PATCH v2 3/4] dcache: introduce d_genocide_safe() Ondrej Mosnacek
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Ondrej Mosnacek @ 2019-08-01 14:02 UTC (permalink / raw)
  To: selinux, Paul Moore; +Cc: Al Viro, linux-fsdevel

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 3/4] dcache: introduce d_genocide_safe()
  2019-08-01 14:02 [PATCH v2 0/4] selinux: fix race when removing selinuxfs entries Ondrej Mosnacek
  2019-08-01 14:02 ` [PATCH v2 1/4] d_walk: optionally lock also parent inode Ondrej Mosnacek
  2019-08-01 14:02 ` [PATCH v2 2/4] d_walk: add leave callback Ondrej Mosnacek
@ 2019-08-01 14:02 ` Ondrej Mosnacek
  2019-08-01 14:02 ` [PATCH v2 4/4] selinux: use d_genocide_safe() in selinuxfs Ondrej Mosnacek
  2019-08-01 16:09 ` [PATCH v2 0/4] selinux: fix race when removing selinuxfs entries Al Viro
  4 siblings, 0 replies; 9+ messages in thread
From: Ondrej Mosnacek @ 2019-08-01 14:02 UTC (permalink / raw)
  To: selinux, Paul Moore; +Cc: Al Viro, linux-fsdevel

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 4/4] selinux: use d_genocide_safe() in selinuxfs
  2019-08-01 14:02 [PATCH v2 0/4] selinux: fix race when removing selinuxfs entries Ondrej Mosnacek
                   ` (2 preceding siblings ...)
  2019-08-01 14:02 ` [PATCH v2 3/4] dcache: introduce d_genocide_safe() Ondrej Mosnacek
@ 2019-08-01 14:02 ` Ondrej Mosnacek
  2019-08-01 16:09 ` [PATCH v2 0/4] selinux: fix race when removing selinuxfs entries Al Viro
  4 siblings, 0 replies; 9+ messages in thread
From: Ondrej Mosnacek @ 2019-08-01 14:02 UTC (permalink / raw)
  To: selinux, Paul Moore; +Cc: Al Viro, linux-fsdevel

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 0/4] selinux: fix race when removing selinuxfs entries
  2019-08-01 14:02 [PATCH v2 0/4] selinux: fix race when removing selinuxfs entries Ondrej Mosnacek
                   ` (3 preceding siblings ...)
  2019-08-01 14:02 ` [PATCH v2 4/4] selinux: use d_genocide_safe() in selinuxfs Ondrej Mosnacek
@ 2019-08-01 16:09 ` Al Viro
  2019-08-08  7:59   ` Ondrej Mosnacek
  4 siblings, 1 reply; 9+ messages in thread
From: Al Viro @ 2019-08-01 16:09 UTC (permalink / raw)
  To: Ondrej Mosnacek; +Cc: selinux, Paul Moore, linux-fsdevel

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...

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/4] d_walk: optionally lock also parent inode
  2019-08-01 14:02 ` [PATCH v2 1/4] d_walk: optionally lock also parent inode Ondrej Mosnacek
@ 2019-08-01 16:10   ` Al Viro
  2019-08-01 16:12   ` Al Viro
  1 sibling, 0 replies; 9+ messages in thread
From: Al Viro @ 2019-08-01 16:10 UTC (permalink / raw)
  To: Ondrej Mosnacek; +Cc: selinux, Paul Moore, linux-fsdevel

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?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/4] d_walk: optionally lock also parent inode
  2019-08-01 14:02 ` [PATCH v2 1/4] d_walk: optionally lock also parent inode Ondrej Mosnacek
  2019-08-01 16:10   ` Al Viro
@ 2019-08-01 16:12   ` Al Viro
  1 sibling, 0 replies; 9+ messages in thread
From: Al Viro @ 2019-08-01 16:12 UTC (permalink / raw)
  To: Ondrej Mosnacek; +Cc: selinux, Paul Moore, linux-fsdevel

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?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 0/4] selinux: fix race when removing selinuxfs entries
  2019-08-01 16:09 ` [PATCH v2 0/4] selinux: fix race when removing selinuxfs entries Al Viro
@ 2019-08-08  7:59   ` Ondrej Mosnacek
  0 siblings, 0 replies; 9+ messages in thread
From: Ondrej Mosnacek @ 2019-08-08  7:59 UTC (permalink / raw)
  To: Al Viro; +Cc: SElinux list, Paul Moore, linux-fsdevel

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-01 14:02 [PATCH v2 0/4] selinux: fix race when removing selinuxfs entries Ondrej Mosnacek
2019-08-01 14:02 ` [PATCH v2 1/4] d_walk: optionally lock also parent inode Ondrej Mosnacek
2019-08-01 16:10   ` Al Viro
2019-08-01 16:12   ` Al Viro
2019-08-01 14:02 ` [PATCH v2 2/4] d_walk: add leave callback Ondrej Mosnacek
2019-08-01 14:02 ` [PATCH v2 3/4] dcache: introduce d_genocide_safe() Ondrej Mosnacek
2019-08-01 14:02 ` [PATCH v2 4/4] selinux: use d_genocide_safe() in selinuxfs Ondrej Mosnacek
2019-08-01 16:09 ` [PATCH v2 0/4] selinux: fix race when removing selinuxfs entries Al Viro
2019-08-08  7:59   ` Ondrej Mosnacek

SELinux Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/selinux/0 selinux/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 selinux selinux/ https://lore.kernel.org/selinux \
		selinux@vger.kernel.org selinux@archiver.kernel.org
	public-inbox-index selinux


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.selinux


AGPL code for this site: git clone https://public-inbox.org/ public-inbox