[PATCH 1/2] xen/xenbus: Fix granting of vmalloc'd memory

[PATCH 1/2] xen/xenbus: Fix granting of vmalloc'd memory

[Simon Leiner]

On some architectures (like ARM), virt_to_gfn cannot be used for
vmalloc'd memory because of its reliance on virt_to_phys. This patch
introduces a check for vmalloc'd addresses and obtains the PFN using
vmalloc_to_pfn in that case.

Signed-off-by: Simon Leiner <simon@leiner.me>
---
 drivers/xen/xenbus/xenbus_client.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
index 786fbb7d8be0..907bcbb93afb 100644
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -379,8 +379,14 @@ int xenbus_grant_ring(struct xenbus_device *dev, void *vaddr,
 	int i, j;
 
 	for (i = 0; i < nr_pages; i++) {
-		err = gnttab_grant_foreign_access(dev->otherend_id,
-						  virt_to_gfn(vaddr), 0);
+		unsigned long gfn;
+
+		if (is_vmalloc_addr(vaddr))
+			gfn = pfn_to_gfn(vmalloc_to_pfn(vaddr));
+		else
+			gfn = virt_to_gfn(vaddr);
+
+		err = gnttab_grant_foreign_access(dev->otherend_id, gfn, 0);
 		if (err < 0) {
 			xenbus_dev_fatal(dev, err,
 					 "granting access to ring page");
-- 
2.24.3 (Apple Git-128)

[PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn

[Simon Leiner]

As virt_to_gfn uses virt_to_phys, it will return invalid addresses when
used with vmalloc'd addresses. This patch introduces a warning, when
virt_to_gfn is used in this way.

Signed-off-by: Simon Leiner <simon@leiner.me>
---
 include/xen/arm/page.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/xen/arm/page.h b/include/xen/arm/page.h
index d7f6af50e200..b0d303b633d0 100644
--- a/include/xen/arm/page.h
+++ b/include/xen/arm/page.h
@@ -76,7 +76,11 @@ static inline unsigned long bfn_to_pfn(unsigned long bfn)
 #define bfn_to_local_pfn(bfn)	bfn_to_pfn(bfn)
 
 /* VIRT <-> GUEST conversion */
-#define virt_to_gfn(v)		(pfn_to_gfn(virt_to_phys(v) >> XEN_PAGE_SHIFT))
+#define virt_to_gfn(v)                                                         \
+	({                                                                     \
+		WARN_ON_ONCE(is_vmalloc_addr(v));                              \
+		pfn_to_gfn(virt_to_phys(v) >> XEN_PAGE_SHIFT);                 \
+	})
 #define gfn_to_virt(m)		(__va(gfn_to_pfn(m) << XEN_PAGE_SHIFT))
 
 /* Only used in PV code. But ARM guests are always HVM. */
-- 
2.24.3 (Apple Git-128)

Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn

[Jan Beulich]

On 25.08.2020 11:31, Simon Leiner wrote:
> --- a/include/xen/arm/page.h
> +++ b/include/xen/arm/page.h
> @@ -76,7 +76,11 @@ static inline unsigned long bfn_to_pfn(unsigned long bfn)
>  #define bfn_to_local_pfn(bfn)	bfn_to_pfn(bfn)
>  
>  /* VIRT <-> GUEST conversion */
> -#define virt_to_gfn(v)		(pfn_to_gfn(virt_to_phys(v) >> XEN_PAGE_SHIFT))
> +#define virt_to_gfn(v)                                                         \
> +	({                                                                     \
> +		WARN_ON_ONCE(is_vmalloc_addr(v));                              \
> +		pfn_to_gfn(virt_to_phys(v) >> XEN_PAGE_SHIFT);                 \
> +	})

Shouldn't such a check cover more than just the vmalloc range,
i.e. everything outside of what __va() can validly return?

Jan

Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn

[Stefano Stabellini]

On Tue, 25 Aug 2020, Jan Beulich wrote:
> On 25.08.2020 11:31, Simon Leiner wrote:
> > --- a/include/xen/arm/page.h
> > +++ b/include/xen/arm/page.h
> > @@ -76,7 +76,11 @@ static inline unsigned long bfn_to_pfn(unsigned long bfn)
> >  #define bfn_to_local_pfn(bfn)	bfn_to_pfn(bfn)
> >  
> >  /* VIRT <-> GUEST conversion */
> > -#define virt_to_gfn(v)		(pfn_to_gfn(virt_to_phys(v) >> XEN_PAGE_SHIFT))
> > +#define virt_to_gfn(v)                                                         \
> > +	({                                                                     \
> > +		WARN_ON_ONCE(is_vmalloc_addr(v));                              \
> > +		pfn_to_gfn(virt_to_phys(v) >> XEN_PAGE_SHIFT);                 \
> > +	})
> 
> Shouldn't such a check cover more than just the vmalloc range,
> i.e. everything outside of what __va() can validly return?

Keep in mind that this patch is meant as sister patch to
https://marc.info/?l=xen-devel&m=159834800203817
so it makes sense to check for vmalloc addresses specifically.

That said, I am not aware of a good way to implement the __va test you
are suggesting.

Re: [PATCH 1/2] xen/xenbus: Fix granting of vmalloc'd memory

[Stefano Stabellini]

On Tue, 25 Aug 2020, Simon Leiner wrote:
> On some architectures (like ARM), virt_to_gfn cannot be used for
> vmalloc'd memory because of its reliance on virt_to_phys. This patch
> introduces a check for vmalloc'd addresses and obtains the PFN using
> vmalloc_to_pfn in that case.
> 
> Signed-off-by: Simon Leiner <simon@leiner.me>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  drivers/xen/xenbus/xenbus_client.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
> index 786fbb7d8be0..907bcbb93afb 100644
> --- a/drivers/xen/xenbus/xenbus_client.c
> +++ b/drivers/xen/xenbus/xenbus_client.c
> @@ -379,8 +379,14 @@ int xenbus_grant_ring(struct xenbus_device *dev, void *vaddr,
>  	int i, j;
>  
>  	for (i = 0; i < nr_pages; i++) {
> -		err = gnttab_grant_foreign_access(dev->otherend_id,
> -						  virt_to_gfn(vaddr), 0);
> +		unsigned long gfn;
> +
> +		if (is_vmalloc_addr(vaddr))
> +			gfn = pfn_to_gfn(vmalloc_to_pfn(vaddr));
> +		else
> +			gfn = virt_to_gfn(vaddr);
> +
> +		err = gnttab_grant_foreign_access(dev->otherend_id, gfn, 0);
>  		if (err < 0) {
>  			xenbus_dev_fatal(dev, err,
>  					 "granting access to ring page");
> -- 
> 2.24.3 (Apple Git-128)
> 

Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn

[Stefano Stabellini]

On Tue, 25 Aug 2020, Simon Leiner wrote:
> As virt_to_gfn uses virt_to_phys, it will return invalid addresses when
> used with vmalloc'd addresses. This patch introduces a warning, when
> virt_to_gfn is used in this way.
> 
> Signed-off-by: Simon Leiner <simon@leiner.me>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  include/xen/arm/page.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/include/xen/arm/page.h b/include/xen/arm/page.h
> index d7f6af50e200..b0d303b633d0 100644
> --- a/include/xen/arm/page.h
> +++ b/include/xen/arm/page.h
> @@ -76,7 +76,11 @@ static inline unsigned long bfn_to_pfn(unsigned long bfn)
>  #define bfn_to_local_pfn(bfn)	bfn_to_pfn(bfn)
>  
>  /* VIRT <-> GUEST conversion */
> -#define virt_to_gfn(v)		(pfn_to_gfn(virt_to_phys(v) >> XEN_PAGE_SHIFT))
> +#define virt_to_gfn(v)                                                         \
> +	({                                                                     \
> +		WARN_ON_ONCE(is_vmalloc_addr(v));                              \
> +		pfn_to_gfn(virt_to_phys(v) >> XEN_PAGE_SHIFT);                 \
> +	})
>  #define gfn_to_virt(m)		(__va(gfn_to_pfn(m) << XEN_PAGE_SHIFT))
>  
>  /* Only used in PV code. But ARM guests are always HVM. */
> -- 
> 2.24.3 (Apple Git-128)
> 

Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn

[Jan Beulich]

On 26.08.2020 01:48, Stefano Stabellini wrote:
> On Tue, 25 Aug 2020, Jan Beulich wrote:
>> On 25.08.2020 11:31, Simon Leiner wrote:
>>> --- a/include/xen/arm/page.h
>>> +++ b/include/xen/arm/page.h
>>> @@ -76,7 +76,11 @@ static inline unsigned long bfn_to_pfn(unsigned long bfn)
>>>  #define bfn_to_local_pfn(bfn)	bfn_to_pfn(bfn)
>>>  
>>>  /* VIRT <-> GUEST conversion */
>>> -#define virt_to_gfn(v)		(pfn_to_gfn(virt_to_phys(v) >> XEN_PAGE_SHIFT))
>>> +#define virt_to_gfn(v)                                                         \
>>> +	({                                                                     \
>>> +		WARN_ON_ONCE(is_vmalloc_addr(v));                              \
>>> +		pfn_to_gfn(virt_to_phys(v) >> XEN_PAGE_SHIFT);                 \
>>> +	})
>>
>> Shouldn't such a check cover more than just the vmalloc range,
>> i.e. everything outside of what __va() can validly return?
> 
> Keep in mind that this patch is meant as sister patch to
> https://marc.info/?l=xen-devel&m=159834800203817
> so it makes sense to check for vmalloc addresses specifically.

I had seen that patch before writing the reply, and I had assumed
precisely what you say. It was for this reason that I did point
out the limitation: While there's a specific issue for the vmalloc
range, any other addresses outside the direct mapping area are as
problematic (aiui).

> That said, I am not aware of a good way to implement the __va test you
> are suggesting.

Hmm, to me __phys_to_virt() and __virt_to_phys_nodebug() give
the impression that they're just linear transformations of the
address, which would seem to suggest there's a pre-determined
address range used for the direct map.

Jan

Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn

[Simon Leiner]

On 26.08.20 08:20, Jan Beulich wrote:
> Hmm, to me __phys_to_virt() and __virt_to_phys_nodebug() give
> the impression that they're just linear transformations of the
> address, which would seem to suggest there's a pre-determined
> address range used for the direct map.

From eyeballing the kernel code, it seems like __is_lm_address() is
suitable for such a check, especially since it is also used in
__virt_to_phys() (the one in arch/arm64/mm/physaddr.c).

Greetings,
Simon

Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn

[Jürgen Groß]

On 26.08.20 09:50, Simon Leiner wrote:
> On 26.08.20 08:20, Jan Beulich wrote:
>> Hmm, to me __phys_to_virt() and __virt_to_phys_nodebug() give
>> the impression that they're just linear transformations of the
>> address, which would seem to suggest there's a pre-determined
>> address range used for the direct map.
> 
>  From eyeballing the kernel code, it seems like __is_lm_address() is
> suitable for such a check, especially since it is also used in
> __virt_to_phys() (the one in arch/arm64/mm/physaddr.c).

This seems to be an Arm specific function.

virt_addr_valid() seems to be a good fit.


Juergen

Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn

[Simon Leiner]

On 26.08.20 09:59, Jürgen Groß wrote:
> This seems to be an Arm specific function.

Is that a problem? The caller site is also ARM specific.

> virt_addr_valid() seems to be a good fit.

If you prefer that anyway, I will change it and resubmit that part of
the patch.

Simon

Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn

[Jürgen Groß]

On 26.08.20 10:14, Simon Leiner wrote:
> On 26.08.20 09:59, Jürgen Groß wrote:
>> This seems to be an Arm specific function.
> 
> Is that a problem? The caller site is also ARM specific.

The caller site is ARM specific, but __is_lm_address() is defined for
arm64 only.

> 
>> virt_addr_valid() seems to be a good fit.
> 
> If you prefer that anyway, I will change it and resubmit that part of
> the patch.

No need for resend, I can fix that when committing.


Juergen

Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn

[Simon Leiner]

On 26.08.20 10:27, Jürgen Groß wrote:
> On 26.08.20 10:14, Simon Leiner wrote:
>> On 26.08.20 09:59, Jürgen Groß wrote:
>>> This seems to be an Arm specific function.
>>
>> Is that a problem? The caller site is also ARM specific.
> 
> The caller site is ARM specific, but __is_lm_address() is defined for
> arm64 only.

Oh, I did not catch that - you're right, thanks :-)


> No need for resend, I can fix that when committing. 

Thanks!

Simon

Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn

[Julien Grall]

Hi Simon,

On 25/08/2020 10:31, Simon Leiner wrote:
> As virt_to_gfn uses virt_to_phys, it will return invalid addresses when
> used with vmalloc'd addresses. This patch introduces a warning, when
> virt_to_gfn is used in this way.
> 
> Signed-off-by: Simon Leiner <simon@leiner.me>
> ---
>   include/xen/arm/page.h | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/include/xen/arm/page.h b/include/xen/arm/page.h
> index d7f6af50e200..b0d303b633d0 100644
> --- a/include/xen/arm/page.h
> +++ b/include/xen/arm/page.h
> @@ -76,7 +76,11 @@ static inline unsigned long bfn_to_pfn(unsigned long bfn)
>   #define bfn_to_local_pfn(bfn)	bfn_to_pfn(bfn)
>   
>   /* VIRT <-> GUEST conversion */
> -#define virt_to_gfn(v)		(pfn_to_gfn(virt_to_phys(v) >> XEN_PAGE_SHIFT))
> +#define virt_to_gfn(v)                                                         \
> +	({                                                                     \
> +		WARN_ON_ONCE(is_vmalloc_addr(v));                              \

virt_to_gfn() will usually be called from generic code. WARN_ON_ONCE() 
will only catch one instance and it means we would have to fix the first 
instance and then re-run to catch the others.

So I think we want to switch to WARN_ON() here.

Cheers,

-- 
Julien Grall

Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn

[Jürgen Groß]

On 26.08.20 20:37, Julien Grall wrote:
> Hi Simon,
> 
> On 25/08/2020 10:31, Simon Leiner wrote:
>> As virt_to_gfn uses virt_to_phys, it will return invalid addresses when
>> used with vmalloc'd addresses. This patch introduces a warning, when
>> virt_to_gfn is used in this way.
>>
>> Signed-off-by: Simon Leiner <simon@leiner.me>
>> ---
>>   include/xen/arm/page.h | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/xen/arm/page.h b/include/xen/arm/page.h
>> index d7f6af50e200..b0d303b633d0 100644
>> --- a/include/xen/arm/page.h
>> +++ b/include/xen/arm/page.h
>> @@ -76,7 +76,11 @@ static inline unsigned long bfn_to_pfn(unsigned 
>> long bfn)
>>   #define bfn_to_local_pfn(bfn)    bfn_to_pfn(bfn)
>>   /* VIRT <-> GUEST conversion */
>> -#define virt_to_gfn(v)        (pfn_to_gfn(virt_to_phys(v) >> 
>> XEN_PAGE_SHIFT))
>> +#define 
>> virt_to_gfn(v)                                                         \
>> +    
>> ({                                                                     \
>> +        WARN_ON_ONCE(is_vmalloc_addr(v));                              \
> 
> virt_to_gfn() will usually be called from generic code. WARN_ON_ONCE() 

"Usually" is a bit gross here. The only generic call site I could find
is xenbus_grant_ring(). All other instances (I counted 22) are not
generic at all.

> will only catch one instance and it means we would have to fix the first 
> instance and then re-run to catch the others.
> 
> So I think we want to switch to WARN_ON() here.

No, please don't. In case there would be a frequent path the result
would be a basically unusable system due to massive console clobbering.


Juergen

Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn

[Julien Grall]

On 27/08/2020 06:21, Jürgen Groß wrote:
> On 26.08.20 20:37, Julien Grall wrote:
> "Usually" is a bit gross here. The only generic call site I could find
> is xenbus_grant_ring(). All other instances (I counted 22) are not
> generic at all.
> 
>> will only catch one instance and it means we would have to fix the 
>> first instance and then re-run to catch the others.
>>
>> So I think we want to switch to WARN_ON() here.
> 
> No, please don't. In case there would be a frequent path the result
> would be a basically unusable system due to massive console clobbering.

Right, but if that's really happenning then you have a much bigger 
problem on your platform because the address returned will be invalid.

So I still don't see the advantage of WARN_ON_ONCE() here.

Cheers,

> 
> 
> Juergen

-- 
Julien Grall

Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn

[Jürgen Groß]

On 27.08.20 10:24, Julien Grall wrote:
> 
> 
> On 27/08/2020 06:21, Jürgen Groß wrote:
>> On 26.08.20 20:37, Julien Grall wrote:
>> "Usually" is a bit gross here. The only generic call site I could find
>> is xenbus_grant_ring(). All other instances (I counted 22) are not
>> generic at all.
>>
>>> will only catch one instance and it means we would have to fix the 
>>> first instance and then re-run to catch the others.
>>>
>>> So I think we want to switch to WARN_ON() here.
>>
>> No, please don't. In case there would be a frequent path the result
>> would be a basically unusable system due to massive console clobbering.
> 
> Right, but if that's really happenning then you have a much bigger 
> problem on your platform because the address returned will be invalid.
> 
> So I still don't see the advantage of WARN_ON_ONCE() here.

Depends of the (potential) source of the warnings. I think we can agree
that e.g. a problem in the pv network stack is rather improbable, as it
would have been detected long ago.

If, however, the problem is being introduced by one of the rather new
pv-drivers (like sound, pvcalls, 9pfs) it is perfectly fine to assume
the overall system is still functional even without those drivers
working correctly. Having a message storm from those sources is still
quite undesirable IMO and doesn't really help.


Juergen

Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1484 bytes --]

On Thu, 27 Aug 2020, Jürgen Groß wrote:
> On 27.08.20 10:24, Julien Grall wrote:
> > 
> > 
> > On 27/08/2020 06:21, Jürgen Groß wrote:
> > > On 26.08.20 20:37, Julien Grall wrote:
> > > "Usually" is a bit gross here. The only generic call site I could find
> > > is xenbus_grant_ring(). All other instances (I counted 22) are not
> > > generic at all.
> > > 
> > > > will only catch one instance and it means we would have to fix the first
> > > > instance and then re-run to catch the others.
> > > > 
> > > > So I think we want to switch to WARN_ON() here.
> > > 
> > > No, please don't. In case there would be a frequent path the result
> > > would be a basically unusable system due to massive console clobbering.
> > 
> > Right, but if that's really happenning then you have a much bigger problem
> > on your platform because the address returned will be invalid.
> > 
> > So I still don't see the advantage of WARN_ON_ONCE() here.
> 
> Depends of the (potential) source of the warnings. I think we can agree
> that e.g. a problem in the pv network stack is rather improbable, as it
> would have been detected long ago.
> 
> If, however, the problem is being introduced by one of the rather new
> pv-drivers (like sound, pvcalls, 9pfs) it is perfectly fine to assume
> the overall system is still functional even without those drivers
> working correctly. Having a message storm from those sources is still
> quite undesirable IMO and doesn't really help.

I agree with this.