| Message ID | 20240229212522.34515-1-dongli.zhang@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] KVM: use KVM_HVA_ERR_BAD to check bad hva | expand |
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().
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
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 --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
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(-)