Messages in this thread | | | From | "Edgecombe, Rick P" <> | Subject | Re: [GIT PULL] x86/shstk for 6.4 | Date | Sun, 7 May 2023 00:10:29 +0000 |
| |
On Sat, 2023-05-06 at 12:34 -0700, Linus Torvalds wrote: > > > > End result: all those architectures that do *not* want the vma > > > > argument don't need to do any extra work, and they just > > > > implement > > the > > > > old version, and the only thing that happened was that it was > > > > > > > renamed. > > > > > > > > Because I really don't want to pull this series as-is, when I > > > > found > > > > what looks like a "this broke an architecture that DOES NOT > > > > EVEN > > > CARE" > > > > bug in the series. > > > > > > > > And yes, my bad for not getting to this earlier to notice this. > > > > > > > > Or alternatively - your bad for not going through this with a > > > > fine > > > > comb like I started doing.
Oof, yes that definitely looks like a bug. Yes, the ifdef solution would be a less error prone way to pull off the addition of the VMA.
I think I did try something like your suggestion during development. My (maybe misguided) concern was that pte_mkwrite_kernel() might not make semantic sense for all architectures since not all architectures were using pte_mkwrite() on kernel memory. Like I know a lot of architectures don't do read-only memory in the kernel even though they do it in userspace.
I also think it would still be leaving things in a slightly worse place than we started by having the actual guts of the pte_mkwrite()'s harder to grep for.
I'm surprised this was missed in automated testing, since the consequence of breaking these should have been pretty immediately obvious. Between that and all the times myself and others looked at it and still failed, maybe the less error prone solution is better.
| |