On Fri, Jan 28, 2022 at 1:13 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> 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.
Merged into selinux/next, and ultimately I changed my mind and decided
to change the current::sighand lines as Ondrej suggested.
--
paul-moore.com
