Re: [BUG] Race between policy reload sidtab conversion and live conversion
From: Tyler Hicks
Date: Thu Feb 25 2021 - 11:51:11 EST
On 2021年02月25日 17:38:25, Ondrej Mosnacek wrote:
>
On Wed, Feb 24, 2021 at 3:43 PM Tyler Hicks <tyhicks@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On 2021年02月24日 10:33:46, Ondrej Mosnacek wrote:
>
> > On Tue, Feb 23, 2021 at 11:37 PM Tyler Hicks
>
> > <tyhicks@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx/
>
> > >
>
> > > 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.
>