Message-ID: <20210225164553.GG6000@sequoia>
Date:   Thu, 25 Feb 2021 10:45:53 -0600
From:   Tyler Hicks <tyhicks@...ux.microsoft.com>
To:     Ondrej Mosnacek <omosnace@...hat.com>
Cc:     Paul Moore <paul@...l-moore.com>,
        Stephen Smalley <stephen.smalley.work@...il.com>,
        SElinux list <selinux@...r.kernel.org>,
        Linux kernel mailing list <linux-kernel@...r.kernel.org>
Subject: Re: [BUG] Race between policy reload sidtab conversion and live
 conversion

On 2021-02-25 17:38:25, Ondrej Mosnacek wrote:
> On Wed, Feb 24, 2021 at 3:43 PM Tyler Hicks <tyhicks@...ux.microsoft.com> wrote:
> > On 2021-02-24 10:33:46, Ondrej Mosnacek wrote:
> > > On Tue, Feb 23, 2021 at 11:37 PM Tyler Hicks
> > > <tyhicks@...ux.microsoft.com> wrote:
> > > > On 2021-02-23 15:50:56, Tyler Hicks wrote:
> > > > > On 2021-02-23 15:43:48, Tyler Hicks wrote:
> > > > > > I'm seeing a race during policy load while the "regular" sidtab
> > > > > > conversion is happening and a live conversion starts to take place in
> > > > > > sidtab_context_to_sid().
> > > > > >
> > > > > > We have an initial policy that's loaded by systemd ~0.6s into boot and
> > > > > > then another policy gets loaded ~2-3s into boot. That second policy load
> > > > > > is what hits the race condition situation because the sidtab is only
> > > > > > partially populated and there's a decent amount of filesystem operations
> > > > > > happening, at the same time, which are triggering live conversions.
> > > >
> > > > Hmm, perhaps this is the same problem that's fixed by Ondrej's proposed
> > > > change here:
> > > >
> > > >  https://lore.kernel.org/selinux/20210212185930.130477-3-omosnace@redhat.com/
> > > >
> > > > I'll put these changes through a validation run (the only place that I
> > > > can seem to reproduce this crash) and see how it looks.
> > >
> > > Hm... I think there is actually another race condition introduced by
> > > the switch from rwlock to RCU [1]... Judging from the call trace you
> > > may be hitting that.
> >
> > I believe your patches above fixed the race I was seeing. I was able to
> > make it through a full validation run without any crashes. Without those
> > patches applied, I would see several crashes resulting from this race
> > over the course of a validation run.
> 
> Hm... okay so probably you were indeed running into that bug. I tried
> to reproduce the other race (I added a BUG_ON to help detect it), but
> wasn't able to reproduce it with my (pretty aggressive) stress test. I
> only managed to trigger it by adding a conditional delay in the right
> place. So I now know the second bug is really there, though it' seems
> to be very unlikely to be hit in practice (might be more likely on
> systems with many CPU cores, though). The first bug, OTOH, is
> triggered almost instantly by my stress test.
> 
> Unless someone objects, I'll start working on a patch to switch back
> to read-write lock for now. If all goes well, I'll send it sometime
> next week.
> 
> >
> > I'll continue to test with your changes and let you know if I end up
> > running into the other race you spotted.
> 
> Thanks, but given the results of my testing it's probably not worth trying :)

Those changes have now survived through several validation runs. I can
confidently say that they fix the race I was seeing.

Tyler

> 
> >
> > Tyler
> >
> > >
> > > Basically, before the switch the sidtab swapover worked like this:
> > > 1. Start live conversion of new entries.
> > > 2. Convert existing entries.
> > > [Still only the old sidtab is visible to readers here.]
> > > 3. Swap sidtab under write lock.
> > > 4. Now only the new sidtab is visible to readers, so the old one can
> > > be destroyed.
> > >
> > > After the switch to RCU, we now have:
> > > 1. Start live conversion of new entries.
> > > 2. Convert existing entries.
> > > 3. RCU-assign the new policy pointer to selinux_state.
> > > [!!! Now actually both old and new sidtab may be referenced by
> > > readers, since there is no synchronization barrier previously provided
> > > by the write lock.]
> > > 4. Wait for synchronize_rcu() to return.
> > > 5. Now only the new sidtab is visible to readers, so the old one can
> > > be destroyed.
> > >
> > > So the race can happen between 3. and 5., if one thread already sees
> > > the new sidtab and adds a new entry there, and a second thread still
> > > has the reference to the old sidtab and also tires to add a new entry;
> > > live-converting to the new sidtab, which it doesn't expect to change
> > > by itself. Unfortunately I failed to realize this when reviewing the
> > > patch :/
> > >
> > > I think the only two options to fix it are A) switching back to
> > > read-write lock (the easy and safe way; undoing the performance
> > > benefits of [1]), or B) implementing a safe two-way live conversion of
> > > new sidtab entries, so that both tables are kept in sync while they
> > > are both available (more complicated and with possible tricky
> > > implications of different interpretations of contexts by the two
> > > policies).
> > >
> > > [1] 1b8b31a2e612 ("selinux: convert policy read-write lock to RCU")
> > >
> > > --
> > > Ondrej Mosnacek
> > > Software Engineer, Linux Security - SELinux kernel
> > > Red Hat, Inc.
> > >
> >
> 
> 
> --
> Ondrej Mosnacek
> Software Engineer, Linux Security - SELinux kernel
> Red Hat, Inc.
> 

Powered by blists - more mailing lists