[PATCH 2/2] selinux: make __inode_security_revalidate non-sleeping

[PATCH 2/2] selinux: make __inode_security_revalidate non-sleeping

[Yugansh Mittal]

Replace the blocking revalidation logic in __inode_security_revalidate()
with a fast, RCU-safe check of the inode security struct.

Previously, the function could invoke inode_doinit_with_dentry() when
may_sleep was true, which might block. With this change we always avoid
sleeping and return -ECHILD if the inode label is invalid, forcing the
caller to retry in a sleepable context.

This ensures that __inode_security_revalidate() can safely run in
non-sleepable contexts while preserving correct retry semantics.

Signed-off-by: Yugansh Mittal <mittalyugansh1@gmail.com>
---
 security/selinux/hooks.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index c95a5874b..2bb94794e 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -282,19 +282,15 @@ static int __inode_security_revalidate(struct inode *inode,
 	if (!selinux_initialized())
 		return 0;
 
-	if (may_sleep)
-		might_sleep();
-	else
-		return -ECHILD;
-
-	/*
-	 * Check to ensure that an inode's SELinux state is valid and try
-	 * reloading the inode security label if necessary.  This will fail if
-	 * @dentry is NULL and no dentry for this inode can be found; in that
-	 * case, continue using the old label.
-	 */
-	inode_doinit_with_dentry(inode, dentry);
-	return 0;
+	rcu_read_lock();
+        isec = selinux_inode(inode);
+        if (unlikely(!isec || is_label_invalid(isec))) {
+                rcu_read_unlock();
+                return -ECHILD;  /* force caller to handle reload elsewhere */
+        }
+        rcu_read_unlock();
+
+	return 0; /* valid and no sleeping done */
 }
 
 static struct inode_security_struct *inode_security_novalidate(struct inode *inode)
-- 
2.43.0

Re: [PATCH 2/2] selinux: make __inode_security_revalidate non-sleeping

[Stephen Smalley]

On Sun, Aug 24, 2025 at 9:01 AM Yugansh Mittal <mittalyugansh1@gmail.com> wrote:
>
> Replace the blocking revalidation logic in __inode_security_revalidate()
> with a fast, RCU-safe check of the inode security struct.
>
> Previously, the function could invoke inode_doinit_with_dentry() when
> may_sleep was true, which might block. With this change we always avoid
> sleeping and return -ECHILD if the inode label is invalid, forcing the
> caller to retry in a sleepable context.

If you look at the callers of __inode_security_revalidate(), you will
see that not all are capable of propagating -ECHILD to their callers
and forcing a retry; IIRC this is only truly possible during rcu path
walk. Hence, this change will produce situations where the inode may
be left with a stale or unlabeled context.
Your patch was marked as 2/2 but I did not see a 1/2 patch.

>
> This ensures that __inode_security_revalidate() can safely run in
> non-sleepable contexts while preserving correct retry semantics.
>
> Signed-off-by: Yugansh Mittal <mittalyugansh1@gmail.com>
> ---
>  security/selinux/hooks.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index c95a5874b..2bb94794e 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -282,19 +282,15 @@ static int __inode_security_revalidate(struct inode *inode,
>         if (!selinux_initialized())
>                 return 0;
>
> -       if (may_sleep)
> -               might_sleep();
> -       else
> -               return -ECHILD;
> -
> -       /*
> -        * Check to ensure that an inode's SELinux state is valid and try
> -        * reloading the inode security label if necessary.  This will fail if
> -        * @dentry is NULL and no dentry for this inode can be found; in that
> -        * case, continue using the old label.
> -        */
> -       inode_doinit_with_dentry(inode, dentry);
> -       return 0;
> +       rcu_read_lock();
> +        isec = selinux_inode(inode);
> +        if (unlikely(!isec || is_label_invalid(isec))) {
> +                rcu_read_unlock();
> +                return -ECHILD;  /* force caller to handle reload elsewhere */
> +        }
> +        rcu_read_unlock();
> +
> +       return 0; /* valid and no sleeping done */
>  }
>
>  static struct inode_security_struct *inode_security_novalidate(struct inode *inode)
> --
> 2.43.0
>

[PATCH v2] [V2] selinux: restore sleepable revalidation; keep fast no-sleep check

[Yugansh Mittal]

The prior change made __inode_security_revalidate() always return
-ECHILD when the inode label appears invalid, avoiding the potential
sleep in inode_doinit_with_dentry(). However, not all callers can
propagate -ECHILD; only RCU path walk reliably can. This caused
cases where the inode could be left with a stale/unlabeled context.

Fix by:
  * Keeping an RCU-safe, non-blocking validity check fast path.
  * Returning -ECHILD only when may_sleep == false.
  * When may_sleep == true, performing the blocking revalidation via
    inode_doinit_with_dentry() as before.

This preserves non-sleeping behavior in atomic/RCU contexts while
maintaining correct reload semantics elsewhere.

