Messages in this thread | | | Date | Thu, 1 Jun 2023 12:47:33 +0800 | Subject | Re: [PATCH 2/2] iommu/vt-d: Use BUG_ON to check NULL value of 'table' | From | Yanfei Xu <> |
| |
On 6/1/2023 9:23 AM, Baolu Lu wrote: > 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.
Will do in V2.
Thanks, Yanfei
> > Best regards, > baolu
| |