Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Date:   Thu, 26 Oct 2023 20:46:33 -0700
From:   Dan Williams <dan.j.williams@intel.com>
To:     Robert Richter <rrichter@amd.com>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Jonathan Cameron <jonathan.cameron@huawei.com>,
        Dave Jiang <dave.jiang@intel.com>,
        Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        Ira Weiny <ira.weiny@intel.com>,
        Ben Widawsky <bwidawsk@kernel.org>,
        Dan Williams <dan.j.williams@intel.com>
CC:     <linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Terry Bowman <terry.bowman@amd.com>,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>
Subject: RE: [PATCH v12 01/20] cxl/port: Fix release of RCD endpoints
Message-ID: <653b3299c1a33_244c8f29449@dwillia2-xfh.jf.intel.com.notmuch>
References: <20231018171713.1883517-1-rrichter@amd.com>
 <20231018171713.1883517-2-rrichter@amd.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20231018171713.1883517-2-rrichter@amd.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?/X0dX3TezvQs5Nb8ZCNTCw8DDJz84LcWiD+nZ+WIVME1HTgBcmSckDE94trD?=
 =?us-ascii?Q?DVXasQTf/EszPBKm9E32vcIROZXhDEjCBlsaV8OyQMGmaO+GnmBVqof/hsgQ?=
 =?us-ascii?Q?l3Bu4UkiG6B51UVp8F3OIAs4u6Zly5JyPkOV20JkVC95BrDCfeXIL38H5QEp?=
 =?us-ascii?Q?7K13UTsjebShFmurQprsPdypaXKhfIL+UfIMHpmfiimR5NSGpfo6QTIw6HyE?=
 =?us-ascii?Q?tK0AR5r1BEdDTwqx0RoFM3824LJJn62TSunaB/6kXGZFjOMLz7789ymet0ye?=
 =?us-ascii?Q?BNUHpeBlKrEBIyJH2MYWJhJxESw0i44htlaJ4V48pgsnP4iv84XMeV3muONJ?=
 =?us-ascii?Q?dibivIe1WmkDtSOF8jke9kORERstT2To1ehAoxxbi04I9rS5yFpGUQfh+WqC?=
 =?us-ascii?Q?quEg5NO3stOhqiNF/DA6i6amsOGtefrshM0LfquXhbP02i86TEILmKTU2pZj?=
 =?us-ascii?Q?W1CQOeoAOPulVrDhgROOOy9y5BTPR+4QdmlDKQ6YAtpkDpY1ZOrNZLzjadic?=
 =?us-ascii?Q?0x1sZr03yqGJkWmRnOZ8XNZL3J+Zt6vqJaqXKlaNPE6wZG5/VLUgFDv6S62h?=
 =?us-ascii?Q?pMwqwFinPOUqhuOWz9Om/VY5j1WBo5bjXDY2P2L4P7IMnVjaNPJgcpDRjrcS?=
 =?us-ascii?Q?s1GRyPskb+ooviQoS232mRkcJqkjz+BdTkT/VDktf7TV5EpyRAlt0850zPYq?=
 =?us-ascii?Q?HVkuUrp7ps3SQkWZrskANaMfmCt6ybPpEJ7oOjz5faEHhOIOcbgdT8O47bNA?=
 =?us-ascii?Q?pfL9KhfTcaT/+vGLhgJHMp6Xg6HqI0rrU1N36O4UTFU7d20unsjaHfUNuusB?=
 =?us-ascii?Q?nYovL9mDDy33DUCHP5AA9P3pWUyIQPzMRXK/hlMNAy0sl1ZBWElNE0lAtH98?=
 =?us-ascii?Q?fB3IreWerKA5FTsWt7mex3C4xTPGXtcXU5zlV2O9zFbfvuAX+Fe2WzN3leO+?=
 =?us-ascii?Q?YnJOuvJdy6qaDy+4kiM6YCh7QZL30aKykYa6IGSh61sb4igWeJFifaZOLvGm?=
 =?us-ascii?Q?gTKwOLZhu6lVCzhyzNFFEtVtNO5LEdn5YlkeTfNXT7c6m1re9vZJ8iRQ3UH/?=
 =?us-ascii?Q?HoC57PT3zcFwy5MZllZA0XOoDs9fknSQIQ4f1yh+CvgXTj6rofShaNr2Di0t?=
 =?us-ascii?Q?Hrj3qs7xUk0ZnDJ5akQBKF20h7xCcnul51XR/YFv2zEGvVzdknwFHPyq5Jy5?=
 =?us-ascii?Q?SsipMD7+Cp+cLrKsn2DM7KkDucO+yz9pK4wORCpOrTCIwkquP2mBKDhxybUC?=
 =?us-ascii?Q?ZbRA1bDBlXUAXNTT9qnE7CFoDBgGFVIDVH5WBQdzFid5Oukn0gFBI8qw0A+P?=
 =?us-ascii?Q?Y2D8nRoUdxwGTUcjWtWoiMvQEKAR0/6vOqj9KTenYWdyDOszXy1bcF+aEgVt?=
 =?us-ascii?Q?xh58tQxVZkyI1klVFF3wOaPfv/VyOrWHu2eDEA28lsXsKptj18Yenk7dAkiY?=
 =?us-ascii?Q?7p66CgTeLGZoIivYz82X3L4X1XdmVgI1siPUT4/bUv0aUtqO1IuIQD9avZgh?=
 =?us-ascii?Q?RVfYcqTycXTONM/Rf8SbXZK//YflZkn5EVJ296hnOdRVjtN4fq1sKBAFhzJU?=
 =?us-ascii?Q?NBP/xuCoRKtyD5d119OyBmbb3GqBc1yRKi97DypyqysWjqttYuj6JxBLWaK4?=
 =?us-ascii?Q?iw=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 28aa363e-c03d-46f8-d037-08dbd69f518b
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Oct 2023 03:46:36.1802
 (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: VKGbi9rXntUiogHwFOg8tCEhKDNBwXnkRlomICoOrnRbmffUtfYS8ezFs5iDXO87W/rNi6wZey4Ux/G06DbPQSS1HE0j1roQthu0X1hroGo=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB5970
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,
        RCVD_IN_DNSWL_BLOCKED,SPF_HELO_NONE,SPF_NONE,URIBL_BLOCKED
        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-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Thu, 26 Oct 2023 20:47:00 -0700 (PDT)

Robert Richter wrote:
> Binding and unbindind RCD endpoints (e.g. mem0 device) caused the
> corresponding endpoint not being released. Reason for that is the
> wrong port discovered for RCD endpoints. See cxl_mem_probe() for
> proper endpoint parent detection. Fix delete_endpoint() respectively.
> 
> Fixes: 0a19bfc8de93 ("cxl/port: Add RCD endpoint port enumeration")
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

This patch causes cxl-topology.sh to crash with use-after-free
signatures. I notice that this path should just be using
endpoint->dev.parent and is missing reference counting on the parent.

I will replace this path with the following:

-- >8 --
Subject: cxl/port: Fix delete_endpoint() vs parent unregistration race

From: Dan Williams <dan.j.williams@intel.com>

The CXL subsystem, at cxl_mem ->probe() time, establishes a lineage of
ports (struct cxl_port objects) between an endpoint and the root of a
CXL topology. Each port including the endpoint port is attached to the
cxl_port driver.

Given that setup it follows that when either any port in that lineage
goes through a cxl_port ->remove() event, or the memdev goes through a
cxl_mem ->remove() event. The hierarchy below the removed port, or the
entire hierarchy if the memdev is removed needs to come down.

The delete_endpoint() callback is careful to check whether it is being
called to tear down the hierarchy, or if it is only being called to
teardown the memdev because an ancestor port is going through
->remove().

That care needs to take the device_lock() of the endpoint's parent.
Which requires 2 bugs to be fixed:

1/ A reference on the parent is needed to prevent use-after-free
   scenarios like this signature:

    BUG: spinlock bad magic on CPU#0, kworker/u56:0/11
    Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS edk2-20230524-3.fc38 05/24/2023
    Workqueue: cxl_port detach_memdev [cxl_core]
    RIP: 0010:spin_bug+0x65/0xa0
    Call Trace:
      do_raw_spin_lock+0x69/0xa0
     __mutex_lock+0x695/0xb80
     delete_endpoint+0xad/0x150 [cxl_core]
     devres_release_all+0xb8/0x110
     device_unbind_cleanup+0xe/0x70
     device_release_driver_internal+0x1d2/0x210
     detach_memdev+0x15/0x20 [cxl_core]
     process_one_work+0x1e3/0x4c0
     worker_thread+0x1dd/0x3d0

2/ In the case of RCH topologies, the parent device that needs to be
   locked is not always @port->dev as returned by cxl_mem_find_port(), use
   endpoint->dev.parent instead.

Fixes: 8dd2bc0f8e02 ("cxl/mem: Add the cxl_mem driver")
Cc: <stable@vger.kernel.org>
Reported-by: Robert Richter <rrichter@amd.com>
Closes: http://lore.kernel.org/r/20231018171713.1883517-2-rrichter@amd.com
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/port.c |   13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 5ba606c6e03f..6bbaca5ac60d 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1227,13 +1227,7 @@ static void delete_endpoint(void *data)
 {
 	struct cxl_memdev *cxlmd = data;
 	struct cxl_port *endpoint = cxlmd->endpoint;
-	struct cxl_port *parent_port;
-	struct device *parent;
-
-	parent_port = cxl_mem_find_port(cxlmd, NULL);
-	if (!parent_port)
-		goto out;
-	parent = &parent_port->dev;
+	struct device *parent = endpoint->dev.parent;
 
 	device_lock(parent);
 	if (parent->driver && !endpoint->dead) {
@@ -1243,15 +1237,16 @@ static void delete_endpoint(void *data)
 	}
 	cxlmd->endpoint = NULL;
 	device_unlock(parent);
-	put_device(parent);
-out:
 	put_device(&endpoint->dev);
+	put_device(parent);
 }
 
 int cxl_endpoint_autoremove(struct cxl_memdev *cxlmd, struct cxl_port *endpoint)
 {
+	struct device *parent = endpoint->dev.parent;
 	struct device *dev = &cxlmd->dev;
 
+	get_device(parent);
 	get_device(&endpoint->dev);
 	cxlmd->endpoint = endpoint;
 	cxlmd->depth = endpoint->depth;