Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220531175910.890307-1-samitolvanen@google.com> <202205311231.C26F6E4E@keescook>
In-Reply-To: <202205311231.C26F6E4E@keescook>
From:   Sami Tolvanen <samitolvanen@google.com>
Date:   Tue, 31 May 2022 13:55:13 -0700
Message-ID: <CABCJKufAo8QoKcxa3oEgjpaLWx3+KmdMCJK0U02=H9L9snAHUQ@mail.gmail.com>
Subject: Re: [PATCH] cfi: Fix __cfi_slowpath_diag RCU usage with cpuidle
To:     Kees Cook <keescook@chromium.org>
Cc:     "Paul E. McKenney" <paulmck@kernel.org>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        llvm@lists.linux.dev, LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, May 31, 2022 at 12:33 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, May 31, 2022 at 10:59:10AM -0700, Sami Tolvanen wrote:
> > RCU_NONIDLE usage during __cfi_slowpath_diag can result in an invalid
> > RCU state in the cpuidle code path:
> >
> >   WARNING: CPU: 1 PID: 0 at kernel/rcu/tree.c:613 rcu_eqs_enter+0xe4/0x138
> >   ...
> >   Call trace:
> >     rcu_eqs_enter+0xe4/0x138
> >     rcu_idle_enter+0xa8/0x100
> >     cpuidle_enter_state+0x154/0x3a8
> >     cpuidle_enter+0x3c/0x58
> >     do_idle.llvm.6590768638138871020+0x1f4/0x2ec
> >     cpu_startup_entry+0x28/0x2c
> >     secondary_start_kernel+0x1b8/0x220
> >     __secondary_switched+0x94/0x98
> >
> > Instead, call rcu_irq_enter/exit to wake up RCU only when needed and
> > disable interrupts for the entire CFI shadow/module check when we do.
> >
> > Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> > ---
> >  kernel/cfi.c | 22 ++++++++++++++++------
> >  1 file changed, 16 insertions(+), 6 deletions(-)
> >
> > diff --git a/kernel/cfi.c b/kernel/cfi.c
> > index 9594cfd1cf2c..08102d19ec15 100644
> > --- a/kernel/cfi.c
> > +++ b/kernel/cfi.c
> > @@ -281,6 +281,8 @@ static inline cfi_check_fn find_module_check_fn(unsigned long ptr)
> >  static inline cfi_check_fn find_check_fn(unsigned long ptr)
> >  {
> >       cfi_check_fn fn = NULL;
> > +     unsigned long flags;
> > +     bool rcu_idle;
> >
> >       if (is_kernel_text(ptr))
> >               return __cfi_check;
> > @@ -290,13 +292,21 @@ static inline cfi_check_fn find_check_fn(unsigned long ptr)
> >        * the shadow and __module_address use RCU, so we need to wake it
> >        * up if necessary.
> >        */
> > -     RCU_NONIDLE({
> > -             if (IS_ENABLED(CONFIG_CFI_CLANG_SHADOW))
> > -                     fn = find_shadow_check_fn(ptr);
> > +     rcu_idle = !rcu_is_watching();
> > +     if (rcu_idle) {
> > +             local_irq_save(flags);
> > +             rcu_irq_enter();
> > +     }
> > +
> > +     if (IS_ENABLED(CONFIG_CFI_CLANG_SHADOW))
> > +             fn = find_shadow_check_fn(ptr);
> > +     if (!fn)
> > +             fn = find_module_check_fn(ptr);
> >
> > -             if (!fn)
> > -                     fn = find_module_check_fn(ptr);
> > -     });
> > +     if (rcu_idle) {
> > +             rcu_irq_exit();
> > +             local_irq_restore(flags);
> > +     }
> >
> >       return fn;
> >  }
> > --
> > 2.36.1.255.ge46751e96f-goog
> >
>
> Thanks for the fix! It looks like these tags could be added, yes?
>
> Fixes: cf68fffb66d6 ("add support for Clang CFI")
> Cc: stable@vger.kernel.org

Yes, agreed.

> Also, to improve the commit log, under what situation was this problem
> encountered?

On affected devices, this warning is triggered at boot.

Sami