[v1] lib/scatterlist: Fix to calculate the last_pg properly

[PATCH] lib/scatterlist: Fix to calculate the last_pg properly

Posted by Yishai Hadas 2 years, 8 months ago

The last_pg is wrong, it is actually the first page of the last
scatterlist element. To get the last page of the last scatterlist
element we have to add prv->length. So it is checking mergability
against the wrong page, Further, a SG element is not guaranteed to end
on a page boundary, so we have to check the sub page location also for
merge eligibility.

Fix the above by checking physical contiguity, compute the actual last
page and then call pages_are_mergable().

Fixes: 1567b49d1a40 ("lib/scatterlist: add check when merging zone device pages")
Reported-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
---
 lib/scatterlist.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index f72aa50c6654..2377917893e7 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -470,22 +470,29 @@ int sg_alloc_append_table_from_pages(struct sg_append_table *sgt_append,
 		return -EOPNOTSUPP;
 
 	if (sgt_append->prv) {
+		unsigned long paddr =
+			(page_to_pfn(sg_page(sgt_append->prv)) * PAGE_SIZE +
+			 sgt_append->prv->offset + sgt_append->prv->length) /
+			PAGE_SIZE;
+
 		if (WARN_ON(offset))
 			return -EINVAL;
 
 		/* Merge contiguous pages into the last SG */
 		prv_len = sgt_append->prv->length;
-		last_pg = sg_page(sgt_append->prv);
-		while (n_pages && pages_are_mergeable(pages[0], last_pg)) {
-			if (sgt_append->prv->length + PAGE_SIZE > max_segment)
-				break;
-			sgt_append->prv->length += PAGE_SIZE;
-			last_pg = pages[0];
-			pages++;
-			n_pages--;
+		if (page_to_pfn(pages[0]) == paddr) {
+			last_pg = pfn_to_page(paddr - PAGE_SIZE);
+			while (n_pages && pages_are_mergeable(pages[0], last_pg)) {
+				if (sgt_append->prv->length + PAGE_SIZE > max_segment)
+					break;
+				sgt_append->prv->length += PAGE_SIZE;
+				last_pg = pages[0];
+				pages++;
+				n_pages--;
+			}
+			if (!n_pages)
+				goto out;
 		}
-		if (!n_pages)
-			goto out;
 	}
 
 	/* compute number of contiguous chunks */
-- 
2.18.1

Re: [PATCH] lib/scatterlist: Fix to calculate the last_pg properly

Posted by Matthew Wilcox 2 years, 8 months ago

On Mon, Jan 09, 2023 at 04:47:01PM +0200, Yishai Hadas wrote:
>  	if (sgt_append->prv) {
> +		unsigned long paddr =
> +			(page_to_pfn(sg_page(sgt_append->prv)) * PAGE_SIZE +

1. page_to_pfn() * PAGE_SIZE is spelled page_to_phys()

2. physical addresses have type phys_addr_t.  Oh, wait, paddr isn't a
physical address, it's a pfn because you divide by PAGE_SIZE at the end.
But you will get truncation on 32-bit, because page_to_pfn() has type
unsigned long.  Anyway, this shouldn't be called paddr.  Maybe last_pfn
or something?

> +			 sgt_append->prv->offset + sgt_append->prv->length) /
> +			PAGE_SIZE;

Re: [PATCH] lib/scatterlist: Fix to calculate the last_pg properly

Posted by Yishai Hadas 2 years, 8 months ago

On 10/01/2023 7:08, Matthew Wilcox wrote:
> On Mon, Jan 09, 2023 at 04:47:01PM +0200, Yishai Hadas wrote:
>>   	if (sgt_append->prv) {
>> +		unsigned long paddr =
>> +			(page_to_pfn(sg_page(sgt_append->prv)) * PAGE_SIZE +
> 1. page_to_pfn() * PAGE_SIZE is spelled page_to_phys()
>
> 2. physical addresses have type phys_addr_t.  Oh, wait, paddr isn't a
> physical address, it's a pfn because you divide by PAGE_SIZE at the end.
> But you will get truncation on 32-bit, because page_to_pfn() has type
> unsigned long.  Anyway, this shouldn't be called paddr.  Maybe last_pfn
> or something?
>
>> +			 sgt_append->prv->offset + sgt_append->prv->length) /
>> +			PAGE_SIZE;

In this area of the patch I just reverted to what we had in 6.1 [1] 
where we were good.

I can send V1 and replace the 'paddr' to be called 'last_pfn' if it's 
really needed.

Alternatively, we can stay with this patch version unless you can see 
here any real issue.

What do you suggest ?

[1] https://elixir.bootlin.com/linux/v6.1.4/source/lib/scatterlist.c#L462

Yishai

Re: [PATCH] lib/scatterlist: Fix to calculate the last_pg properly

Posted by Jason Gunthorpe 2 years, 8 months ago

On Tue, Jan 10, 2023 at 10:01:53AM +0200, Yishai Hadas wrote:
> On 10/01/2023 7:08, Matthew Wilcox wrote:
> > On Mon, Jan 09, 2023 at 04:47:01PM +0200, Yishai Hadas wrote:
> > >   	if (sgt_append->prv) {
> > > +		unsigned long paddr =
> > > +			(page_to_pfn(sg_page(sgt_append->prv)) * PAGE_SIZE +
> > 1. page_to_pfn() * PAGE_SIZE is spelled page_to_phys()
> > 
> > 2. physical addresses have type phys_addr_t.  Oh, wait, paddr isn't a
> > physical address, it's a pfn because you divide by PAGE_SIZE at the end.
> > But you will get truncation on 32-bit, because page_to_pfn() has type
> > unsigned long.  Anyway, this shouldn't be called paddr.  Maybe last_pfn
> > or something?
> > 
> > > +			 sgt_append->prv->offset + sgt_append->prv->length) /
> > > +			PAGE_SIZE;
> 
> In this area of the patch I just reverted to what we had in 6.1 [1] where we
> were good.
> 
> I can send V1 and replace the 'paddr' to be called 'last_pfn' if it's really
> needed.

Please change it as Matthew describes so it is clearer

Jason

Re: [PATCH] lib/scatterlist: Fix to calculate the last_pg properly

Posted by Yishai Hadas 2 years, 8 months ago

On 10/01/2023 15:53, Jason Gunthorpe wrote:
> On Tue, Jan 10, 2023 at 10:01:53AM +0200, Yishai Hadas wrote:
>> On 10/01/2023 7:08, Matthew Wilcox wrote:
>>> On Mon, Jan 09, 2023 at 04:47:01PM +0200, Yishai Hadas wrote:
>>>>    	if (sgt_append->prv) {
>>>> +		unsigned long paddr =
>>>> +			(page_to_pfn(sg_page(sgt_append->prv)) * PAGE_SIZE +
>>> 1. page_to_pfn() * PAGE_SIZE is spelled page_to_phys()
>>>
>>> 2. physical addresses have type phys_addr_t.  Oh, wait, paddr isn't a
>>> physical address, it's a pfn because you divide by PAGE_SIZE at the end.
>>> But you will get truncation on 32-bit, because page_to_pfn() has type
>>> unsigned long.  Anyway, this shouldn't be called paddr.  Maybe last_pfn
>>> or something?
>>>
>>>> +			 sgt_append->prv->offset + sgt_append->prv->length) /
>>>> +			PAGE_SIZE;
>> In this area of the patch I just reverted to what we had in 6.1 [1] where we
>> were good.
>>
>> I can send V1 and replace the 'paddr' to be called 'last_pfn' if it's really
>> needed.
> Please change it as Matthew describes so it is clearer
>
> Jason

OK

Once cleaning the code and renaming to 'last_pfn', could also see that 
we had to use below '- 1' instead of '- PAGE_SIZE'.

The expected diff compared to V0 can be seen below.

Will repeat some testing and send V1 once be ready.

+++ b/lib/scatterlist.c
@@ -470,18 +470,16 @@ int sg_alloc_append_table_from_pages(struct 
sg_append_table *sgt_append,
                 return -EOPNOTSUPP;

         if (sgt_append->prv) {
-               unsigned long paddr =
-                       (page_to_pfn(sg_page(sgt_append->prv)) * PAGE_SIZE +
-                        sgt_append->prv->offset + 
sgt_append->prv->length) /
-                       PAGE_SIZE;
+               unsigned long last_pfn = 
(page_to_phys(sg_page(sgt_append->prv)) +
+                       sgt_append->prv->offset + 
sgt_append->prv->length) / PAGE_SIZE;

                 if (WARN_ON(offset))
                         return -EINVAL;

                 /* Merge contiguous pages into the last SG */
                 prv_len = sgt_append->prv->length;
-               if (page_to_pfn(pages[0]) == paddr) {
-                       last_pg = pfn_to_page(paddr - PAGE_SIZE);
+               if (page_to_pfn(pages[0]) == last_pfn) {
+                       last_pg = pfn_to_page(last_pfn - 1);
                         while (n_pages && pages_are_mergeable(pages[0], 
last_pg)) {
                                 if (sgt_append->prv->length + PAGE_SIZE 
 > max_segment)
                                         break;

Yishai

Re: [PATCH] lib/scatterlist: Fix to calculate the last_pg properly

Posted by Jens Axboe 2 years, 8 months ago

On 1/9/23 7:47 AM, Yishai Hadas wrote:
> The last_pg is wrong, it is actually the first page of the last
> scatterlist element. To get the last page of the last scatterlist
> element we have to add prv->length. So it is checking mergability
> against the wrong page, Further, a SG element is not guaranteed to end
> on a page boundary, so we have to check the sub page location also for
> merge eligibility.
> 
> Fix the above by checking physical contiguity, compute the actual last
> page and then call pages_are_mergable().

Reviewed-by: Jens Axboe <axboe@kernel.dk>

Andrew, can you pick this one up?

-- 
Jens Axboe

Re: [PATCH] lib/scatterlist: Fix to calculate the last_pg properly

Posted by Andrew Morton 2 years, 8 months ago

On Mon, 9 Jan 2023 20:18:39 -0700 Jens Axboe <axboe@kernel.dk> wrote:

> On 1/9/23 7:47 AM, Yishai Hadas wrote:
> > The last_pg is wrong, it is actually the first page of the last
> > scatterlist element. To get the last page of the last scatterlist
> > element we have to add prv->length. So it is checking mergability
> > against the wrong page, Further, a SG element is not guaranteed to end
> > on a page boundary, so we have to check the sub page location also for
> > merge eligibility.
> > 
> > Fix the above by checking physical contiguity, compute the actual last
> > page and then call pages_are_mergable().
> 
> Reviewed-by: Jens Axboe <axboe@kernel.dk>
> 
> Andrew, can you pick this one up?

This patch is against Yishai's 

CommitDate: Thu Jan 5 16:01:05 2023 -0400

    lib/scatterlist: Fix to merge contiguous pages into the last SG properly
    

which is in -next, apparently via a tree of Jason's.  So I'll assume
that Jason will handleit?

Re: [PATCH] lib/scatterlist: Fix to calculate the last_pg properly

Posted by Jason Gunthorpe 2 years, 8 months ago

On Tue, Jan 10, 2023 at 04:03:47PM -0800, Andrew Morton wrote:
> On Mon, 9 Jan 2023 20:18:39 -0700 Jens Axboe <axboe@kernel.dk> wrote:
> 
> > On 1/9/23 7:47 AM, Yishai Hadas wrote:
> > > The last_pg is wrong, it is actually the first page of the last
> > > scatterlist element. To get the last page of the last scatterlist
> > > element we have to add prv->length. So it is checking mergability
> > > against the wrong page, Further, a SG element is not guaranteed to end
> > > on a page boundary, so we have to check the sub page location also for
> > > merge eligibility.
> > > 
> > > Fix the above by checking physical contiguity, compute the actual last
> > > page and then call pages_are_mergable().
> > 
> > Reviewed-by: Jens Axboe <axboe@kernel.dk>
> > 
> > Andrew, can you pick this one up?
> 
> This patch is against Yishai's 
> 
> CommitDate: Thu Jan 5 16:01:05 2023 -0400
> 
>     lib/scatterlist: Fix to merge contiguous pages into the last SG properly
>     
> 
> which is in -next, apparently via a tree of Jason's.  So I'll assume
> that Jason will handleit?

It is in v6.2-rc3 already, but sure I will take this too

Jason

Re: [PATCH] lib/scatterlist: Fix to calculate the last_pg properly

Posted by Logan Gunthorpe 2 years, 8 months ago


On 2023-01-09 07:47, Yishai Hadas wrote:
> The last_pg is wrong, it is actually the first page of the last
> scatterlist element. To get the last page of the last scatterlist
> element we have to add prv->length. So it is checking mergability
> against the wrong page, Further, a SG element is not guaranteed to end
> on a page boundary, so we have to check the sub page location also for
> merge eligibility.
> 
> Fix the above by checking physical contiguity, compute the actual last
> page and then call pages_are_mergable().
> 
> Fixes: 1567b49d1a40 ("lib/scatterlist: add check when merging zone device pages")
> Reported-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>

Looks right by my eye, thanks.

Reviewed-by: Logan Gunthorpe <logang@deltatee.com>

Logan