Messages in this thread | | | Date | Mon, 13 Dec 2021 20:09:17 +0100 | From | Borislav Petkov <> | Subject | Re: [PATCH 1/3] X86/db: Change __this_cpu_read() to this_cpu_read() in hw_breakpoint_active() |
| |
On Mon, Dec 13, 2021 at 12:22:13PM +0800, Lai Jiangshan wrote: > From: Lai Jiangshan <laijs@linux.alibaba.com>
Lai, what you're touching is complex stuff. If your commit messages are hard to parse then that makes reviewing those patches not a fun experience. Not in the least.
So please try to restrain yourself and write proper English. Run your commit message through a spellchecker at least so that there are no funky words.
Also, read Documentation/process/submitting-patches.rst for hints on how to write it.
The structure and the explanation is in the right direction but please try to formulate them as understandable as possible.
> __this_cpu_read() can not be instrumented except its own debugging code > when CONFIG_DEBUG_PREEMPT. The debugging code will call > __this_cpu_preempt_check(). __this_cpu_preempt_check() itself is also > noinstr, so __this_cpu_read() can be used in noinstr. > > But these is one exception when exc_debug_kernel() calls local_db_save() > which calls hw_breakpoint_active() which calls __this_cpu_read(). If > the data accessed by __this_cpu_preempt_check() is also watched by > hw_breakpoints, it would cause recursive #DB.
Up until here is good.
> this_cpu_read() in X86 is also non instrumentable, and it doesn't access
"x86" not "X86" or any other way.
Also, read this: Documentation/process/maintainer-tip.rst
as it has more hints about commit message structure etc.
> to any extra data except the percpu cpu_dr7, and cpu_dr7 is disallowed > to be watched in arch_build_bp_info(). So this_cpu_read() is safe to > be used when hw_breakpoints is still active, and __this_cpu_read() here > should be changed to this_cpu_read(). > > This problem can only happen when the system owner uses a kernel with > CONFIG_DEBUG_PREEMPT enabled and deliberately use hw_breakpoints on > the data that __this_cpu_preempt_check() accesses. Sot it is just a > problem with no significance. > > One might suggest that, all the data accessed by noinstr functions > should be marked in denylist for hw_breakpoints. That would complexify
should be marked in denylist for hw_breakpoints. That would complexify Unknown word [denylist] in commit message, suggestions: ['deny list', 'deny-list', 'dentistry']
should be marked in denylist for hw_breakpoints. That would complexify Unknown word [complexify] in commit message, suggestions: ['complexity', 'complexion']
> the noinstrment framework and add hurdles to anyone that who want to
the noinstrment framework and add hurdles to anyone that who want to Unknown word [noinstrment] in commit message, suggestions: ['instrument']
So you need to restrain yourself and stop inventing new English words.
> add a new noinstr function. All we need is to suppress #DB in the IST
Who is "we"?
> interrupt entry path until safe place where #DB is disabled in hardware > or #DB handler can handle well even it hits data accessed by noinstr > function. Changing __this_cpu_read() to this_cpu_read() is fit for it.
You don't need to write *what* your patch is doing - that is clear from the diff.
I'll let Peter comment on what should actually be used.
Thx.
-- Regards/Gruss, Boris.
https://people.kernel.org/tglx/notes-about-netiquette
| |