From: Yunfei Wang <[email protected]>
In __alloc_and_insert_iova_range, there is an issue that retry_pfn
overflows. The value of iovad->anchor.pfn_hi is ~0UL, then when
iovad->cached_node is iovad->anchor, curr_iova->pfn_hi + 1 will
overflow. As a result, if the retry logic is executed, low_pfn is
updated to 0, and then new_pfn < low_pfn returns false to make the
allocation successful.
This issue occurs in the following two situations:
1. The first iova size exceeds the domain size. When initializing
iova domain, iovad->cached_node is assigned as iovad->anchor. For
example, the iova domain size is 10M, start_pfn is 0x1_F000_0000,
and the iova size allocated for the first time is 11M. The
following is the log information, new->pfn_lo is smaller than
iovad->cached_node.
Example log:
[ 223.798112][T1705487] sh: [name:iova&]__alloc_and_insert_iova_range
start_pfn:0x1f0000,retry_pfn:0x0,size:0xb00,limit_pfn:0x1f0a00
[ 223.799590][T1705487] sh: [name:iova&]__alloc_and_insert_iova_range
success start_pfn:0x1f0000,new->pfn_lo:0x1efe00,new->pfn_hi:0x1f08ff
2. The node with the largest iova->pfn_lo value in the iova domain
is deleted, iovad->cached_node will be updated to iovad->anchor,
and then the alloc iova size exceeds the maximum iova size that can
be allocated in the domain.
Adding judgment that retry_pfn must be greater than iovad->start_pfn
can fix this issue.
Signed-off-by: jianjiao zeng <[email protected]>
Signed-off-by: Yunfei Wang <[email protected]>
---
drivers/iommu/iova.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index a44ad92fc5eb..0073206c2a95 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -209,7 +209,8 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
} while (curr && new_pfn <= curr_iova->pfn_hi && new_pfn >= low_pfn);
if (high_pfn < size || new_pfn < low_pfn) {
- if (low_pfn == iovad->start_pfn && retry_pfn < limit_pfn) {
+ if (low_pfn == iovad->start_pfn &&
+ retry_pfn >= iovad->start_pfn && retry_pfn < limit_pfn) {
high_pfn = limit_pfn;
low_pfn = retry_pfn;
curr = iova_find_limit(iovad, limit_pfn);
--
2.18.0
On 2023-01-09 08:34, [email protected] wrote:
> From: Yunfei Wang <[email protected]>
>
> In __alloc_and_insert_iova_range, there is an issue that retry_pfn
> overflows. The value of iovad->anchor.pfn_hi is ~0UL, then when
> iovad->cached_node is iovad->anchor, curr_iova->pfn_hi + 1 will
> overflow. As a result, if the retry logic is executed, low_pfn is
> updated to 0, and then new_pfn < low_pfn returns false to make the
> allocation successful.
>
> This issue occurs in the following two situations:
> 1. The first iova size exceeds the domain size. When initializing
> iova domain, iovad->cached_node is assigned as iovad->anchor. For
> example, the iova domain size is 10M, start_pfn is 0x1_F000_0000,
> and the iova size allocated for the first time is 11M. The
> following is the log information, new->pfn_lo is smaller than
> iovad->cached_node.
>
> Example log:
> [ 223.798112][T1705487] sh: [name:iova&]__alloc_and_insert_iova_range
> start_pfn:0x1f0000,retry_pfn:0x0,size:0xb00,limit_pfn:0x1f0a00
> [ 223.799590][T1705487] sh: [name:iova&]__alloc_and_insert_iova_range
> success start_pfn:0x1f0000,new->pfn_lo:0x1efe00,new->pfn_hi:0x1f08ff
>
> 2. The node with the largest iova->pfn_lo value in the iova domain
> is deleted, iovad->cached_node will be updated to iovad->anchor,
> and then the alloc iova size exceeds the maximum iova size that can
> be allocated in the domain.
>
> Adding judgment that retry_pfn must be greater than iovad->start_pfn
> can fix this issue.
Hmm, indeed that's a sneaky little bug - thanks for the thorough
analysis - but couldn't we avoid the overflow entirely? I don't have the
complete logic paged in, but superficially it seems like:
retry_pfn = curr_iova->pfn_hi;
...
retry_pfn <= limit_pfn
...
low_pfn = retry_pfn + 1;
should still work, shouldn't it?
Thanks,
Robin.
> Signed-off-by: jianjiao zeng <[email protected]>
> Signed-off-by: Yunfei Wang <[email protected]>
> ---
> drivers/iommu/iova.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> index a44ad92fc5eb..0073206c2a95 100644
> --- a/drivers/iommu/iova.c
> +++ b/drivers/iommu/iova.c
> @@ -209,7 +209,8 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
> } while (curr && new_pfn <= curr_iova->pfn_hi && new_pfn >= low_pfn);
>
> if (high_pfn < size || new_pfn < low_pfn) {
> - if (low_pfn == iovad->start_pfn && retry_pfn < limit_pfn) {
> + if (low_pfn == iovad->start_pfn &&
> + retry_pfn >= iovad->start_pfn && retry_pfn < limit_pfn) {
> high_pfn = limit_pfn;
> low_pfn = retry_pfn;
> curr = iova_find_limit(iovad, limit_pfn);
On Mon, 2023-01-09 at 11:47 +0000, Robin Murphy wrote:
> On 2023-01-09 08:34, [email protected] wrote:
> > From: Yunfei Wang <[email protected]>
> >
> > In __alloc_and_insert_iova_range, there is an issue that retry_pfn
> > overflows. The value of iovad->anchor.pfn_hi is ~0UL, then when
> > iovad->cached_node is iovad->anchor, curr_iova->pfn_hi + 1 will
> > overflow. As a result, if the retry logic is executed, low_pfn is
> > updated to 0, and then new_pfn < low_pfn returns false to make the
> > allocation successful.
> >
> > This issue occurs in the following two situations:
> > 1. The first iova size exceeds the domain size. When initializing
> > iova domain, iovad->cached_node is assigned as iovad->anchor. For
> > example, the iova domain size is 10M, start_pfn is 0x1_F000_0000,
> > and the iova size allocated for the first time is 11M. The
> > following is the log information, new->pfn_lo is smaller than
> > iovad->cached_node.
> >
> > Example log:
> > [ 223.798112][T1705487] sh:
> > [name:iova&]__alloc_and_insert_iova_range
> > start_pfn:0x1f0000,retry_pfn:0x0,size:0xb00,limit_pfn:0x1f0a00
> > [ 223.799590][T1705487] sh:
> > [name:iova&]__alloc_and_insert_iova_range
> > success start_pfn:0x1f0000,new->pfn_lo:0x1efe00,new-
> > >pfn_hi:0x1f08ff
> >
> > 2. The node with the largest iova->pfn_lo value in the iova domain
> > is deleted, iovad->cached_node will be updated to iovad->anchor,
> > and then the alloc iova size exceeds the maximum iova size that can
> > be allocated in the domain.
> >
> > Adding judgment that retry_pfn must be greater than iovad-
> > >start_pfn
> > can fix this issue.
>
> Hmm, indeed that's a sneaky little bug - thanks for the thorough
> analysis - but couldn't we avoid the overflow entirely? I don't have
> the
> complete logic paged in, but superficially it seems like:
>
> retry_pfn = curr_iova->pfn_hi;
> ...
> retry_pfn <= limit_pfn
> ...
> low_pfn = retry_pfn + 1;
>
> should still work, shouldn't it?
>
> Thanks,
> Robin.
>
Hi Robin,
Thanks for your suggestion, your solution is more perfect,
PATCH v2 version will modify it.
Thanks,
Yunfei.
> > Signed-off-by: jianjiao zeng <[email protected]>
> > Signed-off-by: Yunfei Wang <[email protected]>
> > ---
> > drivers/iommu/iova.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> > index a44ad92fc5eb..0073206c2a95 100644
> > --- a/drivers/iommu/iova.c
> > +++ b/drivers/iommu/iova.c
> > @@ -209,7 +209,8 @@ static int __alloc_and_insert_iova_range(struct
> > iova_domain *iovad,
> > } while (curr && new_pfn <= curr_iova->pfn_hi && new_pfn >=
> > low_pfn);
> >
> > if (high_pfn < size || new_pfn < low_pfn) {
> > - if (low_pfn == iovad->start_pfn && retry_pfn <
> > limit_pfn) {
> > + if (low_pfn == iovad->start_pfn &&
> > + retry_pfn >= iovad->start_pfn && retry_pfn <
> > limit_pfn) {
> > high_pfn = limit_pfn;
> > low_pfn = retry_pfn;
> > curr = iova_find_limit(iovad, limit_pfn);