1. Patchew
  2. linux
  3. [PATCH 0/2] Misc cleanup for iommu/vt-d
  4. View patch

[PATCH 2/2] iommu/vt-d: Use BUG_ON to check NULL value of 'table'

Yanfei Xu posted 2 patches 2 years, 8 months ago
There is a newer version of this series
[PATCH 2/2] iommu/vt-d: Use BUG_ON to check NULL value of 'table'
Posted by Yanfei Xu 2 years, 8 months ago 
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);
 
 		/* Setup the PASID DIR pointer: */
 		pds = context_get_sm_pds(table);
-- 
2.34.1
Re: [PATCH 2/2] iommu/vt-d: Use BUG_ON to check NULL value of 'table'
Posted by Baolu Lu 2 years, 8 months ago 
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;

?

Best regards,
baolu
Re: [PATCH 2/2] iommu/vt-d: Use BUG_ON to check NULL value of 'table'
Posted by Jason Gunthorpe 2 years, 8 months ago 
On Wed, May 31, 2023 at 11:26:55AM +0800, 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;
> 
> ?

If it is not something you think needs active debugging then just let
it crash on the NULL pointer deref. You get a nice backtrace from that
already.

The additional value of the WARN is it gives you a line number, a
prettier print and it might recover more cleanly (or not, it might
just crash somewhere else).

Jason
Re: [PATCH 2/2] iommu/vt-d: Use BUG_ON to check NULL value of 'table'
Posted by Yanfei Xu 2 years, 8 months ago 
Hi Baolu,

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;
+               }

Thanks,
Yanfei

> ?
>
> Best regards,
> baolu
Re: [PATCH 2/2] iommu/vt-d: Use BUG_ON to check NULL value of 'table'
Posted by Baolu Lu 2 years, 8 months ago 
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
Re: [PATCH 2/2] iommu/vt-d: Use BUG_ON to check NULL value of 'table'
Posted by Yanfei Xu 2 years, 8 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.