Messages in this thread | | | From | Lai Jiangshan <> | Date | Tue, 14 Dec 2021 10:51:23 +0800 | Subject | Re: [PATCH 1/3] X86/db: Change __this_cpu_read() to this_cpu_read() in hw_breakpoint_active() |
| |
Hello
On Tue, Dec 14, 2021 at 3:09 AM Borislav Petkov <bp@alien8.de> wrote:
> 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.
The commit message was checked via VIM spellchecker. It did highlight denylist, noinstr, noinstrument, complexify, and a lot more.
There are too many false-negative results from VIM spellchecker, and I searched denylist, complexify via google and they are used by some other places so I kept them.
I'm sorry for not searching in the kernel tree to find a proper word for noinstrument, not searching the web for better words for denylist, complexify.
I will change a spellchecker and improve my English.
> > > 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.
What I wanted to say in this paragraph is that why I chose this way to fix it since there are several ways/policies to fix it.
"Changing __this_cpu_read() to this_cpu_read() is fit for" this policy.
I don't think it can be seen in the diff.
> I don't really follow the argument for why this_cpu_read(); why not > raw_cpu_read() instead, which is what __this_cpu_read() is based on. > Also, this really needs a comment.
Yes, raw_cpu_read() is better.
Some other places in noinstr function use this_cpu_read(), so I did not search if there is a better alternative. I just reviewed the definition of this_cpu_read() and concluded that it can be used.
> /* > * Must not hit a breakpoint in check_preempt_disabled() > */ > return raw_cpu_read(cpu_dr7) & DR_GLOBAL_ENABLE_MASK;
Although, this comment is describing raw_cpu_read() obviously, I often can't get which code is a comment in other places referring to due to later changes with new code added and removed.
Can I duplicate the code in the comments? Use raw_cpu_read() instead of __this_cpu_read() to avoid hitting a breakpoint in check_preempt_disabled().
Thanks Lai
| |