linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] lib/scatterlist: Fix to calculate the last_pg properly
@ 2023-01-09 14:47 Yishai Hadas
  2023-01-09 18:08 ` Logan Gunthorpe
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Yishai Hadas @ 2023-01-09 14:47 UTC (permalink / raw)
  To: linux-kernel, linux-block, linux-mm, jgg, axboe, logang
  Cc: hch, alex.williamson, yishaih, leonro, maorg

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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] lib/scatterlist: Fix to calculate the last_pg properly
  2023-01-09 14:47 [PATCH] lib/scatterlist: Fix to calculate the last_pg properly Yishai Hadas
@ 2023-01-09 18:08 ` Logan Gunthorpe
  2023-01-10  3:18 ` Jens Axboe
  2023-01-10  5:08 ` Matthew Wilcox
  2 siblings, 0 replies; 9+ messages in thread
From: Logan Gunthorpe @ 2023-01-09 18:08 UTC (permalink / raw)
  To: Yishai Hadas, linux-kernel, linux-block, linux-mm, jgg, axboe
  Cc: hch, alex.williamson, leonro, maorg



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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] lib/scatterlist: Fix to calculate the last_pg properly
  2023-01-09 14:47 [PATCH] lib/scatterlist: Fix to calculate the last_pg properly Yishai Hadas
  2023-01-09 18:08 ` Logan Gunthorpe
@ 2023-01-10  3:18 ` Jens Axboe
  2023-01-11  0:03   ` Andrew Morton
  2023-01-10  5:08 ` Matthew Wilcox
  2 siblings, 1 reply; 9+ messages in thread
From: Jens Axboe @ 2023-01-10  3:18 UTC (permalink / raw)
  To: Yishai Hadas, linux-kernel, linux-block, linux-mm, jgg, logang,
	Andrew Morton
  Cc: hch, alex.williamson, leonro, maorg

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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] lib/scatterlist: Fix to calculate the last_pg properly
  2023-01-09 14:47 [PATCH] lib/scatterlist: Fix to calculate the last_pg properly Yishai Hadas
  2023-01-09 18:08 ` Logan Gunthorpe
  2023-01-10  3:18 ` Jens Axboe
@ 2023-01-10  5:08 ` Matthew Wilcox
  2023-01-10  8:01   ` Yishai Hadas
  2 siblings, 1 reply; 9+ messages in thread
From: Matthew Wilcox @ 2023-01-10  5:08 UTC (permalink / raw)
  To: Yishai Hadas
  Cc: linux-kernel, linux-block, linux-mm, jgg, axboe, logang, hch,
	alex.williamson, leonro, maorg

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;

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] lib/scatterlist: Fix to calculate the last_pg properly
  2023-01-10  5:08 ` Matthew Wilcox
@ 2023-01-10  8:01   ` Yishai Hadas
  2023-01-10 13:53     ` Jason Gunthorpe
  0 siblings, 1 reply; 9+ messages in thread
From: Yishai Hadas @ 2023-01-10  8:01 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: linux-kernel, linux-block, linux-mm, jgg, axboe, logang, hch,
	alex.williamson, leonro, maorg

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] lib/scatterlist: Fix to calculate the last_pg properly
  2023-01-10  8:01   ` Yishai Hadas
@ 2023-01-10 13:53     ` Jason Gunthorpe
  2023-01-10 15:05       ` Yishai Hadas
  0 siblings, 1 reply; 9+ messages in thread
From: Jason Gunthorpe @ 2023-01-10 13:53 UTC (permalink / raw)
  To: Yishai Hadas
  Cc: Matthew Wilcox, linux-kernel, linux-block, linux-mm, axboe,
	logang, hch, alex.williamson, leonro, maorg

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] lib/scatterlist: Fix to calculate the last_pg properly
  2023-01-10 13:53     ` Jason Gunthorpe
@ 2023-01-10 15:05       ` Yishai Hadas
  0 siblings, 0 replies; 9+ messages in thread
From: Yishai Hadas @ 2023-01-10 15:05 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Matthew Wilcox, linux-kernel, linux-block, linux-mm, axboe,
	logang, hch, alex.williamson, leonro, maorg

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] lib/scatterlist: Fix to calculate the last_pg properly
  2023-01-10  3:18 ` Jens Axboe
@ 2023-01-11  0:03   ` Andrew Morton
  2023-01-11 13:28     ` Jason Gunthorpe
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2023-01-11  0:03 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Yishai Hadas, linux-kernel, linux-block, linux-mm, jgg, logang,
	hch, alex.williamson, leonro, maorg

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?


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] lib/scatterlist: Fix to calculate the last_pg properly
  2023-01-11  0:03   ` Andrew Morton
@ 2023-01-11 13:28     ` Jason Gunthorpe
  0 siblings, 0 replies; 9+ messages in thread
From: Jason Gunthorpe @ 2023-01-11 13:28 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Jens Axboe, Yishai Hadas, linux-kernel, linux-block, linux-mm,
	logang, hch, alex.williamson, leonro, maorg

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2023-01-11 13:33 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-09 14:47 [PATCH] lib/scatterlist: Fix to calculate the last_pg properly Yishai Hadas
2023-01-09 18:08 ` Logan Gunthorpe
2023-01-10  3:18 ` Jens Axboe
2023-01-11  0:03   ` Andrew Morton
2023-01-11 13:28     ` Jason Gunthorpe
2023-01-10  5:08 ` Matthew Wilcox
2023-01-10  8:01   ` Yishai Hadas
2023-01-10 13:53     ` Jason Gunthorpe
2023-01-10 15:05       ` Yishai Hadas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).