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. >
