Re: CVS commit: src/sys/arch/x86/x86

[Maxime Villard <max%m00nbsd.net@localhost>]

Le 20/07/2016 à 07:07, Takashi YAMAMOTO a écrit :
> On Wed, Jul 20, 2016 at 3:54 AM, Maxime Villard <maxv%netbsd.org@localhost <mailto:maxv%netbsd.org@localhost>> wrote:
>
>     Module Name:    src
>     Committed By:   maxv
>     Date:           Tue Jul 19 18:54:45 UTC 2016
>
>     Modified Files:
>             src/sys/arch/x86/x86: pmap.c
>
>     Log Message:
>     This loop makes no sense at all.
>
>
> - can you provide more meaningful commit message?
>
> - why don't you remove it then?

Look at the loop. It does not do anything. The bug was introduced six years
ago in this commit [1], and there is clearly a mistake: the guy just added
a loop around the code, but now the 'continue' refers to that new loop and
not the underlying one.

I could have fixed it to keep the original behavior, but in fact, I don't
even see why this hack is relevant. This hack is supposed to make sure we
never write-protect the PTE space, but as far as I can tell, the userland
space is below it, and the kernel space is above it. So if the kernel or
userland ever tries to write-protect this area, UVM will figure out it is
not included in the space and will reject the request.

In all cases, even if there were a way for someone to write-protect the PTE
space, this hack would still be wrong: some pages in the range wouldn't be
protected, and this is something the caller may not handle properly. And
even beyond that, if write-protecting the PTE space were possible, it would
be a huge flow in the design.

So all this makes absolutely no sense at all. I didn't want to spend too
much time on it, so I just added an XXX, for the next passer-by. I guess the
best thing to do would be turning the 'continue' to a 'panic'; or just
removing the loop completely, since the KASSERT below does mostly the same
thing.

[1] http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/arch/x86/x86/pmap.c?only_with_tag=MAIN#rev1.112