Messages in this thread | | | Subject | Re: [PATCH 02/24] x86/traps: Move arch/x86/kernel/traps.c to arch/x86/entry/ | From | Lai Jiangshan <> | Date | Thu, 2 Sep 2021 19:54:25 +0800 |
| |
On 2021/9/2 18:50, Peter Zijlstra wrote: > On Thu, Sep 02, 2021 at 05:21:51PM +0800, Lai Jiangshan wrote: >> >> >> On 2021/9/2 16:09, Joerg Roedel wrote: >>> On Wed, Sep 01, 2021 at 01:50:03AM +0800, Lai Jiangshan wrote: >>>> arch/x86/entry/Makefile | 5 ++++- >>>> arch/x86/{kernel => entry}/traps.c | 0 >>>> arch/x86/kernel/Makefile | 5 +---- >>>> 3 files changed, 5 insertions(+), 5 deletions(-) >>>> rename arch/x86/{kernel => entry}/traps.c (100%) >>> >>> From looking over the patch-set I didn't spot the reason for putting the >>> entry C code into traps.c. Can you explain that please? Otherwise I'd >>> prefer to create a new file, like arch/x86/entry/entry64.c. >> >> >> I agree, and I think Peter is handling it. > > I don't think I said that. I said I like the patches but there's a lot > of scary code and details to review, which takes time. > > I've now done a second reading of the patches and provided some more > feedback, but I'm very sure I didn't get to the scary details yet. > > One thing that got pointed out (by Andrew Cooper) is that by moving the > whole SWAPGS trainwreck to C it becomes entirely 'trivial' to sneak in > an 'accidental' per-cpu reference before having done the SWAPGS dance. > > I'm myself not (yet) convinced that's a good enough reason to leave it > in ASM, but it does certainly need consideration. >
It is real concern and it proves that my having put the C code in traps.c was totally wrong.
To relieve the concern, I think the C code can be put into a single file, like arch/x86/entry/entry64.c, and be documented that it is as critical, dangerous as entry_64.S and any one should take no less care on modifying/reviewing it than on modifying/reviewing entry_64.S.
And all the other users of native_swapgs can be moved to this file too, such as __[rd|wr]gsbase_inactive().
A noninstr function can sometimes have 'accidental' instrument to sneak in. For example, stack-protector is instrumenting many noninstr functions now if the CONFIG is yes. It is normally Ok and gcc is adding per-function control on it.
But the C code can not be instrumented by any way. For example stack-protector would add per-cpu reference before having done the SWAPGS dance.) Entry C code required a stronger limitation than noninstr code.
By the way, can objtool check the per-cpu reference?
| |