| Message ID | 20241220174731.514432-29-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | module: Use RCU instead of RCU-sched. | expand |
On 12/20/24 18:41, Sebastian Andrzej Siewior wrote: > __module_address() can be invoked within a RCU section, there is no > requirement to have preemption disabled. > > I'm not sure if using rcu_read_lock() will introduce the regression that > has been fixed in commit 14c4c8e41511a ("cfi: Use > rcu_read_{un}lock_sched_notrace"). > > Cc: Elliot Berman <quic_eberman@quicinc.com> > Cc: Kees Cook <kees@kernel.org> > Cc: Nathan Chancellor <nathan@kernel.org> > Cc: Sami Tolvanen <samitolvanen@google.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: llvm@lists.linux.dev > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > kernel/cfi.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/kernel/cfi.c b/kernel/cfi.c > index 08caad7767176..c8f2b5a51b2e6 100644 > --- a/kernel/cfi.c > +++ b/kernel/cfi.c > @@ -71,6 +71,10 @@ static bool is_module_cfi_trap(unsigned long addr) > struct module *mod; > bool found = false; > > + /* > + * XXX this could be RCU protected but would it introcude the regression > + * fixed in 14c4c8e41511a ("cfi: Use rcu_read_{un}lock_sched_notrace") > + */ > rcu_read_lock_sched_notrace(); > > mod = __module_address(addr); I think that since 89245600941e ("cfi: Switch to -fsanitize=kcfi"), this can be a call to rcu_read_lock_sched(), or in your case rcu_read_lock(). The recursive case where __cfi_slowpath_diag() could end up calling itself is no longer present, as all that logic is gone. I then don't see another reason this should use the notrace variant. @Sami, could you please confirm this?
On Fri, Dec 20, 2024 at 06:41:42PM +0100, Sebastian Andrzej Siewior wrote: > __module_address() can be invoked within a RCU section, there is no > requirement to have preemption disabled. > > I'm not sure if using rcu_read_lock() will introduce the regression that > has been fixed in commit 14c4c8e41511a ("cfi: Use > rcu_read_{un}lock_sched_notrace"). > You can replace the rcu_read_lock_sched_notrace() with guard(rcu)(). Regular rcu lock doesn't generate function traces, so the recursive loop isn't possible. I've tested: - the current kernel (no recursive loop) - Revert back to rcu_read_lock_sched() (fails) - Your series as-is (no recurisve loop) - Replace with guard(rcu)() (no recursive loop) Whether you'd like to stick with the current patch or replace with guard(rcu)(): Tested-by: Elliot Berman <elliot.berman@oss.qualcomm.com> # sm8650-qrd - I don't know why I didn't mention steps to reproduce, even for my own benefit. Lesson learned :) Here are the steps to reproduce; you'll need a system with support for CFI: qemu arm64 probably does the trick and you'll need clang>=16. I'm happy to help test future revisions of this series since I have the setup all done. ``` modprobe -a dummy_stm stm_ftrace stm_p_basic mkdir -p /sys/kernel/config/stp-policy/dummy_stm.0.my-policy/default echo function > /sys/kernel/tracing/current_tracer echo 1 > /sys/kernel/tracing/tracing_on echo dummy_stm.0 > /sys/class/stm_source/ftrace/stm_source_link ``` The trace buffer should not be full of stm calls due to the recursive loop as mentioned in my original commit. Regards, Elliot Berman > Cc: Elliot Berman <quic_eberman@quicinc.com> > Cc: Kees Cook <kees@kernel.org> > Cc: Nathan Chancellor <nathan@kernel.org> > Cc: Sami Tolvanen <samitolvanen@google.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: llvm@lists.linux.dev > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > kernel/cfi.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/kernel/cfi.c b/kernel/cfi.c > index 08caad7767176..c8f2b5a51b2e6 100644 > --- a/kernel/cfi.c > +++ b/kernel/cfi.c > @@ -71,6 +71,10 @@ static bool is_module_cfi_trap(unsigned long addr) > struct module *mod; > bool found = false; > > + /* > + * XXX this could be RCU protected but would it introcude the regression > + * fixed in 14c4c8e41511a ("cfi: Use rcu_read_{un}lock_sched_notrace") > + */ > rcu_read_lock_sched_notrace(); > > mod = __module_address(addr); > -- > 2.45.2 >
Hi Petr, On Mon, Dec 30, 2024 at 1:13 PM Petr Pavlu <petr.pavlu@suse.com> wrote: > > On 12/20/24 18:41, Sebastian Andrzej Siewior wrote: > > __module_address() can be invoked within a RCU section, there is no > > requirement to have preemption disabled. > > > > I'm not sure if using rcu_read_lock() will introduce the regression that > > has been fixed in commit 14c4c8e41511a ("cfi: Use > > rcu_read_{un}lock_sched_notrace"). > > > > Cc: Elliot Berman <quic_eberman@quicinc.com> > > Cc: Kees Cook <kees@kernel.org> > > Cc: Nathan Chancellor <nathan@kernel.org> > > Cc: Sami Tolvanen <samitolvanen@google.com> > > Cc: Steven Rostedt <rostedt@goodmis.org> > > Cc: llvm@lists.linux.dev > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > > --- > > kernel/cfi.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/kernel/cfi.c b/kernel/cfi.c > > index 08caad7767176..c8f2b5a51b2e6 100644 > > --- a/kernel/cfi.c > > +++ b/kernel/cfi.c > > @@ -71,6 +71,10 @@ static bool is_module_cfi_trap(unsigned long addr) > > struct module *mod; > > bool found = false; > > > > + /* > > + * XXX this could be RCU protected but would it introcude the regression > > + * fixed in 14c4c8e41511a ("cfi: Use rcu_read_{un}lock_sched_notrace") > > + */ > > rcu_read_lock_sched_notrace(); > > > > mod = __module_address(addr); > > I think that since 89245600941e ("cfi: Switch to -fsanitize=kcfi"), this > can be a call to rcu_read_lock_sched(), or in your case rcu_read_lock(). > The recursive case where __cfi_slowpath_diag() could end up calling > itself is no longer present, as all that logic is gone. I then don't see > another reason this should use the notrace variant. > > @Sami, could you please confirm this? Switching is_module_cfi_trap() to use rcu_read_lock() in this series should be fine. KCFI checks don't perform function calls, so there's no risk of recursion, and this function is only called during the error handling path. Sami
Hi Elliot, On Mon, Dec 30, 2024 at 7:33 PM Elliot Berman <elliot.berman@oss.qualcomm.com> wrote: > > On Fri, Dec 20, 2024 at 06:41:42PM +0100, Sebastian Andrzej Siewior wrote: > > __module_address() can be invoked within a RCU section, there is no > > requirement to have preemption disabled. > > > > I'm not sure if using rcu_read_lock() will introduce the regression that > > has been fixed in commit 14c4c8e41511a ("cfi: Use > > rcu_read_{un}lock_sched_notrace"). > > > > You can replace the rcu_read_lock_sched_notrace() with guard(rcu)(). > Regular rcu lock doesn't generate function traces, so the recursive loop > isn't possible. > > I've tested: > - the current kernel (no recursive loop) > - Revert back to rcu_read_lock_sched() (fails) Which kernel version did you test? I assume something pre-KCFI as arm64 doesn't use this code since v6.1. > - Your series as-is (no recurisve loop) Note that this patch only adds a comment to is_module_cfi_trap(), so I wouldn't expect a functional change. Sami
On Thu, Jan 02, 2025 at 04:24:22PM -0800, Sami Tolvanen wrote: > Hi Elliot, > > On Mon, Dec 30, 2024 at 7:33 PM Elliot Berman > <elliot.berman@oss.qualcomm.com> wrote: > > > > On Fri, Dec 20, 2024 at 06:41:42PM +0100, Sebastian Andrzej Siewior wrote: > > > __module_address() can be invoked within a RCU section, there is no > > > requirement to have preemption disabled. > > > > > > I'm not sure if using rcu_read_lock() will introduce the regression that > > > has been fixed in commit 14c4c8e41511a ("cfi: Use > > > rcu_read_{un}lock_sched_notrace"). > > > > > > > You can replace the rcu_read_lock_sched_notrace() with guard(rcu)(). > > Regular rcu lock doesn't generate function traces, so the recursive loop > > isn't possible. > > > > I've tested: > > - the current kernel (no recursive loop) > > - Revert back to rcu_read_lock_sched() (fails) > > Which kernel version did you test? I assume something pre-KCFI as > arm64 doesn't use this code since v6.1. > Ah, thanks for calling me out. I dug a bit more, I thought I was looking at a recursive loop in the ftrace buffers, but was actually the expected behavior. When I tested on the other configurations, the stm dummy driver hadn't kicked in yet by the time I looked at the ftrace. Indeed, this function code is not used on arm64. I experimented with an x86 build as well and I was able to get the hang I remember seeing after some tweaks to force a CFI failure. Still, guard(rcu)() is okay by me :) > > - Your series as-is (no recurisve loop) > > Note that this patch only adds a comment to is_module_cfi_trap(), so I > wouldn't expect a functional change. > Agreed I wouldn't expect it to make any issues; I mentioned it for completeness sake. Regards, Elliot
Hi, On Mon, Jan 6, 2025 at 10:00 AM Elliot Berman <elliot.berman@oss.qualcomm.com> wrote: > > On Thu, Jan 02, 2025 at 04:24:22PM -0800, Sami Tolvanen wrote: > > Hi Elliot, > > > > On Mon, Dec 30, 2024 at 7:33 PM Elliot Berman > > <elliot.berman@oss.qualcomm.com> wrote: > > > > > > On Fri, Dec 20, 2024 at 06:41:42PM +0100, Sebastian Andrzej Siewior wrote: > > > > __module_address() can be invoked within a RCU section, there is no > > > > requirement to have preemption disabled. > > > > > > > > I'm not sure if using rcu_read_lock() will introduce the regression that > > > > has been fixed in commit 14c4c8e41511a ("cfi: Use > > > > rcu_read_{un}lock_sched_notrace"). > > > > > > > > > > You can replace the rcu_read_lock_sched_notrace() with guard(rcu)(). > > > Regular rcu lock doesn't generate function traces, so the recursive loop > > > isn't possible. > > > > > > I've tested: > > > - the current kernel (no recursive loop) > > > - Revert back to rcu_read_lock_sched() (fails) > > > > Which kernel version did you test? I assume something pre-KCFI as > > arm64 doesn't use this code since v6.1. > > > > Ah, thanks for calling me out. I dug a bit more, I thought I was looking > at a recursive loop in the ftrace buffers, but was actually the expected > behavior. When I tested on the other configurations, the stm dummy > driver hadn't kicked in yet by the time I looked at the ftrace. Indeed, > this function code is not used on arm64. > > I experimented with an x86 build as well and I was able to get the hang > I remember seeing after some tweaks to force a CFI failure. Still, > guard(rcu)() is okay by me :) OK, great. That makes sense. Thanks for taking the time to test this! Sami
On 2025-01-06 13:24:28 [-0800], Sami Tolvanen wrote: > Hi, Hi, > OK, great. That makes sense. Thanks for taking the time to test this! Thank you two for testing, confirming and adding more informations to what and why. I take some of this for the patch description. > Sami Sebastian
diff --git a/kernel/cfi.c b/kernel/cfi.c index 08caad7767176..c8f2b5a51b2e6 100644 --- a/kernel/cfi.c +++ b/kernel/cfi.c @@ -71,6 +71,10 @@ static bool is_module_cfi_trap(unsigned long addr) struct module *mod; bool found = false; + /* + * XXX this could be RCU protected but would it introcude the regression + * fixed in 14c4c8e41511a ("cfi: Use rcu_read_{un}lock_sched_notrace") + */ rcu_read_lock_sched_notrace(); mod = __module_address(addr);
__module_address() can be invoked within a RCU section, there is no requirement to have preemption disabled. I'm not sure if using rcu_read_lock() will introduce the regression that has been fixed in commit 14c4c8e41511a ("cfi: Use rcu_read_{un}lock_sched_notrace"). Cc: Elliot Berman <quic_eberman@quicinc.com> Cc: Kees Cook <kees@kernel.org> Cc: Nathan Chancellor <nathan@kernel.org> Cc: Sami Tolvanen <samitolvanen@google.com> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: llvm@lists.linux.dev Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- kernel/cfi.c | 4 ++++ 1 file changed, 4 insertions(+)