diff mbox series

lib/scatterlist: Fix to calculate the last_pg properly

Message ID 20230109144701.83021-1-yishaih@nvidia.com
State New
Headers
Series lib/scatterlist: Fix to calculate the last_pg properly |

Commit Message

Yishai Hadas Jan. 9, 2023, 2:47 p.m. UTC 
  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(-)

Comments

Logan Gunthorpe Jan. 9, 2023, 6:08 p.m. UTC | #1 
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
Jens Axboe Jan. 10, 2023, 3:18 a.m. UTC | #2 
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?
Matthew Wilcox Jan. 10, 2023, 5:08 a.m. UTC | #3 
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;
Yishai Hadas Jan. 10, 2023, 8:01 a.m. UTC | #4 
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
Jason Gunthorpe Jan. 10, 2023, 1:53 p.m. UTC | #5 
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
Yishai Hadas Jan. 10, 2023, 3:05 p.m. UTC | #6 
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
Andrew Morton Jan. 11, 2023, 12:03 a.m. UTC | #7 
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?
Jason Gunthorpe Jan. 11, 2023, 1:28 p.m. UTC | #8 
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
diff mbox series

Patch

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 */