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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAEjxPJ4KA+8iijcN0CrgquuXhzJ1e1CaeEaSdaj6_5gfZWeOYg@mail.gmail.com>
Date: Tue, 26 Aug 2025 16:08:49 -0400
From: Stephen Smalley <stephen.smalley.work@...il.com>
To: Yugansh Mittal <mittalyugansh1@...il.com>
Cc: paul@...l-moore.com, selinux@...r.kernel.org, omosnace@...hat.com, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] [V2] selinux: restore sleepable revalidation; keep
 fast no-sleep check

On Tue, Aug 26, 2025 at 1:24 PM Yugansh Mittal <mittalyugansh1@...il.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@...il.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
>