Messages in this thread | | | Date | Thu, 1 Jun 2023 09:23:44 +0800 | Subject | Re: [PATCH 2/2] iommu/vt-d: Use BUG_ON to check NULL value of 'table' | From | Baolu Lu <> |
| |
On 5/31/23 3:09 PM, Yanfei Xu wrote: > On 5/31/2023 11:26 AM, Baolu Lu wrote: >> On 5/30/23 5:25 PM, Yanfei Xu wrote: >>> Checking NULL value of 'table' variable deserves a BUG_ON as the >>> following code will trigger a crash by dereferencing the NULL >>> 'table' pointer. Crash in advance with BUG_ON to avoid WARN_ON >>> plus NULL pointer dereferencing can simplify the crash log. >>> >>> Signed-off-by: Yanfei Xu<yanfei.xu@intel.com> >>> --- >>> drivers/iommu/intel/iommu.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c >>> index e98f1b122b49..8aa3bfdb7f95 100644 >>> --- a/drivers/iommu/intel/iommu.c >>> +++ b/drivers/iommu/intel/iommu.c >>> @@ -1944,7 +1944,7 @@ static int domain_context_mapping_one(struct >>> dmar_domain *domain, >>> if (sm_supported(iommu)) { >>> unsigned long pds; >>> - WARN_ON(!table); >>> + BUG_ON(!table); >> >> BUG_ON() is not recommended. Perhaps, >> >> if (!table) >> -ENODEV; >> > Agree:) It is always better to handle the error than crash kernel. > > How about: > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c > index 8096273b034c..7f077e3a4128 100644 > --- a/drivers/iommu/intel/iommu.c > +++ b/drivers/iommu/intel/iommu.c > @@ -1944,7 +1944,10 @@ static int domain_context_mapping_one(struct > dmar_domain *domain, > if (sm_supported(iommu)) { > unsigned long pds; > > - WARN_ON(!table); > + if (WARN_ON(!table)) { > + ret = -ENODEV; > + goto out_unlock; > + }
I'd recommend to remove this line directly. This pointer will be accessed in the following code, if empty "table" really happens, the kernel will report a NULL pointer reference warning at the first place.
In the same function, I also saw "WARN_ON(did == 0);". It's unnecessary as domain_id_iommu() will never return 0. Perhaps we can clean it up as well.
Best regards, baolu
| |