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: <56393e84-485b-42ba-5fce-d4a0d0017653@amd.com>
Date:   Mon, 21 Nov 2022 19:23:49 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH v4] dma-buf: fix racing conflict of dma_heap_add()
Content-Language: en-US
To:     Dawei Li <set_pte_at@outlook.com>, sumit.semwal@linaro.org
Cc:     benjamin.gaignard@collabora.com, labbott@redhat.com,
        Brian.Starkey@arm.com, jstultz@google.com, afd@ti.com,
        sspatil@android.com, linux-media@vger.kernel.org,
        dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org,
        linux-kernel@vger.kernel.org
References: <TYCP286MB2323873BBDF88020781FB986CA3B9@TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM>
 <TYCP286MB2323894F9939C27291FD998CCA0A9@TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
In-Reply-To: <TYCP286MB2323894F9939C27291FD998CCA0A9@TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bWY4ZUluOWowcmVYTjI1bjRVT0FLbkExa29tMHNxUEtLTDFRcFhmWEhaVXhU?=
 =?utf-8?B?UWRFWUs1bnh5bUFCbi9xVFp5bDVTZ0pUTS94OHBTNlBpdjhkTWdRUDV5Y2c4?=
 =?utf-8?B?RFVrK2Z4bnRlOGp1RUFRMXFEczQ5NkNkNmsvZE5DMnFTOW9DZWNnZGR1K2Zz?=
 =?utf-8?B?L1F5WFFXdVVTOFZJRm9QZ2UwL1B2OTlLTUlDaDBVK1BxdksrUVFRc2NvOUFr?=
 =?utf-8?B?UUEwZ3JNV2dBUDRZcGNDMHEyY2RwMERMVkh1RDlXaW9BMFl6WkdrcGF4eVlG?=
 =?utf-8?B?TVZzT3BDMjRjYW4yWUtUVHpyZWhQUHBiVFpXRHJnZjB5Mm1TOHhKa1RFKzBV?=
 =?utf-8?B?QnlLQU1jRDVVbWR1N2tmdGFyRFM4UGNVRU9salVDRXlYMWRZK0x3Zy85NTly?=
 =?utf-8?B?WURRUkpOVXNCQkhtVWZwNGxkdXNxTkVYS0xpME84M1V1aXBnRXNOejE0c251?=
 =?utf-8?B?aTJtNlBPYWdUWWVYMUtnMll3ZTh2T3dMNmZXRjhvQ3lGdWxWOWVUTm1xblY2?=
 =?utf-8?B?ZE5WOUtEVFhXK3BVYXFXUm1mSTMvbFNobFZnR0tEaTQzU2dJWVQ4bHl3Y09q?=
 =?utf-8?B?SjZFRlU3YTZrY0h1YkFCUEUxUlVvRUREVmRWbFVlMFluRTdmalJDUmhndW1E?=
 =?utf-8?B?U2lnN0pldlViNElpZlExT3FmQnU4SzQ0MXlrdTNGVTlIVkMxai8yTXpXRWFB?=
 =?utf-8?B?UHB4OFZPMzNUU1p6bzNnYUFaUHptQXJObmdkUTYzWEpaazRndXdKTGtYZHpN?=
 =?utf-8?B?K21GRTY5VGlrUStNVy90WEJTaW9YMk5hNUtsMjdsWGtSRVQvMVpQcU1YNytK?=
 =?utf-8?B?R2cxTWMyQkplL0J3MzJaV1JwSTFLcWhQbUF1ODRsUUJEQ0tMWEErMmE0dXRt?=
 =?utf-8?B?WCtwY1VueDRBc0xQRlYveWxhK2tGc1B4YUc1MEVZSGhsN0owcnNOZnlDUUNH?=
 =?utf-8?B?NUJjNkxvRFlSejdwMkRPalpTQnFvM0pxcXduejFVeGNoQUNKZzArNys0Vm1z?=
 =?utf-8?B?REVBZXBDOVBqREQxdVRwRFZaNkJQd01kQ1F1d1BpYzFnWjJRNGhpOGRRckda?=
 =?utf-8?B?RFBZWHF4Q3dFeW9aWXpLdFh1Y0YvUWVEQ094MDNDMWlFd0w1MHBaSlp3RFVl?=
 =?utf-8?B?MkZ2clR1UkJLbWl5T256RUdPY0tqaHU5NS9Pc045UDR5N25GcSs5SjUxcWdP?=
 =?utf-8?B?TnQ4aUxVc1dneU9pSGZXaVZ2czVLOGc5bFBzL3B2WjArUjNUS2JTSFNrL3Yv?=
 =?utf-8?B?dzkxdnNZbkxWMnRPSkVpbVpDTEhwNDRjT3NmRjJDY0lUOVdTK0g3aWwzdmJz?=
 =?utf-8?B?bU5BV0lZcnVuSmxaWElmbDBpdndKWjFXSWs0UEFaL3EwQy93WnJCZWJRdFMw?=
 =?utf-8?B?OUg1Q2NCc08zeTVhM2NNVEZZWEFGS1ZoaWNLRUpSNTFhK0tBMUNZOWdyVkdy?=
 =?utf-8?B?RTZlaGhOZHd0KzFjdUlRL05nT3hHYXM3cjVJSGd3eVZhNGMrSUZjV3R0eC9K?=
 =?utf-8?B?azJqOW5ySEpRc2xEM0VnUXVHS0lMUHIrN2x3T0JVZ1Fuc3dlWHJFWDNXZnlU?=
 =?utf-8?B?UW45ZmNyS1FDWWVNZzBFMktpWmpIQVFlN2FHRUpMM2lQcnpBQlBGMkRmYjRO?=
 =?utf-8?B?SU00dnQ1eENjVExhLzEzZW9UeFN3ZlVnMUFQUURVMnRZdEoycE5EUDRCZ2Ux?=
 =?utf-8?B?SFhZQzFUSEo1NVVPR0RKSXFvUjNDVFg4RmEwSHZ3Z2ppYUhCcXBadHRTMTQz?=
 =?utf-8?B?a0FHR3VpTDNVSEkxcWRaWHlYOElYN2MvK0tXY3FSZGkzOFN2WTYvbUdYbXkx?=
 =?utf-8?B?aWU1alZiai9mbWQxNy9CSTZ5Q2hwTTdwZ2U0RTFwcG1HZjJ4cXo5WkIzc2h2?=
 =?utf-8?B?M3doV1JlMmpvNHN1cVlPS3pPbFJxbXBvRkFaUlk1amIycXJ4TjQvUU1DQjVU?=
 =?utf-8?B?MWs5d3lSOTc0dXB3eUFINUc2OHlCVkkyWkMrenlkc000emgvUTh1aFhES1RC?=
 =?utf-8?B?UEFiT0JqazZoblRIT3ZPTVpQVU9ZenQ1MldhOW5VSmFvM0pDYm9BWGNwd0xG?=
 =?utf-8?B?SWhUTVpnOG9qY01KNGJ4MmVUeEh3R2Q3dVZDa2doTnlzcHh0QXdJVDNzRnRB?=
 =?utf-8?B?L1k2Z0Zqc3dMbGNxRFo0MWRBc05oMkc1TzBwZzJ0amN3SGFRRjFTTWJuVm5q?=
 =?utf-8?Q?+6qu3D3rESEtxpi+wQHC4sQ7JoqirLyhabDkGbfhh6Wo?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e7060fa7-e589-4a13-d494-08dacbed8d10
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3587.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Nov 2022 18:23:56.2287
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Nat/OP+eE1/vj+0q2oWKzshzQs1Hx1fP2MNYhhBGLFxS1/YEofYZQNmxCapP1h3+
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM8PR12MB5461
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS 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