Signed-off-by: Yugansh Mittal <mittalyugansh1@gmail.com>
---
 security/selinux/hooks.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index c95a5874b..170ae6d65 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -279,20 +279,34 @@ static int __inode_security_revalidate(struct inode *inode,
 				       struct dentry *dentry,
 				       bool may_sleep)
 {
+	struct inode_security_struct *isec;
+
 	if (!selinux_initialized())
 		return 0;
 
-	if (may_sleep)
-		might_sleep();
-	else
-		return -ECHILD;
+	/* Fast, non-blocking validity check first */
+	rcu_read_lock();
+	isec = selinux_inode(inode);
+	if (likely(isec && !is_label_invalid(isec))) {
+		rcu_read_unlock();
+		return 0;   /* valid and no sleeping done */
+	}
+	rcu_read_unlock();
 
 	/*
-	 * Check to ensure that an inode's SELinux state is valid and try
-	 * reloading the inode security label if necessary.  This will fail if
-	 * @dentry is NULL and no dentry for this inode can be found; in that
-	 * case, continue using the old label.
-	 */
+	* Label looks invalid. If we can't sleep, signal caller that a
+	* retry in a sleepable context is required. Only contexts like
+	* RCU path walk are expected to propagate -ECHILD.
+	*/
+	if (!may_sleep)
+	return -ECHILD;
+
+	/*
+	* Sleepable context: reload the label. This may block.
+	* If @dentry is NULL and no dentry can be found we'll continue
+	* using the old label, consistent with prior behavior.
+	*/
+	might_sleep();
 	inode_doinit_with_dentry(inode, dentry);
 	return 0;
 }
-- 
2.43.0

Re: [PATCH v2] [V2] selinux: restore sleepable revalidation; keep fast no-sleep check

[Stephen Smalley]

On Tue, Aug 26, 2025 at 1:24 PM Yugansh Mittal <mittalyugansh1@gmail.com> wrote:
>
> The prior change made __inode_security_revalidate() always return

The prior change wasn't accepted so we wouldn't normally reference it
in a patch description,
just in a changelog that would go after the diffstat and not be
included into the commit message.

> -ECHILD when the inode label appears invalid, avoiding the potential
> sleep in inode_doinit_with_dentry(). However, not all callers can
> propagate -ECHILD; only RCU path walk reliably can. This caused
> cases where the inode could be left with a stale/unlabeled context.
>
> Fix by:

No need to fix that which is not broken.

>   * Keeping an RCU-safe, non-blocking validity check fast path.
>   * Returning -ECHILD only when may_sleep == false.
>   * When may_sleep == true, performing the blocking revalidation via
>     inode_doinit_with_dentry() as before.
>
> This preserves non-sleeping behavior in atomic/RCU contexts while
> maintaining correct reload semantics elsewhere.
>
> Signed-off-by: Yugansh Mittal <mittalyugansh1@gmail.com>

It is unclear what problem you are trying to solve with the current
code, but your current patch doesn't compile alone (seems to have a
dependency on some other patch you haven't posted anywhere I can see).
Also doesn't pass muster with the ./scripts/checkpatch.pl script.
See https://github.com/SELinuxProject/selinux-kernel/wiki/Getting-Started
for tips on how to get started with SELinux development and to prepare
patches that will be acceptable.

> ---
>  security/selinux/hooks.c | 32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index c95a5874b..170ae6d65 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -279,20 +279,34 @@ static int __inode_security_revalidate(struct inode *inode,
>                                        struct dentry *dentry,
>                                        bool may_sleep)
>  {
> +       struct inode_security_struct *isec;
> +
>         if (!selinux_initialized())
>                 return 0;
>
> -       if (may_sleep)
> -               might_sleep();
> -       else
> -               return -ECHILD;
> +       /* Fast, non-blocking validity check first */
> +       rcu_read_lock();

Why do we need rcu_read_lock() here? We don't take it elsewhere that
we access isec.

> +       isec = selinux_inode(inode);
> +       if (likely(isec && !is_label_invalid(isec))) {

isec cannot be NULL for an inode when SELinux is enabled, so no need
to test for it.
is_label_invalid() is not defined in this patch and no other patch
from you appears to have been posted.

> +               rcu_read_unlock();
> +               return 0;   /* valid and no sleeping done */
> +       }
> +       rcu_read_unlock();
>
>         /*
> -        * Check to ensure that an inode's SELinux state is valid and try
> -        * reloading the inode security label if necessary.  This will fail if
> -        * @dentry is NULL and no dentry for this inode can be found; in that
> -        * case, continue using the old label.
> -        */
> +       * Label looks invalid. If we can't sleep, signal caller that a
> +       * retry in a sleepable context is required. Only contexts like
> +       * RCU path walk are expected to propagate -ECHILD.
> +       */
> +       if (!may_sleep)
> +       return -ECHILD;

Indentation problem, checkpatch.pl would have caught it.

> +
> +       /*
> +       * Sleepable context: reload the label. This may block.
> +       * If @dentry is NULL and no dentry can be found we'll continue
> +       * using the old label, consistent with prior behavior.
> +       */
> +       might_sleep();
>         inode_doinit_with_dentry(inode, dentry);
>         return 0;
>  }

What makes your patch better than the code that was in place before it?
Have you compared the resulting assembly and/or run any benchmarks?

> --
> 2.43.0
>