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;
Date:   Mon, 21 Nov 2022 23:28:13 +0800
From:   Dawei Li <set_pte_at@outlook.com>
To:     sumit.semwal@linaro.org, christian.koenig@amd.com
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
Subject: Re: [PATCH v4] dma-buf: fix racing conflict of dma_heap_add()
Message-ID: <TYCP286MB2323894F9939C27291FD998CCA0A9@TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM>
References: <TYCP286MB2323873BBDF88020781FB986CA3B9@TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <TYCP286MB2323873BBDF88020781FB986CA3B9@TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM>
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
X-MS-PublicTrafficType: Email
X-MS-TrafficTypeDiagnostic: TYCP286MB2323:EE_|OSZP286MB1557:EE_
X-MS-Office365-Filtering-Correlation-Id: 8bdd4859-e0bd-4a7a-ac5a-08dacbd5037c
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: GqoSvLfYXio+NfurrX6DuPkLI264iiky6pW8TDT72WhS29smZPkFMMY9nQ6Js8Nq7LLLJYXGcXvmkVgfha4Awaiw3rOXIz8mw8UY0a5e8n5viR/tpusvEsCLasF69YhMuezOgHOtOQ+aMSZHz1KkoaGYpz0ajSdhgBuUIfFw4Vsl8Teo1zhzT7CsANhMmGLJcQhtaXdByccj5hDUZzCE7LhbBWgEHSsNPswitvR3YtRVI/MdEZhvpQT/i4xWYVJ9HdvMl+GrhppuCO5aBmjQUx6jttqxHNWBIandEpyH0rpWiGwASMapPPNAZsTv1iA0NtmjmvE/oL+M+IXkGpf7nBhEs9jefOvXUwxTTmLBNM/mKESNfsdlI9TH7Cq8mX78Q4DMa/d2JV//ZQBUGumPt7UK11Wks0MEemughLvod/jFYismmwtmxPJIGSZVgUkl5ZD/DVMd8UzAkFCQG6ERU+0v4LuADDimU1TcQkEO9E2DNdSqQVJ9RPJhWcEMu03xkDXJWOmV8thXsV+hnFlYBNsEFeYGfIpBYBeKP94onGnfCu+PVwvw9Kn6ncwZ8/y7Va9oa9TjfUlBwuECMII7+qgt9dPdiXPzLzJk9mdpYkv//mmInoVxdq58t3hb9O6W0YLcOdtFHrT/7wmvcirq2Q==
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?vBpnwVeJMnCyVEzp23LIvfCxQ9la+ozPK451H4PzT5prytczX1hOpXAI8LNp?=
 =?us-ascii?Q?hq7RM4SNqa3ddlx2eKwoN5DU5/ZQSQexItwVZAtx9BXSLCEOY4mcYwGB75WW?=
 =?us-ascii?Q?3n4K9Pptcx7Yp8dQvgKzKEYkNNFPQuTlUpgxlCRqFUDFGlpCMKcsFvJj3tY1?=
 =?us-ascii?Q?eJCm79itu3QHeqmQaSA6OUWDW328IkX6N09nhtGLw+4rCwBiYDOZwkhC0rPm?=
 =?us-ascii?Q?wJ8nPld8YwB9RJptBHj6NDcocKADHKXao/RImLIthniuBjfrWpcsYCS0Qu8f?=
 =?us-ascii?Q?FSX+yHeQ3okcguK7dOE5UPqCmMralAVsiVQNBk5j0F0cvsZ10vIyA8z+CK2F?=
 =?us-ascii?Q?Przkq3RfsoiFq8ZfGnASZ44tCt0QCZVdIXfbmJx9GdlXIi3+QE13x3cFA3w9?=
 =?us-ascii?Q?/73X3WeYy8wYIfd2EXGyqO3r3XMgaqNege21cp7LbqE5RkIEcEheSKYa3bo+?=
 =?us-ascii?Q?t2Y/OLhYbMX5uv1qwoF4xAlzR02VNFOBQRt4rVZKCmw9SyotRCUJ21dUljq7?=
 =?us-ascii?Q?ykYzQ71M+D7l5KP6QEdRECl5No0Qo6WJRiuYgqxIhYm9xv38qMNYLTlefAQW?=
 =?us-ascii?Q?q+k+eadEcwbnPNajXiSVmRReP9fJoA1CsMsaIyKivfpyph3EA1MHFvTuB+gx?=
 =?us-ascii?Q?S01L7/wm7VtWqi07xYOcbawz4yX0jgpAeYy0RoAtwuvEIovgApFdKpTWSvxP?=
 =?us-ascii?Q?pLdArMxbedEjY2bZPIlxOhXeY5JWUdPXOq1deyzp+SmWoslCM8ZiLdbSewCN?=
 =?us-ascii?Q?ugDxyn112kVpSapEA60iPHFgTmbsYRimgXNN4PdG25ekHy0LNe0hIIbC0yqa?=
 =?us-ascii?Q?d6AqR16G/R3H3PFevtIkWiejd28oAE9ks+Ai+Pt+1oiokQ5TDZqloy7mw3iJ?=
 =?us-ascii?Q?6SGoVfk0NX/S13FwHiYe5Dnqo+DlXInis16GEVPDz/OFvOD5PjCOHS6dyxvS?=
 =?us-ascii?Q?adXAXZkNqaM/4zkra1VaiTBPyk5m0jhHH2XtT+XzosDM84JoIsWyYptw2P3N?=
 =?us-ascii?Q?aeOQb0zOHvskmHZoI9fTSfuZGzYKfBqvb/cEbyE7uEUS3dtbftTSxrdXEDZp?=
 =?us-ascii?Q?eiYPLk00LMI0EBnfsCnlSQmW2eDE8jR6PWCaJTZ7s02mXj1B7Az0y5wOOv6Y?=
 =?us-ascii?Q?uAmT7HL4POnakvTaHrwL3mQAwfXLLuSS+AVGRt/BK1h4xI7DW8fz+Ahluh00?=
 =?us-ascii?Q?OUvtBzxQnkMC9f6A52x/ow9bI7PwQs618Kfw38he7oTbAof8EGHh0eJW+d9J?=
 =?us-ascii?Q?c98tr8+J7ZS4gtw7X7ThxgevikZkIycJTTC0Rz/7eQ=3D=3D?=
X-OriginatorOrg: outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8bdd4859-e0bd-4a7a-ac5a-08dacbd5037c
X-MS-Exchange-CrossTenant-AuthSource: TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Nov 2022 15:28:19.4874
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa
X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000
X-MS-Exchange-Transport-CrossTenantHeadersStamped: OSZP286MB1557
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,
        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

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://lore.kernel.org/all/TYCP286MB2323950197F60FC3473123B7CA349@TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM/
> 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
>