Hi Dawei,

from the technical description, coding style etc.. it looks clean to me, 
but I'm the completely wrong person to ask for a background check.

I have a high level understanding of how dma-heaps work, but not a 
single line of this code is from me.

Feel free to add my Acked-by, but Laura, John and others do you have any 
opinion?

Regards,
Christian.

Am 21.11.22 um 16:28 schrieb Dawei Li:
> On Sat, Nov 05, 2022 at 12:05:36AM +0800, Dawei Li wrote:
>
> Hi Christian,
> May I have your opinion on this change?
>
> Thanks,
> Dawei
>
>> Racing conflict could be:
>> task A                 task B
>> list_for_each_entry
>> strcmp(h->name))
>>                         list_for_each_entry
>>                         strcmp(h->name)
>> kzalloc                kzalloc
>> ......                 .....
>> device_create          device_create
>> list_add
>>                         list_add
>>
>> The root cause is that task B has no idea about the fact someone
>> else(A) has inserted heap with same name when it calls list_add,
>> so a potential collision occurs.
>>
>> Fixes: c02a81fba74f ("dma-buf: Add dma-buf heaps framework")
>> Signed-off-by: Dawei Li <set_pte_at@outlook.com>
>> ---
>> v1: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2FTYCP286MB2323950197F60FC3473123B7CA349%40TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM%2F&amp;data=05%7C01%7Cchristian.koenig%40amd.com%7C1989f31257fc458a27c508dacbd5078e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638046413707443203%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=OWPUPrIHGnzwXyQE4WlIthThwSuSK2y3Eq2Wq5zAzbo%3D&amp;reserved=0
>> v1->v2: Narrow down locking scope, check the existence of heap before
>> insertion, as suggested by Andrew Davis.
>> v2->v3: Remove double checking.
>> v3->v4: Minor coding style and patch formatting adjustment.
>> ---
>>   drivers/dma-buf/dma-heap.c | 28 +++++++++++++++-------------
>>   1 file changed, 15 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
>> index 8f5848aa144f..59d158873f4c 100644
>> --- a/drivers/dma-buf/dma-heap.c
>> +++ b/drivers/dma-buf/dma-heap.c
>> @@ -233,18 +233,6 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
>>   		return ERR_PTR(-EINVAL);
>>   	}
>>   
>> -	/* check the name is unique */
>> -	mutex_lock(&heap_list_lock);
>> -	list_for_each_entry(h, &heap_list, list) {
>> -		if (!strcmp(h->name, exp_info->name)) {
>> -			mutex_unlock(&heap_list_lock);
>> -			pr_err("dma_heap: Already registered heap named %s\n",
>> -			       exp_info->name);
>> -			return ERR_PTR(-EINVAL);
>> -		}
>> -	}
>> -	mutex_unlock(&heap_list_lock);
>> -
>>   	heap = kzalloc(sizeof(*heap), GFP_KERNEL);
>>   	if (!heap)
>>   		return ERR_PTR(-ENOMEM);
>> @@ -283,13 +271,27 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
>>   		err_ret = ERR_CAST(dev_ret);
>>   		goto err2;
>>   	}
>> -	/* Add heap to the list */
>> +
>>   	mutex_lock(&heap_list_lock);
>> +	/* check the name is unique */
>> +	list_for_each_entry(h, &heap_list, list) {
>> +		if (!strcmp(h->name, exp_info->name)) {
>> +			mutex_unlock(&heap_list_lock);
>> +			pr_err("dma_heap: Already registered heap named %s\n",
>> +			       exp_info->name);
>> +			err_ret = ERR_PTR(-EINVAL);
>> +			goto err3;
>> +		}
>> +	}
>> +
>> +	/* Add heap to the list */
>>   	list_add(&heap->list, &heap_list);
>>   	mutex_unlock(&heap_list_lock);
>>   
>>   	return heap;
>>   
>> +err3:
>> +	device_destroy(dma_heap_class, heap->heap_devt);
>>   err2:
>>   	cdev_del(&heap->heap_cdev);
>>   err1:
>> -- 
>> 2.25.1
>>