[1/2] xen/virtio: Fix n_pages calculation in xen_grant_dma_map(unmap)_page()

Commit Message

Oleksandr Tyshchenko Oct. 5, 2022, 5:48 p.m. UTC

From: Oleksandr Tyshchenko <[e-mail]>

Take page offset into the account when calculating the number of pages
to be granted.

Signed-off-by: Oleksandr Tyshchenko <[e-mail]>
Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory access under Xen")
---
 drivers/xen/grant-dma-ops.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Stefano Stabellini Oct. 6, 2022, 12:07 a.m. UTC | #1

On Wed, 5 Oct 2022, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <[e-mail]>
> 
> Take page offset into the account when calculating the number of pages
> to be granted.
> 
> Signed-off-by: Oleksandr Tyshchenko <[e-mail]>
> Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory access under Xen")

Reviewed-by: Stefano Stabellini <[e-mail]>


> ---
>  drivers/xen/grant-dma-ops.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> index 8973fc1e9ccc..1998d0e8ce82 100644
> --- a/drivers/xen/grant-dma-ops.c
> +++ b/drivers/xen/grant-dma-ops.c
> @@ -153,7 +153,7 @@ static dma_addr_t xen_grant_dma_map_page(struct device *dev, struct page *page,
>  					 unsigned long attrs)
>  {
>  	struct xen_grant_dma_data *data;
> -	unsigned int i, n_pages = PFN_UP(size);
> +	unsigned int i, n_pages = PFN_UP(offset + size);
>  	grant_ref_t grant;
>  	dma_addr_t dma_handle;
>  
> @@ -185,7 +185,8 @@ static void xen_grant_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
>  				     unsigned long attrs)
>  {
>  	struct xen_grant_dma_data *data;
> -	unsigned int i, n_pages = PFN_UP(size);
> +	unsigned long offset = dma_handle & (PAGE_SIZE - 1);
> +	unsigned int i, n_pages = PFN_UP(offset + size);
>  	grant_ref_t grant;
>  
>  	if (WARN_ON(dir == DMA_NONE))
> -- 
> 2.25.1
>

Juergen Gross Oct. 6, 2022, 5:06 a.m. UTC | #2

On 05.10.22 19:48, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <[e-mail]>
> 
> Take page offset into the account when calculating the number of pages
> to be granted.
> 
> Signed-off-by: Oleksandr Tyshchenko <[e-mail]>
> Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory access under Xen")

Reviewed-by: Juergen Gross <[e-mail]>


Juergen

Xenia Ragiadakou Oct. 6, 2022, 7:35 a.m. UTC | #3

On 10/5/22 20:48, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <[e-mail]>
> 
> Take page offset into the account when calculating the number of pages
> to be granted.
> 
> Signed-off-by: Oleksandr Tyshchenko <[e-mail]>
> Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory access under Xen")
> ---
>   drivers/xen/grant-dma-ops.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> index 8973fc1e9ccc..1998d0e8ce82 100644
> --- a/drivers/xen/grant-dma-ops.c
> +++ b/drivers/xen/grant-dma-ops.c
> @@ -153,7 +153,7 @@ static dma_addr_t xen_grant_dma_map_page(struct device *dev, struct page *page,
>   					 unsigned long attrs)
>   {
>   	struct xen_grant_dma_data *data;
> -	unsigned int i, n_pages = PFN_UP(size);
> +	unsigned int i, n_pages = PFN_UP(offset + size);

Here, why do we use PFN_UP and not XEN_PFN_UP?
Also, since the variable 'n_pages' seems to refer to the number of 
grants (unless I got it all entirely wrong ...), wouldn't it be more 
suitable to call explicitly gnttab_count_grant()?
If the above questions have been already answered in the past, please 
ignore.

>   	grant_ref_t grant;
>   	dma_addr_t dma_handle;
>   
> @@ -185,7 +185,8 @@ static void xen_grant_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
>   				     unsigned long attrs)
>   {
>   	struct xen_grant_dma_data *data;
> -	unsigned int i, n_pages = PFN_UP(size);
> +	unsigned long offset = dma_handle & (PAGE_SIZE - 1);
> +	unsigned int i, n_pages = PFN_UP(offset + size);
>   	grant_ref_t grant;
>   
>   	if (WARN_ON(dir == DMA_NONE))

Juergen Gross Oct. 6, 2022, 8:05 a.m. UTC | #4

On 06.10.22 09:35, Xenia Ragiadakou wrote:
> 
> On 10/5/22 20:48, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <[e-mail]>
>>
>> Take page offset into the account when calculating the number of pages
>> to be granted.
>>
>> Signed-off-by: Oleksandr Tyshchenko <[e-mail]>
>> Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory access 
>> under Xen")
>> ---
>>   drivers/xen/grant-dma-ops.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
>> index 8973fc1e9ccc..1998d0e8ce82 100644
>> --- a/drivers/xen/grant-dma-ops.c
>> +++ b/drivers/xen/grant-dma-ops.c
>> @@ -153,7 +153,7 @@ static dma_addr_t xen_grant_dma_map_page(struct device 
>> *dev, struct page *page,
>>                        unsigned long attrs)
>>   {
>>       struct xen_grant_dma_data *data;
>> -    unsigned int i, n_pages = PFN_UP(size);
>> +    unsigned int i, n_pages = PFN_UP(offset + size);
> 
> Here, why do we use PFN_UP and not XEN_PFN_UP?
> Also, since the variable 'n_pages' seems to refer to the number of grants 
> (unless I got it all entirely wrong ...), wouldn't it be more suitable to call 
> explicitly gnttab_count_grant()?

Good point.

I think this will need another patch for switching grant-dma-ops.c to
use XEN_PAGE_SIZE and XEN_PAGE_SHIFT.


Juergen

Oleksandr Tyshchenko Oct. 6, 2022, 10:14 a.m. UTC | #5

On Thu, Oct 6, 2022 at 11:05 AM Juergen Gross <[e-mail]> wrote:

> On 06.10.22 09:35, Xenia Ragiadakou wrote:
>


Hello Xenia, Juergen

[sorry for the possible format issues]



> >
> > On 10/5/22 20:48, Oleksandr Tyshchenko wrote:
> >> From: Oleksandr Tyshchenko <[e-mail]>
> >>
> >> Take page offset into the account when calculating the number of pages
> >> to be granted.
> >>
> >> Signed-off-by: Oleksandr Tyshchenko <[e-mail]>
> >> Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory
> access
> >> under Xen")
> >> ---
> >>   drivers/xen/grant-dma-ops.c | 5 +++--
> >>   1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> >> index 8973fc1e9ccc..1998d0e8ce82 100644
> >> --- a/drivers/xen/grant-dma-ops.c
> >> +++ b/drivers/xen/grant-dma-ops.c
> >> @@ -153,7 +153,7 @@ static dma_addr_t xen_grant_dma_map_page(struct
> device
> >> *dev, struct page *page,
> >>                        unsigned long attrs)
> >>   {
> >>       struct xen_grant_dma_data *data;
> >> -    unsigned int i, n_pages = PFN_UP(size);
> >> +    unsigned int i, n_pages = PFN_UP(offset + size);
> >
> > Here, why do we use PFN_UP and not XEN_PFN_UP?
> > Also, since the variable 'n_pages' seems to refer to the number of
> grants
> > (unless I got it all entirely wrong ...), wouldn't it be more suitable
> to call
> > explicitly gnttab_count_grant()?
>
> Good point.
>

+1


>
> I think this will need another patch for switching grant-dma-ops.c to
> use XEN_PAGE_SIZE and XEN_PAGE_SHIFT.
>

+1

I can create a separate patch for converting on top of this series, it
would be nice to clarify one point.

So I will convert PAGE_SIZE/PAGE_SHIFT to XEN_PAGE_SIZE/XEN_PAGE_SHIFT
respectively (where appropriate).

Should the PFN_UP be converted to XEN_PFN_UP *or* use
gnttab_count_grant() explicitly? Personally I would prefer the former, but
would also be ok with the latter.



>
>
> Juergen
>
>

Juergen Gross Oct. 6, 2022, 10:24 a.m. UTC | #6

On 06.10.22 12:14, Oleksandr Tyshchenko wrote:
> 
> 
> On Thu, Oct 6, 2022 at 11:05 AM Juergen Gross <[e-mail] 
> <mailto:[e-mail]>> wrote:
> 
>     On 06.10.22 09:35, Xenia Ragiadakou wrote:
> 
> 
> 
> Hello Xenia, Juergen
> 
> [sorry for the possible format issues]
> 
>      >
>      > On 10/5/22 20:48, Oleksandr Tyshchenko wrote:
>      >> From: Oleksandr Tyshchenko <[e-mail]
>     <mailto:[e-mail]>>
>      >>
>      >> Take page offset into the account when calculating the number of pages
>      >> to be granted.
>      >>
>      >> Signed-off-by: Oleksandr Tyshchenko <[e-mail]
>     <mailto:[e-mail]>>
>      >> Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory
>     access
>      >> under Xen")
>      >> ---
>      >>   drivers/xen/grant-dma-ops.c | 5 +++--
>      >>   1 file changed, 3 insertions(+), 2 deletions(-)
>      >>
>      >> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
>      >> index 8973fc1e9ccc..1998d0e8ce82 100644
>      >> --- a/drivers/xen/grant-dma-ops.c
>      >> +++ b/drivers/xen/grant-dma-ops.c
>      >> @@ -153,7 +153,7 @@ static dma_addr_t xen_grant_dma_map_page(struct device
>      >> *dev, struct page *page,
>      >>                        unsigned long attrs)
>      >>   {
>      >>       struct xen_grant_dma_data *data;
>      >> -    unsigned int i, n_pages = PFN_UP(size);
>      >> +    unsigned int i, n_pages = PFN_UP(offset + size);
>      >
>      > Here, why do we use PFN_UP and not XEN_PFN_UP?
>      > Also, since the variable 'n_pages' seems to refer to the number of grants
>      > (unless I got it all entirely wrong ...), wouldn't it be more suitable to
>     call
>      > explicitly gnttab_count_grant()?
> 
>     Good point.
> 
> 
> +1
> 
> 
>     I think this will need another patch for switching grant-dma-ops.c to
>     use XEN_PAGE_SIZE and XEN_PAGE_SHIFT.
> 
> 
> +1
> 
> I can create a separate patch for converting on top of this series, it would be 
> nice to clarify one point.
> 
> So I will convert PAGE_SIZE/PAGE_SHIFT to XEN_PAGE_SIZE/XEN_PAGE_SHIFT 
> respectively (where appropriate).

Yes, that would be the idea.

> Should the PFN_UP be converted to XEN_PFN_UP *or* use 
> gnttab_count_grant() explicitly? Personally I would prefer the former, but would 
> also be ok with the latter.

I agree XEN_PFN_UP would be better, especially as XEN_PAGE_SIZE/XEN_PAGE_SHIFT
will be used in the same functions.


Juergen

Oleksandr Tyshchenko Oct. 6, 2022, 10:30 a.m. UTC | #7

On Thu, Oct 6, 2022 at 1:24 PM Juergen Gross <[e-mail]> wrote:

Hello Juergen

[sorry for the possible format issues]

On 06.10.22 12:14, Oleksandr Tyshchenko wrote:
> >
> >
> > On Thu, Oct 6, 2022 at 11:05 AM Juergen Gross <[e-mail]
> > <mailto:[e-mail]>> wrote:
> >
> >     On 06.10.22 09:35, Xenia Ragiadakou wrote:
> >
> >
> >
> > Hello Xenia, Juergen
> >
> > [sorry for the possible format issues]
> >
> >      >
> >      > On 10/5/22 20:48, Oleksandr Tyshchenko wrote:
> >      >> From: Oleksandr Tyshchenko <[e-mail]
> >     <mailto:[e-mail]>>
> >      >>
> >      >> Take page offset into the account when calculating the number of
> pages
> >      >> to be granted.
> >      >>
> >      >> Signed-off-by: Oleksandr Tyshchenko <
> [e-mail]
> >     <mailto:[e-mail]>>
> >      >> Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict
> memory
> >     access
> >      >> under Xen")
> >      >> ---
> >      >>   drivers/xen/grant-dma-ops.c | 5 +++--
> >      >>   1 file changed, 3 insertions(+), 2 deletions(-)
> >      >>
> >      >> diff --git a/drivers/xen/grant-dma-ops.c
> b/drivers/xen/grant-dma-ops.c
> >      >> index 8973fc1e9ccc..1998d0e8ce82 100644
> >      >> --- a/drivers/xen/grant-dma-ops.c
> >      >> +++ b/drivers/xen/grant-dma-ops.c
> >      >> @@ -153,7 +153,7 @@ static dma_addr_t
> xen_grant_dma_map_page(struct device
> >      >> *dev, struct page *page,
> >      >>                        unsigned long attrs)
> >      >>   {
> >      >>       struct xen_grant_dma_data *data;
> >      >> -    unsigned int i, n_pages = PFN_UP(size);
> >      >> +    unsigned int i, n_pages = PFN_UP(offset + size);
> >      >
> >      > Here, why do we use PFN_UP and not XEN_PFN_UP?
> >      > Also, since the variable 'n_pages' seems to refer to the number
> of grants
> >      > (unless I got it all entirely wrong ...), wouldn't it be more
> suitable to
> >     call
> >      > explicitly gnttab_count_grant()?
> >
> >     Good point.
> >
> >
> > +1
> >
> >
> >     I think this will need another patch for switching grant-dma-ops.c to
> >     use XEN_PAGE_SIZE and XEN_PAGE_SHIFT.
> >
> >
> > +1
> >
> > I can create a separate patch for converting on top of this series, it
> would be
> > nice to clarify one point.
> >
> > So I will convert PAGE_SIZE/PAGE_SHIFT to XEN_PAGE_SIZE/XEN_PAGE_SHIFT
> > respectively (where appropriate).
>
> Yes, that would be the idea.
>
> > Should the PFN_UP be converted to XEN_PFN_UP *or* use
> > gnttab_count_grant() explicitly? Personally I would prefer the former,
> but would
> > also be ok with the latter.
>
> I agree XEN_PFN_UP would be better, especially as
> XEN_PAGE_SIZE/XEN_PAGE_SHIFT
> will be used in the same functions.
>


Thanks for the clarification.



>
>
> Juergen
>

Patch

diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
index 8973fc1e9ccc..1998d0e8ce82 100644
--- a/drivers/xen/grant-dma-ops.c
+++ b/drivers/xen/grant-dma-ops.c
@@ -153,7 +153,7 @@  static dma_addr_t xen_grant_dma_map_page(struct device *dev, struct page *page,
 					 unsigned long attrs)
 {
 	struct xen_grant_dma_data *data;
-	unsigned int i, n_pages = PFN_UP(size);
+	unsigned int i, n_pages = PFN_UP(offset + size);
 	grant_ref_t grant;
 	dma_addr_t dma_handle;
 
@@ -185,7 +185,8 @@  static void xen_grant_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
 				     unsigned long attrs)
 {
 	struct xen_grant_dma_data *data;
-	unsigned int i, n_pages = PFN_UP(size);
+	unsigned long offset = dma_handle & (PAGE_SIZE - 1);
+	unsigned int i, n_pages = PFN_UP(offset + size);
 	grant_ref_t grant;
 
 	if (WARN_ON(dir == DMA_NONE))