| Message ID | b85e0715-3224-4f45-ad6b-ebb9f08c015d@moroto.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu/dart: Fix return code in apple_dart_domain_alloc_paging() | expand |
On Mon, Oct 30, 2023 at 12:03:12PM +0300, Dan Carpenter wrote: > The apple_dart_domain_alloc_paging() function is supposed to return NULL > on error. Returning an error pointer will lead to an Oops in > __iommu_domain_alloc(). > > Fixes: 482feb5c6492 ("iommu/dart: Call apple_dart_finalize_domain() as part of alloc_paging()") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > drivers/iommu/apple-dart.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Really need to fix this so the function does return ERR_PTR.. Thanks, Jason
On Mon, Oct 30, 2023 at 08:56:50AM -0300, Jason Gunthorpe wrote: > On Mon, Oct 30, 2023 at 12:03:12PM +0300, Dan Carpenter wrote: > > The apple_dart_domain_alloc_paging() function is supposed to return NULL > > on error. Returning an error pointer will lead to an Oops in > > __iommu_domain_alloc(). > > > > Fixes: 482feb5c6492 ("iommu/dart: Call apple_dart_finalize_domain() as part of alloc_paging()") > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > --- > > drivers/iommu/apple-dart.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > > Really need to fix this so the function does return ERR_PTR.. It's called as a function pointer. Changing that will create a backporting hazard unless we rename the pointer or something. regards, dan carpenter
On Mon, Oct 30, 2023 at 03:00:56PM +0300, Dan Carpenter wrote: > On Mon, Oct 30, 2023 at 08:56:50AM -0300, Jason Gunthorpe wrote: > > On Mon, Oct 30, 2023 at 12:03:12PM +0300, Dan Carpenter wrote: > > > The apple_dart_domain_alloc_paging() function is supposed to return NULL > > > on error. Returning an error pointer will lead to an Oops in > > > __iommu_domain_alloc(). > > > > > > Fixes: 482feb5c6492 ("iommu/dart: Call apple_dart_finalize_domain() as part of alloc_paging()") > > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > > --- > > > drivers/iommu/apple-dart.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > > > > Really need to fix this so the function does return ERR_PTR.. > > It's called as a function pointer. Changing that will create a > backporting hazard unless we rename the pointer or something. You can make that argument about almost any change in the kernel.. IMHO we needed an annotation like __user/__iomem/etc to indicate ERR_PTR so at least there is some hope of trivially finding it. Jason
On Mon, Oct 30, 2023 at 09:38:23AM -0300, Jason Gunthorpe wrote: > On Mon, Oct 30, 2023 at 03:00:56PM +0300, Dan Carpenter wrote: > > On Mon, Oct 30, 2023 at 08:56:50AM -0300, Jason Gunthorpe wrote: > > > On Mon, Oct 30, 2023 at 12:03:12PM +0300, Dan Carpenter wrote: > > > > The apple_dart_domain_alloc_paging() function is supposed to return NULL > > > > on error. Returning an error pointer will lead to an Oops in > > > > __iommu_domain_alloc(). > > > > > > > > Fixes: 482feb5c6492 ("iommu/dart: Call apple_dart_finalize_domain() as part of alloc_paging()") > > > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > > > --- > > > > drivers/iommu/apple-dart.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > > > > > > Really need to fix this so the function does return ERR_PTR.. > > > > It's called as a function pointer. Changing that will create a > > backporting hazard unless we rename the pointer or something. > > You can make that argument about almost any change in the kernel.. > > IMHO we needed an annotation like __user/__iomem/etc to indicate > ERR_PTR so at least there is some hope of trivially finding it. I was thinking about this some more, I think I prefer we fix the core code to accept the ERR_PTR and just start moving gently in that direction. I was working on some other stuff and it is starting to get really confusing that only this op is different. I'll send a patch Jason
diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c index ee05f4824bfa..cb38a7a826dc 100644 --- a/drivers/iommu/apple-dart.c +++ b/drivers/iommu/apple-dart.c @@ -761,7 +761,7 @@ static struct iommu_domain *apple_dart_domain_alloc_paging(struct device *dev) ret = apple_dart_finalize_domain(dart_domain, cfg); if (ret) { kfree(dart_domain); - return ERR_PTR(ret); + return NULL; } } return &dart_domain->domain;
The apple_dart_domain_alloc_paging() function is supposed to return NULL on error. Returning an error pointer will lead to an Oops in __iommu_domain_alloc(). Fixes: 482feb5c6492 ("iommu/dart: Call apple_dart_finalize_domain() as part of alloc_paging()") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- drivers/iommu/apple-dart.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)