diff mbox series

[1/1] KVM: use KVM_HVA_ERR_BAD to check bad hva

Message ID 20240229212522.34515-1-dongli.zhang@oracle.com
State New
Headers
Series [1/1] KVM: use KVM_HVA_ERR_BAD to check bad hva |

Commit Message

Dongli Zhang Feb. 29, 2024, 9:25 p.m. UTC 
  Replace PAGE_OFFSET with KVM_HVA_ERR_BAD, to facilitate the cscope when
looking for where KVM_HVA_ERR_BAD is used.

Every time I use cscope to query the functions that are impacted by the
return value (KVM_HVA_ERR_BAD) of __gfn_to_hva_many(), I may miss
kvm_is_error_hva().

Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
 include/linux/kvm_host.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Kai Huang Feb. 29, 2024, 9:53 p.m. UTC | #1 
On 1/03/2024 10:25 am, Dongli Zhang wrote:
> Replace PAGE_OFFSET with KVM_HVA_ERR_BAD, to facilitate the cscope when
> looking for where KVM_HVA_ERR_BAD is used.
> 
> Every time I use cscope to query the functions that are impacted by the
> return value (KVM_HVA_ERR_BAD) of __gfn_to_hva_many(), I may miss
> kvm_is_error_hva().

I am not sure "to facilitate cscope" could be a justification to do some 
code change in the kernel.

> 
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
> ---
>   include/linux/kvm_host.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 7e7fd25b09b3..4dc0300e7766 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -143,7 +143,7 @@ static inline bool is_noslot_pfn(kvm_pfn_t pfn)
>   
>   static inline bool kvm_is_error_hva(unsigned long addr)
>   {
> -	return addr >= PAGE_OFFSET;
> +	return addr >= KVM_HVA_ERR_BAD;
>   }
>   
>   #endif


Also, IIUC the KVM_HVA_ERR_BAD _theoretically_ can be any random value 
that can make kvm_is_error_hva() return false, while kvm_is_error_hva() 
must catch all error HVAs.

