MIME-Version: 1.0
References: <20211213042215.3096-1-jiangshanlai@gmail.com> <20211213042215.3096-2-jiangshanlai@gmail.com>
 <YbeaXT1Y80baey3t@zn.tnic>
In-Reply-To: <YbeaXT1Y80baey3t@zn.tnic>
From:   Lai Jiangshan <jiangshanlai@gmail.com>
Date:   Tue, 14 Dec 2021 10:51:23 +0800
Message-ID: <CAJhGHyB=VY-m7fJ+bapWgPsczhH0q9oeMmCTP5cOYJAP0VW5ZA@mail.gmail.com>
Subject: Re: [PATCH 1/3] X86/db: Change __this_cpu_read() to this_cpu_read()
 in hw_breakpoint_active()
To:     Borislav Petkov <bp@alien8.de>
Cc:     LKML <linux-kernel@vger.kernel.org>, X86 ML <x86@kernel.org>,
        Lai Jiangshan <laijs@linux.alibaba.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        "H. Peter Anvin" <hpa@zytor.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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