Re: [PATCH v2] selinux: various sparse fixes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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(&current->sighand->siglock);
> > > > > +               spin_lock_irq(unrcu_pointer(&current->sighand->siglock));
> > > > >                 if (!fatal_signal_pending(current)) {
> > > > >                         flush_sigqueue(&current->pending);
> > > > >                         flush_sigqueue(&current->signal->shared_pending);
> > > > > @@ -2542,13 +2542,13 @@ static void selinux_bprm_committed_creds(struct linux_binprm *bprm)
> > > > >                         sigemptyset(&current->blocked);
> > > > >                         recalc_sigpending();
> > > > >                 }
> > > > > -               spin_unlock_irq(&current->sighand->siglock);
> > > > > +               spin_unlock_irq(unrcu_pointer(&current->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



[Index of Archives]     [Selinux Refpolicy]     [Linux SGX]     [Fedora Users]     [Fedora Desktop]     [Yosemite Photos]     [Yosemite Camping]     [Yosemite Campsites]     [KDE Users]     [Gnome Users]

  Powered by Linux