E.g., if we ever change KVM_HVA_ERR_BAD to use any other value (although 
I don't see why this could ever happen), then using KVM_HVA_ERR_BAD in 
kvm_is_error_hva() would be broken.

In other words, it seems to me we should just use PAGE_OFFSET in 
kvm_is_error_hva().
Dongli Zhang Feb. 29, 2024, 10:13 p.m. UTC | #2 
On 2/29/24 13:53, Huang, Kai wrote:
> 
> 
> On 1/03/2024 10:25 am, Dongli Zhang wrote:
>> Replace PAGE_OFFSET with KVM_HVA_ERR_BAD, to facilitate the cscope when
>> looking for where KVM_HVA_ERR_BAD is used.
>>
>> Every time I use cscope to query the functions that are impacted by the
>> return value (KVM_HVA_ERR_BAD) of __gfn_to_hva_many(), I may miss
>> kvm_is_error_hva().
> 
> I am not sure "to facilitate cscope" could be a justification to do some code
> change in the kernel.
> 
>>
>> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
>> ---
>>   include/linux/kvm_host.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index 7e7fd25b09b3..4dc0300e7766 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -143,7 +143,7 @@ static inline bool is_noslot_pfn(kvm_pfn_t pfn)
>>     static inline bool kvm_is_error_hva(unsigned long addr)
>>   {
>> -    return addr >= PAGE_OFFSET;
>> +    return addr >= KVM_HVA_ERR_BAD;
>>   }
>>     #endif
> 
> 
> Also, IIUC the KVM_HVA_ERR_BAD _theoretically_ can be any random value that can
> make kvm_is_error_hva() return false, while kvm_is_error_hva() must catch all
> error HVAs.
> 
> E.g., if we ever change KVM_HVA_ERR_BAD to use any other value (although I don't
> see why this could ever happen), then using KVM_HVA_ERR_BAD in
> kvm_is_error_hva() would be broken.
> 
> In other words, it seems to me we should just use PAGE_OFFSET in
> kvm_is_error_hva().
> 


At least so far PAGE_OFFSET is the same value as KVM_HVA_ERR_BAD (except
mips/s390), as line 141. Therefore, this is "No functional change".

It indicates the userspace VMM can never have hva in the range of kernel space.

 139 #ifndef KVM_HVA_ERR_BAD
 140
 141 #define KVM_HVA_ERR_BAD         (PAGE_OFFSET)
 142 #define KVM_HVA_ERR_RO_BAD      (PAGE_OFFSET + PAGE_SIZE)
 143
 144 static inline bool kvm_is_error_hva(unsigned long addr)
 145 {
 146         return addr >= PAGE_OFFSET;
 147 }
 148
 149 #endif


Regarding to "facilitate cscope", this happened since long time ago when I read
about ept_violation/mmio path.

1. The __gfn_to_hva_many() may return KVM_HVA_ERR_BAD for mmio.
2. Then I used cscope to find the location of KVM_HVA_ERR_BAD.
3. The kvm_is_error_hva() is not in the results.
4. It took me a while to figure out that the 'KVM_HVA_ERR_BAD' is indirectly
used by kvm_is_error_hva().

This is just based on my own experience when reading mmio code path. Thank you
very much!

Dongli Zhang
Kai Huang Feb. 29, 2024, 10:39 p.m. UTC | #3 
On 1/03/2024 11:13 am, Dongli Zhang wrote:
> 
> 
> On 2/29/24 13:53, Huang, Kai wrote:
>>
>>
>> On 1/03/2024 10:25 am, Dongli Zhang wrote:
>>> Replace PAGE_OFFSET with KVM_HVA_ERR_BAD, to facilitate the cscope when
>>> looking for where KVM_HVA_ERR_BAD is used.
>>>
>>> Every time I use cscope to query the functions that are impacted by the
>>> return value (KVM_HVA_ERR_BAD) of __gfn_to_hva_many(), I may miss
>>> kvm_is_error_hva().
>>
>> I am not sure "to facilitate cscope" could be a justification to do some code
>> change in the kernel.
>>
>>>
>>> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
>>> ---
>>>    include/linux/kvm_host.h | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>>> index 7e7fd25b09b3..4dc0300e7766 100644
>>> --- a/include/linux/kvm_host.h
>>> +++ b/include/linux/kvm_host.h
>>> @@ -143,7 +143,7 @@ static inline bool is_noslot_pfn(kvm_pfn_t pfn)
>>>      static inline bool kvm_is_error_hva(unsigned long addr)
>>>    {
>>> -    return addr >= PAGE_OFFSET;
>>> +    return addr >= KVM_HVA_ERR_BAD;
>>>    }
>>>      #endif
>>
>>
>> Also, IIUC the KVM_HVA_ERR_BAD _theoretically_ can be any random value that can
>> make kvm_is_error_hva() return false, while kvm_is_error_hva() must catch all
>> error HVAs.
>>
>> E.g., if we ever change KVM_HVA_ERR_BAD to use any other value (although I don't
>> see why this could ever happen), then using KVM_HVA_ERR_BAD in
>> kvm_is_error_hva() would be broken.
>>
>> In other words, it seems to me we should just use PAGE_OFFSET in
>> kvm_is_error_hva().
>>
> 
> 
> At least so far PAGE_OFFSET is the same value as KVM_HVA_ERR_BAD (except
> mips/s390), as line 141. Therefore, this is "No functional change".
> 
> It indicates the userspace VMM can never have hva in the range of kernel space.
> 
>   139 #ifndef KVM_HVA_ERR_BAD
>   140
>   141 #define KVM_HVA_ERR_BAD         (PAGE_OFFSET)
>   142 #define KVM_HVA_ERR_RO_BAD      (PAGE_OFFSET + PAGE_SIZE)
>   143
>   144 static inline bool kvm_is_error_hva(unsigned long addr)
>   145 {
>   146         return addr >= PAGE_OFFSET;
>   147 }
>   148
>   149 #endif
> 
> 
> Regarding to "facilitate cscope", this happened since long time ago when I read
> about ept_violation/mmio path.
> 
> 1. The __gfn_to_hva_many() may return KVM_HVA_ERR_BAD for mmio.
> 2. Then I used cscope to find the location of KVM_HVA_ERR_BAD.
> 3. The kvm_is_error_hva() is not in the results.
> 4. It took me a while to figure out that the 'KVM_HVA_ERR_BAD' is indirectly
> used by kvm_is_error_hva().
> 
> This is just based on my own experience when reading mmio code path. Thank you
> very much!

Neither of these can justify this patch.

As I replied earlier, _logically_, IIUC kvm_is_error_hva() shouldn't use 
KVM_HVA_ERR_BAD, because the former needs to catch *ALL* bad HVA but the 
latter could be some *RANDOM* bad HVA.
diff mbox series

Patch

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 7e7fd25b09b3..4dc0300e7766 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -143,7 +143,7 @@  static inline bool is_noslot_pfn(kvm_pfn_t pfn)
 
 static inline bool kvm_is_error_hva(unsigned long addr)
 {
-	return addr >= PAGE_OFFSET;
+	return addr >= KVM_HVA_ERR_BAD;
 }
 
 #endif