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 <[email protected]>
Signed-off-by: Yishai Hadas <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Yishai Hadas <[email protected]>
Looks right by my eye, thanks.
Reviewed-by: Logan Gunthorpe <[email protected]>
Logan
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 <[email protected]>
Andrew, can you pick this one up?
--
Jens Axboe
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;
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
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
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
On Mon, 9 Jan 2023 20:18:39 -0700 Jens Axboe <[email protected]> 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 <[email protected]>
>
> 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?
On Tue, Jan 10, 2023 at 04:03:47PM -0800, Andrew Morton wrote:
> On Mon, 9 Jan 2023 20:18:39 -0700 Jens Axboe <[email protected]> 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 <[email protected]>
> >
> > 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