Message ID | TYCP286MB2323873BBDF88020781FB986CA3B9@TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp503088wru; Fri, 4 Nov 2022 09:19:45 -0700 (PDT) X-Google-Smtp-Source: AMsMyM4VTD+xEhfLoMHw3tMompK2qSEnkTCJk7HA9yBsn+NF4kjmp642F9MG/yYqNLnnuKI/hUk5 X-Received: by 2002:a63:2a08:0:b0:46a:e2a8:4ead with SMTP id q8-20020a632a08000000b0046ae2a84eadmr31825024pgq.132.1667578785070; Fri, 04 Nov 2022 09:19:45 -0700 (PDT) ARC-Seal: i=2; a=rsa-sha256; t=1667578785; cv=pass; d=google.com; s=arc-20160816; b=tXCPTxU2JFodTm6BXU9IEQ45sZhizK9QvAf5ewCY+plG8+zNXG5tx08upQSRKBH1IR qcpIiCLt5PL7l3jI3TfC+WU5pGbTcIKK+XmlDzx8tT3NrnvqfRJwziRBtbN3ZuXQOjuH aYYXJqqBNF8lmk0lFqmvM9DvwV8k/LZliQt335O/nOPOKzV6Xt3efR70TDu8JRpWXT5d If8TfzeyxXJcWQQNplr+8SYkc9dJQufpf/Vdtk6wQpqf0WF/MPxz8TacjgXjFMTUTtlw fWJ7TukRqjoTrjBnpyGQmGMiUrZwilpnfO/cOdPDKCMyOxesewbQ6BlaDEHp0KD8TFNq WwoQ== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:mime-version:content-transfer-encoding :message-id:date:subject:cc:to:from:dkim-signature; bh=EAausU8rFK0NzdqFnWtnDeOC1n9wuoalCC5vsr0tk60=; b=SDeIsrqdBX0WihhojQR7k8g3UOnbX/XI/qUMNaTmqFrAThnKxyr3jM08lrT/f0/1vX Us+Hs3eqZ9tdBT0iytgLBJEDbzp+uet7+X168hhwtcATd7Mku7PQkwlL+J0KatS6r2MM VqRDly78b72A5wTI98JwNDXUF9FDyZrD13fMGir79JIasL81CmlshYeSWQbtypLuP0Cg QUH1AHGfCQFRKsGHqYdsFkox/6OQeb/H8iEL+9F3hQ3NiwoJCnT7VObQHGpe1hnwMlol qDhiuBbfe1lXzkI8X/KQYeFmFS9YoALMD8QLlKcdi4STU+uIBYA5RLsnvW2VgQ9Muny1 c3YA== ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@outlook.com header.s=selector1 header.b=kpu6IZvy; arc=pass (i=1); spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=outlook.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id l12-20020a056a0016cc00b00544ce272399si5651793pfc.173.2022.11.04.09.19.28; Fri, 04 Nov 2022 09:19:45 -0700 (PDT) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@outlook.com header.s=selector1 header.b=kpu6IZvy; arc=pass (i=1); spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=outlook.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231860AbiKDQGK (ORCPT <rfc822;jimliu8233@gmail.com> + 99 others); Fri, 4 Nov 2022 12:06:10 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39642 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229749AbiKDQGH (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 4 Nov 2022 12:06:07 -0400 Received: from JPN01-OS0-obe.outbound.protection.outlook.com (mail-os0jpn01olkn2054.outbound.protection.outlook.com [40.92.98.54]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1FD8B1BEA3; Fri, 4 Nov 2022 09:06:04 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Bbh6U9XHZsCBZcAEmKFfPicrmoZ3BFtWZ4B/HuoXPzQE4JN4zvISmU/2iG5BHi0I/SSB8IXyl8EhRAS5us2k6xTnHzQtrz4rNYLA0LY3ZsISkZ5djjGlRIs8G/AfQHB3gwjbv7O55bxe+YxGKCYF03eLo3w4/KEum58fMVO8qnemW2GOIZTHz6ZU4BVKUntbEYfQLDrF59GobCKg8wYGRuTkgG3W41CIopZYD1EWT20YBRVpBvRI0dY6FWT7njiOnckAAJ3CuNVbyd3BpVphuurbhOeOTlKg4Q1WXxCzMSkoPa8rB9g56lmfqfgjOleY32w01IzkVYLRholPQr0cgQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=EAausU8rFK0NzdqFnWtnDeOC1n9wuoalCC5vsr0tk60=; b=XRmUT06Zb10OBTGPTbLBJSVUfqc3S97PgJXxy6Qo/OJyVF+pGA5it0DhcrRLtRjAML94skEvg/QI53WJXCTFWnqjtvWg3YjojjDfM895amxFBq6HKmODMIVBGf9JzK+OxDQS/+U5hdsOyn7rOiNeEswzlCFUOkyc0tTyk7dEK44lTGXtRmJW02vIPVoIAvNC6YVc9FcDBHaj7jZVRxYzdt6U37zGWxQnYnZo4BoKFXr9ZAezPwWRioRR2yzfac/PKPkzJ3ywSwrcoFNzMRjWvWAZAx2Lp7K366BFnuYSj5nOUs3zgFI+JqwX6SATiVA5bVejWR/bXQRCgQ6KsPgutw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=outlook.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=EAausU8rFK0NzdqFnWtnDeOC1n9wuoalCC5vsr0tk60=; b=kpu6IZvykMTFZzZP6X/TCqTL6ABOODX3ncl87qO7x+CaZUqKCwxJGn/QLFk11+PHmJ2O0m7jQ9MzKOSaP3sNpIJOqnKe0xr0xCeNfOoMUBX3Rvr/VDuNGGyyX6w1aCeHt4xEB0i7w8b4VCYhKSQRr04UUiisfXzKRNiXgfb+V+oIllDIxfYUu1I3LcG9RjUNoyzipxJ2j9KnVMHayGwISKJ8wwdfN5xdUD3AgX1RuniQhckGPwuwmNlFmYASwSuT1PSAjgppWAAsSsewDxolsFo7zzx21guBusAoThX6rSSIcMleRBDe5J8P5iAFYW50ZB3xoXwqR5HwBGXD37VyQg== Received: from TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM (2603:1096:400:152::9) by TYWP286MB3223.JPNP286.PROD.OUTLOOK.COM (2603:1096:400:2d6::5) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5791.24; Fri, 4 Nov 2022 16:06:00 +0000 Received: from TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM ([fe80::c90e:cbf3:c23d:43a5]) by TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM ([fe80::c90e:cbf3:c23d:43a5%9]) with mapi id 15.20.5791.024; Fri, 4 Nov 2022 16:06:00 +0000 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, Dawei Li <set_pte_at@outlook.com> Subject: [PATCH v4] dma-buf: fix racing conflict of dma_heap_add() Date: Sat, 5 Nov 2022 00:05:36 +0800 Message-ID: <TYCP286MB2323873BBDF88020781FB986CA3B9@TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM> X-Mailer: git-send-email 2.25.1 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-TMN: [+1ZKHlzTD/1HMwxkcwagGc0cFaIxQTFu] X-ClientProxiedBy: TY2PR02CA0041.apcprd02.prod.outlook.com (2603:1096:404:a6::29) To TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM (2603:1096:400:152::9) X-Microsoft-Original-Message-ID: <20221104160536.4123-1-set_pte_at@outlook.com> MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: TYCP286MB2323:EE_|TYWP286MB3223:EE_ X-MS-Office365-Filtering-Correlation-Id: 4522d9fd-dda1-4cb9-4da5-08dabe7e776c X-MS-Exchange-SLBlob-MailProps: hmxDvsT8QC9IPEq7hVSzi8CjtgIfWrB0P2xvIfdtuKc/0tqlgZnU4lfB0vqmy+0SL8Go1P/nWJhrujpcmWhOviI2CQy5eczzf2P6QlNViAm0524ANVYEC6dqDKm6kJ2AxhuAAXi8sBhW7jYXR2BMn4vuab7PL/w4E7XL9CdzT2f3WtvQprRWeukP6A9/0eP9IX2D59tO/tJSyc4ljYdxd0jMSAjOm3KL70dUpfxR0aGNb/z0xxtn7kQSFVZZnzUSRh4BM3JzRgJF5Z06rHY6CEriSsei6U6B/VR9eF5gestn+qOWSBYM3hbVoSTwzY3NkCssF6QS5Y/WGYGTCVMtOW3xjTQ9tRo8pfp+cAIHB1QxyYS4k/GzNThlc6o/EpqB3TUmL8iSrShs5CH3lfOY5pSQUZAQwxfQMWeUrICw/pnfC3aB+9wqhsub/Pp0S9736UOwHQpDeaF8FMJIP41VoCz0fGDvGbsU6G5wnOmElvHfyjfz8Z1t/49VmY7LBtH+qdiMKGvxRVuO0Vxjr37cqKvh5mSM3U7msGy4BslsAhARaKyuVHSfWOrWuFnmh9+o6TiPUA78SsgBjbX+Q5YEzJ/bBlHpHIgUAfNkDPtzzel9bYH219+U4GiAz91R48UDoMiw+T/OHXwUxvMcA1/Fet8F3NL+FmBOG5YyaQje3iTPcIYAJ3irhwkV9nO3kPnAMTySEiN5C3NRE6jBnhpKvbjbf3JaVOtaerMMR+6zjiI9I9nvaPvNbFqDY0+ecDISfOgLVHveGXA0x766KJafj4DoQXnazHrDDgeDLo+zuRjbzZDoqVAADg== X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: nu7IBIHOlDPhMuaYrdBnvYVRRF0dfixRQuPzejFZIGmybqC9VP66uAHfos7T6MC6Qhnuzou4CoXKofwA3V09fk2XI4kZ9B7Dll8Ou1aOzFIkdTO1P0G9ZLJtNSBTQsVP067Vy7bNHlgP8f2BdQUKMfBOdJiMf0VdkYO1NJbil1dCJGpTRUBpUPVw/btiGNkxNs+d0RZlcAhHEB8/5aP5a7QjO/XcHRnSLqLkC9Ab5IT43/3LVDUAdOL5rvvqfeLZ1XbWA3VxQOtTnQF6HJA71HpxVG9Ei6r/94wk4qeivp4AIMK0rO9jXnWFMwGA1lmbyHjxLRT/0cmDYi09Pg/pnXxxeKhYI6tuTWhi/sdu5uU/d6CM+tbqFO++b6oWGf3AUdRMzMGiT1peaEijDHEP7nD6ztuGD36Bb15cDxul4tL8l8K2YtmKVXHc0vrGPtCLi1qLJUxx7eBf8mOcVg5xhJTv2EdtXZdULozpOVvY7jgU8tGTRP+NzP3X+DEHZ2XzvFB8fw+xjg7fddHRzqResYYJyRTd5FSOR+DRfseHP3ZFL2VfRaeN7b/iR9sR09DMY52mZBmcmaXrReYBlL+13ZAxscjJr6Pz2/0IpZeiy5FrZ6wpTlPPOINzlGIg8c7r22VDocqZ7yraUHk7d+zherd5sRCasIbDoHI8C76XgeuUgFMQch66nzpb4lULetZP X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: ImzAWx0EIQC9SbCxcmZTO3K7PeKDTSWsUrClVGni2cSBGE3GBYoy+M5R31SnRRxzypqSm+UjCmNA0+M7fui9ySQFqyCgWBF853lhP9wZbwMivzSL5j2eXzpIb13NOcYMuTXL3TJy7lbPnoGvTmT13V7MbU0/LS/x+Cmf9nDzu8/pzFTfF77+hElAP70uef6Iw9hU4zFlAXT8IYrIINWXivXDeM6rulMqc172mlCnmts8dGi4JWIwsY0sqk8aD+cjMBQ40tsbshZoy3VoVqClHzNRcHXw7tyYD+7TBNHI0Rj6UXDMtP57nhWpYv+Knqs9WQshm/DkzVMHBzf58KzjTN1VOoXyMesd6As3oJH8pG+b5hIppMCM8Ns1FChhOQ/UsgWvj341uwJxoBT6KJ0rFjQ1VFshugHHrNZleTYSgAXwpVa+nW702C0zCctNnvlcjwUyWEfukn8+EX8oj3Lf7lsiq0yr5sWyRruKdiUfqv46fseXtdEP0WeYecCqxmCxDxY0hfJFonipMRCqpG5Z/g+E+IlmeFSg4kFPq8XzOZaTzdQGwGExxSZMSgE/yJDSvyFEbl0ioNGsdkBpuLhRCIUX25m994dZV3gSV60uVf0kLJKNLFC72KiFA/CaZ6BiKl2sa/z5GvJbQtqcPT57UmM7PJHuY4Mir1VfgTgf3PE9psEWCyhfjbY6UHTCI3hqXwnIV4BKBJ7HH+E1AGerJR6Pbd1LdNkSqJyspWzD1o8zqyrfdqEle/B0pWcyO+4L8ACW98eqT3jeMBu20tGD6ar4Wx5tKucVPQ5Ke76oA8lUdPkuAz38fYlsU7OqBsiqfxvhQWqdm1J/uoVwPGo1cmM1xeWlKQ7vUGJZIXneVR/XQEtaPWTOwIQuBJIZsP8M9tfKw+aYi2oZdcrw57bWCyTwSTzgjFbqoFBwHNeYVDOpn2QhtyAtd/kkt2E9pEX//7nTN/DDqAzvLVWKWQxOOK4tSqem+lJJPmU4y50YiTt3XKRIWX5mK7eKAJvw0Cmgf0kATUL2o/FgsM2Iow/vYt5BmBEz0JZcFTwR7IQh4FDG8fXeRjJnztDNqYE8FCKndlf+kMdfD6LdMIFWI+Ozw1XGFIPSwdVnoZtBNFy8d2aPhODAE45JTeGPIG1UoknJBj4zxafkqGAoOb6TJGYYCVywoM9tPiBOkyrZVO8NrQrxDQBdHwo7wLQowCYbznHMqRreVabdA84rLdh57v08HGEEWpNw7wL9dWJQCFPxClJw/kDMulpcX9w/YVDvB1XOm9Gh1PIH8nlCFJBVtN1Ajw== X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 4522d9fd-dda1-4cb9-4da5-08dabe7e776c X-MS-Exchange-CrossTenant-AuthSource: TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Nov 2022 16:06:00.7589 (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: TYWP286MB3223 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,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 X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1748583092334291048?= X-GMAIL-MSGID: =?utf-8?q?1748583092334291048?= |
Series |
[v4] dma-buf: fix racing conflict of dma_heap_add()
|
|
Commit Message
Dawei Li
Nov. 4, 2022, 4:05 p.m. UTC
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(-)
Comments
On 11/4/22 11:05 AM, Dawei Li wrote: > 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> LGTM, Thanks, Acked-by: Andrew Davis <afd@ti.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:
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 >
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&data=05%7C01%7Cchristian.koenig%40amd.com%7C1989f31257fc458a27c508dacbd5078e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638046413707443203%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=OWPUPrIHGnzwXyQE4WlIthThwSuSK2y3Eq2Wq5zAzbo%3D&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 >>
Hi Dawei Li, On Mon, 21 Nov 2022 at 23:53, Christian König <christian.koenig@amd.com> wrote: > > 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> Looks good to me as well. I will apply it over on drm-misc. Best, Sumit. > >> --- > >> v1: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2FTYCP286MB2323950197F60FC3473123B7CA349%40TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM%2F&data=05%7C01%7Cchristian.koenig%40amd.com%7C1989f31257fc458a27c508dacbd5078e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638046413707443203%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=OWPUPrIHGnzwXyQE4WlIthThwSuSK2y3Eq2Wq5zAzbo%3D&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 > >> >
On Mon, Nov 21, 2022 at 10:24 AM Christian König <christian.koenig@amd.com> wrote: > > 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? No objection from me. Thanks Dawei for submitting this improvement! Acked-by: John Stultz <jstultz@google.com> thanks -john
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: