Message ID | 20191211104226.20620-1-jack@suse.cz (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | powerpc/book3s64: Fix error handling in mm_iommu_do_alloc() | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (42159d2de18ffa66c2714d988a8c162db8b03956) |
snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 17 lines checked |
On 11/12/2019 21:42, Jan Kara wrote: > The last jump to free_exit in mm_iommu_do_alloc() happens after page > pointers in struct mm_iommu_table_group_mem_t were already converted to > physical addresses. Thus calling put_page() on these physical addresses > will likely crash. Convert physical addresses back to page pointers > during the error cleanup. > > Signed-off-by: Jan Kara <jack@suse.cz> > --- > arch/powerpc/mm/book3s64/iommu_api.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > Beware, this is completely untested, spotted just by code audit. > > diff --git a/arch/powerpc/mm/book3s64/iommu_api.c b/arch/powerpc/mm/book3s64/iommu_api.c > index 56cc84520577..06c403381c9c 100644 > --- a/arch/powerpc/mm/book3s64/iommu_api.c > +++ b/arch/powerpc/mm/book3s64/iommu_api.c > @@ -154,7 +154,7 @@ static long mm_iommu_do_alloc(struct mm_struct *mm, unsigned long ua, > (mem2->entries << PAGE_SHIFT)))) { > ret = -EINVAL; > mutex_unlock(&mem_list_mutex); > - goto free_exit; > + goto convert_exit; > } > } > > @@ -166,6 +166,9 @@ static long mm_iommu_do_alloc(struct mm_struct *mm, unsigned long ua, > > return 0; > > +convert_exit: > + for (i = 0; i < pinned; i++) > + mem->hpages[i] = pfn_to_page(mem->hpas[i] >> PAGE_SHIFT); Good find. Although doing it where you added "goto convert_exit" seems cleaner imho. Thanks, > free_exit: > /* free the reference taken */ > for (i = 0; i < pinned; i++) >
On Fri 20-12-19 16:06:05, Alexey Kardashevskiy wrote: > > > On 11/12/2019 21:42, Jan Kara wrote: > > The last jump to free_exit in mm_iommu_do_alloc() happens after page > > pointers in struct mm_iommu_table_group_mem_t were already converted to > > physical addresses. Thus calling put_page() on these physical addresses > > will likely crash. Convert physical addresses back to page pointers > > during the error cleanup. > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > arch/powerpc/mm/book3s64/iommu_api.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > Beware, this is completely untested, spotted just by code audit. > > > > diff --git a/arch/powerpc/mm/book3s64/iommu_api.c b/arch/powerpc/mm/book3s64/iommu_api.c > > index 56cc84520577..06c403381c9c 100644 > > --- a/arch/powerpc/mm/book3s64/iommu_api.c > > +++ b/arch/powerpc/mm/book3s64/iommu_api.c > > @@ -154,7 +154,7 @@ static long mm_iommu_do_alloc(struct mm_struct *mm, unsigned long ua, > > (mem2->entries << PAGE_SHIFT)))) { > > ret = -EINVAL; > > mutex_unlock(&mem_list_mutex); > > - goto free_exit; > > + goto convert_exit; > > } > > } > > > > @@ -166,6 +166,9 @@ static long mm_iommu_do_alloc(struct mm_struct *mm, unsigned long ua, > > > > return 0; > > > > +convert_exit: > > + for (i = 0; i < pinned; i++) > > + mem->hpages[i] = pfn_to_page(mem->hpas[i] >> PAGE_SHIFT); > > > Good find. Although doing it where you added "goto convert_exit" seems > cleaner imho. Thanks, I don't really care :). V2 attached. Honza
On 20/12/2019 20:57, Jan Kara wrote: > On Fri 20-12-19 16:06:05, Alexey Kardashevskiy wrote: >> >> >> On 11/12/2019 21:42, Jan Kara wrote: >>> The last jump to free_exit in mm_iommu_do_alloc() happens after page >>> pointers in struct mm_iommu_table_group_mem_t were already converted to >>> physical addresses. Thus calling put_page() on these physical addresses >>> will likely crash. Convert physical addresses back to page pointers >>> during the error cleanup. >>> >>> Signed-off-by: Jan Kara <jack@suse.cz> >>> --- >>> arch/powerpc/mm/book3s64/iommu_api.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> Beware, this is completely untested, spotted just by code audit. >>> >>> diff --git a/arch/powerpc/mm/book3s64/iommu_api.c b/arch/powerpc/mm/book3s64/iommu_api.c >>> index 56cc84520577..06c403381c9c 100644 >>> --- a/arch/powerpc/mm/book3s64/iommu_api.c >>> +++ b/arch/powerpc/mm/book3s64/iommu_api.c >>> @@ -154,7 +154,7 @@ static long mm_iommu_do_alloc(struct mm_struct *mm, unsigned long ua, >>> (mem2->entries << PAGE_SHIFT)))) { >>> ret = -EINVAL; >>> mutex_unlock(&mem_list_mutex); >>> - goto free_exit; >>> + goto convert_exit; >>> } >>> } >>> >>> @@ -166,6 +166,9 @@ static long mm_iommu_do_alloc(struct mm_struct *mm, unsigned long ua, >>> >>> return 0; >>> >>> +convert_exit: >>> + for (i = 0; i < pinned; i++) >>> + mem->hpages[i] = pfn_to_page(mem->hpas[i] >> PAGE_SHIFT); >> >> >> Good find. Although doing it where you added "goto convert_exit" seems >> cleaner imho. Thanks, > > I don't really care :). V2 attached. Usually patches are posted using git send-mail, not as attachment. A nit: v2 has now curly braces which v1 did not. Either way, v2 is correct. Tested-by: Alexey Kardashevskiy <aik@ozlabs.ru> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
diff --git a/arch/powerpc/mm/book3s64/iommu_api.c b/arch/powerpc/mm/book3s64/iommu_api.c index 56cc84520577..06c403381c9c 100644 --- a/arch/powerpc/mm/book3s64/iommu_api.c +++ b/arch/powerpc/mm/book3s64/iommu_api.c @@ -154,7 +154,7 @@ static long mm_iommu_do_alloc(struct mm_struct *mm, unsigned long ua, (mem2->entries << PAGE_SHIFT)))) { ret = -EINVAL; mutex_unlock(&mem_list_mutex); - goto free_exit; + goto convert_exit; } } @@ -166,6 +166,9 @@ static long mm_iommu_do_alloc(struct mm_struct *mm, unsigned long ua, return 0; +convert_exit: + for (i = 0; i < pinned; i++) + mem->hpages[i] = pfn_to_page(mem->hpas[i] >> PAGE_SHIFT); free_exit: /* free the reference taken */ for (i = 0; i < pinned; i++)
The last jump to free_exit in mm_iommu_do_alloc() happens after page pointers in struct mm_iommu_table_group_mem_t were already converted to physical addresses. Thus calling put_page() on these physical addresses will likely crash. Convert physical addresses back to page pointers during the error cleanup. Signed-off-by: Jan Kara <jack@suse.cz> --- arch/powerpc/mm/book3s64/iommu_api.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) Beware, this is completely untested, spotted just by code audit.