On 2/14/2018 12:33 PM, Paolo Bonzini wrote:
On 14/02/2018 17:55, Joe Moriarty wrote:
Hi Paolo,
Thank you for the review. I wasn't sure if the change I proposed was
correct or not. I will take your suggestion of posting to the mailing
list instead of as a patch the next time I encounter a situation like
this again. In the meantime, I will look at doing your suggestion of
passing kvm_memory_slot down to gfn_to_rmap() and the other functions
you pointed out above for the next version of the patch.
It's not easy, but I can send you a box of beers if you get round to it.
Note that I'm still not sure how the NULL pointer dereference can
happen, and you didn't include more output from your tool, so you might
be wasting your time after all...
Anyway, I would start basically by mapping the paths from try_async_pf's
callers to mmu_set_spte and from there to rmap_add.
On the other hand, in the rmap_remove path, you probably should just
exit immediately if slot is NULL. (Guangrong, do you have any idea why
we don't zap SPTEs in kvm_arch_free_memslot?)
Paolo
The crux of the problem is the possible return of NULL from
search_memslots() located at line 950 in include/linux/kvm_host.h. This
is called by __gfn_to_memslot(). I would assume this is a case of this
is never going to happen. I say this because the kernel would panic
today with a NULL pointer dereference at line 1254 of arch/x86/kvm/mmu.c
if it did happen.
I was thinking a BUG_ON() would be more appropriate at the end of
search_memslots() before the "return NULL". That is if my
aforementioned assumption is correct about the code path. If not, then
continue on with the suggested fix of passing down kvm_memory_slot to
gfn_to_rmap().
Let me know which solution you would like.
Joe
