Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <9c3a29fa-6533-de16-472c-7570605da3ed@intel.com>
Date:   Thu, 1 Jun 2023 12:47:33 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH 2/2] iommu/vt-d: Use BUG_ON to check NULL value of 'table'
Content-Language: en-CA
To:     Baolu Lu <baolu.lu@linux.intel.com>, <dwmw2@infradead.org>,
        <joro@8bytes.org>, <will@kernel.org>, <robin.murphy@arm.com>
CC:     <iommu@lists.linux.dev>, <linux-kernel@vger.kernel.org>
References: <20230530092503.152926-1-yanfei.xu@intel.com>
 <20230530092503.152926-3-yanfei.xu@intel.com>
 <3e75f9de-4c67-93aa-9fb4-28da9849dd48@linux.intel.com>
 <a745c910-f4be-62f6-cb02-a610c04f4698@intel.com>
 <e735528a-7bbd-b9a8-7945-10f779fb750e@linux.intel.com>
From:   Yanfei Xu <yanfei.xu@intel.com>
In-Reply-To: <e735528a-7bbd-b9a8-7945-10f779fb750e@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?S0Q5L3BndDJjRDRkYzlxK1VueVU5VFpjNS84V0V3ZFFrRzJKb3MzdDg1VDVo?=
 =?utf-8?B?QTFsaVdxOUE4VUVjVGl2YlM1NjhqbTAvRENSckUyamp2TW1wM2F4ZFI1MGJp?=
 =?utf-8?B?TmhqMkhKaE9xeDA1bEFSbitPWjlGZ3NVTjJtV3dlbmRzRUZwQzFULzhqVHRW?=
 =?utf-8?B?S040dkdhcllTalhkZk9QQnMzelJaU2JsZVBpeGFHaXZIdGNYYjZNT21Ua3NZ?=
 =?utf-8?B?ZENXcEdVRDNreFQ4ZnpqNWlXQU83dG41aldRamYxMkJ3eCs4Wm54cVBHMzBv?=
 =?utf-8?B?R1NwekhaMzAwR2RrVmE2K245dFZMWmJnUTdTenlOajFVT2c5ZXl4NVB1MkZv?=
 =?utf-8?B?VVpEQzd3SG1KckovQThXQllXTER0TnRsYy9YWm9xc083ejdRS0xIWVl3SUE5?=
 =?utf-8?B?ZVQ4R3BOZTNVU3VOS1ptSVRxRmR1UXZPdzc3Rm5TdXR2UXRjT2o4bXZsd3p3?=
 =?utf-8?B?ajhBNzJHKytrQ3JyVnBHYTVxdkdTYkNQelAzU3F6MEsvNUtudnREUy9iSkZw?=
 =?utf-8?B?U2pXaWhGQzNHWURQd2ZkS0VIQ2tQb2VCRCtiOFFvYnc2eGZJK0xTa016akFj?=
 =?utf-8?B?Q3dTVUVQTDZhdytWS0o0RC9YdU45RjBaL0JKakRZUVpSaG1DRFdFS3hhWFh1?=
 =?utf-8?B?UkJNRlJuaEJPcmN0cmtJN3hiTHVPL3o1SC9Pb3hQZWJDeE9UcWZaaUtLcG9I?=
 =?utf-8?B?c1Q2QnJiWG5qSTlWWUZFaGY0SFk1ZCs5MnZnWnNWb1RxS3F1NDVUUm5MVVZ0?=
 =?utf-8?B?WUxIU3hrK3RxTllLTGtGU3BHTXBkNUw3RVZBK1pjMEx3VWJkdktKV2RoWTNt?=
 =?utf-8?B?R3crd0xKTlJRZEZ2a01vdkxHK0I1eXE4Z2FLZ1ZjclA2VlFGbHNsTHgzYk5S?=
 =?utf-8?B?OGpMd3l1MFRkRUgrbEFyWi9JMW1jUVA1WVM3dG92dmNocGZtU3NmSjFpV1RD?=
 =?utf-8?B?RjR5T29xWE9zTS9tRUEzSXhNQnJBNk1ZWUN5Z2dqNjcxdGNMZE1Ed3U4NG9S?=
 =?utf-8?B?L25sY1NWOFBFZnlQYlhWTWtxejFrL3JIQ25oUkpUREg3K2JCREtvd3NXclFY?=
 =?utf-8?B?cXB3S2l2OTdDN0Z0OWJIdm85bVppK1RDQlIxdjQ0UUdlNGVtV0w1cFpPZnh2?=
 =?utf-8?B?OVFjMEVnVVBNc2ZaWWZ3a1VEL2xWSHNzZkxOajVvRzBrWU1QN1phcUltci9I?=
 =?utf-8?B?ei85aldDQmMzb21zNDRPT0llUjdxem1EdkRrVHBoU3dGaXJRdFF6VnZuVld5?=
 =?utf-8?B?MXNhWVZIUmo3R0IyeUxaRTlWeWFmbFFYNG5oc1M0QW1CaU16RmVSdHYzTmdw?=
 =?utf-8?B?QzhvRkRhV0pvcWxNelo0NGhHYjFoMGdBeHhRQXZFMlNsUlJMc0ZXcy81MUlx?=
 =?utf-8?B?S0g0OGozb04xeERMRG9sTlNYVWFYR1VkaGNpTWZEMkNrQmJkamNxMU1XOXFk?=
 =?utf-8?B?cTY1eVVPR1JMOSs3cjlhNXk0U0dQVXFnUUJQQTVGQUJjOElJNURUandVUGRx?=
 =?utf-8?B?TW9xS3BwT05XU2JSS1FScmwxWWFabU5mYXZIdmJrb1pEeEc2bktYRnBnL1RM?=
 =?utf-8?B?ZHVIcVZQMC85cFg1bFRKTkxiUEdwS0ZiYXIzSmMxOVE4QzhOMHFSd0U1dVN3?=
 =?utf-8?B?S29rQnBZY1Y1WXV5aXJIWlEyRUVCZ3FpSkE2eVFpRVFsYkVCMlJqbU9ZSnlq?=
 =?utf-8?B?NzFYYWQwTGhJMGh1S3dKa3NYRDZYVld2NkRsWm4zV2ZwTU9TNm5OV1ltclJC?=
 =?utf-8?B?NjU0cFZhZlhHSmMvWnNDWi8vS3pIMjU4L1pXSi81VkVqVGIvSUJFeHBPbXBp?=
 =?utf-8?B?OFFLSWwxa1RxRzRDZW9GMkVWTGNIdVB1UjVMTUgzbHVnTFNqRUxlMlQzaHB5?=
 =?utf-8?B?dGx1VzRXTlNRQ1RBMmNLbVpLRGRPQ3VwT0ovQXl1VG1zN3VjZjM4dUJMYU1G?=
 =?utf-8?B?c1JvVWt4bXpwU2FPR2dYbjZQdnZFZkVJVi8zY3k1L1ZYRStLWEE1OUlXa0xY?=
 =?utf-8?B?ZDhoRDdDb1NhdFVybHBlbWlxYThhZklHbVFSd0lsUXdDN1RkZmxtVEFnaHJV?=
 =?utf-8?B?Z3NlTTRCeWJQZ0x3eW81MGMyN3RCL1hodENMdkRsamFyWSttVFArNktiQTRT?=
 =?utf-8?Q?0aqlqaJWw8vCtrfcCiRC0SFf1?=
X-MS-Exchange-CrossTenant-Network-Message-Id: d780a032-b035-4cec-26ff-08db625b565a
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB4138.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Jun 2023 04:47:43.8420
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Ig9MQdEXlMQEu8sSFU8d4EDJPjWnNYXmZDXZoLLZP8YTUawQQPjANVSjFBJ0kiA+jzdSRu/IJ/msyX7LTV3yXQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR11MB5413
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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