Re: [PATCH 1/3] X86/db: Change __this_cpu_read() to this_cpu_read() in hw_breakpoint_active()

[Lai Jiangshan]

Hello

On Tue, Dec 14, 2021 at 3:09 AM Borislav Petkov <bp@xxxxxxxxx> 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