| Message ID | 20230109083429.25622-1-yf.wang@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu/iova: Fix alloc iova overflows issue | expand |
On 2023-01-09 08:34, yf.wang@mediatek.com wrote: > From: Yunfei Wang <yf.wang@mediatek.com> > > 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 <jianjiao.zeng@mediatek.com> > Signed-off-by: Yunfei Wang <yf.wang@mediatek.com> > --- > 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, yf.wang@mediatek.com wrote: > > From: Yunfei Wang <yf.wang@mediatek.com> > > > > 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 <jianjiao.zeng@mediatek.com> > > Signed-off-by: Yunfei Wang <yf.wang@mediatek.com> > > --- > > 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);
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);