On Fri, Jan 28, 2022 at 9:35 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
>
> On Thu, Jan 27, 2022 at 9:04 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > On Thu, Jan 27, 2022 at 2:03 PM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> > >
> > > On Thu, Jan 27, 2022 at 7:22 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > > > When running the SELinux code through sparse, there are a handful of
> > > > warnings. This patch resolves some of these warnings caused by
> > > > "__rcu" mismatches.
> > > >
> > > > % make W=1 C=1 security/selinux/
> > > >
> > > > Signed-off-by: Paul Moore <paul@xxxxxxxxxxxxxx>
> > > > ---
> > > > security/selinux/hooks.c | 6 +++---
> > > > security/selinux/ibpkey.c | 2 +-
> > > > security/selinux/netnode.c | 5 +++--
> > > > security/selinux/netport.c | 2 +-
> > > > 4 files changed, 8 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > > index 221e642025f5..0e857f86f5a7 100644
> > > > --- a/security/selinux/hooks.c
> > > > +++ b/security/selinux/hooks.c
> > > > @@ -2534,7 +2534,7 @@ static void selinux_bprm_committed_creds(struct linux_binprm *bprm)
> > > > if (rc) {
> > > > clear_itimer();
> > > >
> > > > - spin_lock_irq(¤t->sighand->siglock);
> > > > + spin_lock_irq(unrcu_pointer(¤t->sighand->siglock));
> > > > if (!fatal_signal_pending(current)) {
> > > > flush_sigqueue(¤t->pending);
> > > > flush_sigqueue(¤t->signal->shared_pending);
> > > > @@ -2542,13 +2542,13 @@ static void selinux_bprm_committed_creds(struct linux_binprm *bprm)
> > > > sigemptyset(¤t->blocked);
> > > > recalc_sigpending();
> > > > }
> > > > - spin_unlock_irq(¤t->sighand->siglock);
> > > > + spin_unlock_irq(unrcu_pointer(¤t->sighand->siglock));
> > >
> > > Shouldn't this be:
> > >
> > > spin_[un]lock_irq(&unrcu_pointer(current->sighand)->siglock);
> >
> > Maybe.
> >
> > The __rcu space annotation is definitely on task_struct::sighand, but
> > my (quick) look at unrcu_pointer() was that the the de-rcu'ification
> > applies to all of the dereferencing that is passed as the macro
> > argument. Because of that I decided to pass the entire dereferencing
> > chain to the unrcu_pointer() macro just in case. If that way of
> > thinking is incorrect please let me know, otherwise I would rather
> > just leave it as it is in v2.
>
> While it does work this way, too, it just doesn't feel right. IMHO
> it's more self-documenting to mark the exact pointer for which we are
> applying the RCU access exception.
FWIW, the documentation aspect here is the "__rcu" marking on the
pointer in the task_struct, and possibly the sparse warnings. What we
are doing here is basically a hack to tell sparse to be quiet, and
since these core kernel variable annotations are likely to come and go
independent of the SELinux code I'm not overly worried about minor
self-documenting issues such as this (they are going to go out-of-date
anyway). I'm more concerned with keeping a clean {sparse} build.
--
paul-moore.com
