From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f48.google.com (mail-wr1-f48.google.com [209.85.221.48]) by mail19.linbit.com (LINBIT Mail Daemon) with ESMTP id 02C6A4205B8 for ; Tue, 15 Nov 2022 14:16:47 +0100 (CET) Received: by mail-wr1-f48.google.com with SMTP id z14so24053194wrn.7 for ; Tue, 15 Nov 2022 05:16:47 -0800 (PST) Date: Tue, 15 Nov 2022 16:16:43 +0300 From: Dan Carpenter To: Philipp Reisner Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Cc: Jens Axboe , kernel-janitors@vger.kernel.org, Andreas Gruenbacher , linux-block@vger.kernel.org, Lars Ellenberg , drbd-dev@lists.linbit.com Subject: [Drbd-dev] [PATCH] drbd: use after free in drbd_create_device() List-Id: "*Coordination* of development, patches, contributions -- *Questions* \(even to developers\) go to drbd-user, please." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , The drbd_destroy_connection() frees the "connection" so use the _safe() iterator to prevent a use after free. Fixes: b6f85ef9538b ("drbd: Iterate over all connections") Signed-off-by: Dan Carpenter --- Smatch assumes that kref_put() generally calls the free function so it gets very confused by drbd_delete_device() which calls: kref_put(&device->kref, drbd_destroy_device); Four times in a row. (Smatch has some checking for incremented reference counts but even there it assumes that people are going to hold one reference and not four). drivers/block/drbd/drbd_main.c:2831 drbd_delete_device() error: dereferencing freed memory 'device' drivers/block/drbd/drbd_main.c:2833 drbd_delete_device() warn: passing freed memory 'device' drivers/block/drbd/drbd_main.c:2835 drbd_delete_device() error: dereferencing freed memory 'device' The drbd_adm_get_status_all() function makes me itch as well. It seems like we drop a reference and then take it again? drivers/block/drbd/drbd_nl.c:4019 drbd_adm_get_status_all() warn: 'resource' was already freed. drivers/block/drbd/drbd_nl.c:4021 drbd_adm_get_status_all() warn: 'resource' was already freed. drivers/block/drbd/drbd_main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c index f3e4db16fd07..8532b839a343 100644 --- a/drivers/block/drbd/drbd_main.c +++ b/drivers/block/drbd/drbd_main.c @@ -2672,7 +2672,7 @@ static int init_submitter(struct drbd_device *device) enum drbd_ret_code drbd_create_device(struct drbd_config_context *adm_ctx, unsigned int minor) { struct drbd_resource *resource = adm_ctx->resource; - struct drbd_connection *connection; + struct drbd_connection *connection, *n; struct drbd_device *device; struct drbd_peer_device *peer_device, *tmp_peer_device; struct gendisk *disk; @@ -2789,7 +2789,7 @@ enum drbd_ret_code drbd_create_device(struct drbd_config_context *adm_ctx, unsig return NO_ERROR; out_idr_remove_from_resource: - for_each_connection(connection, resource) { + for_each_connection_safe(connection, n, resource) { peer_device = idr_remove(&connection->peer_devices, vnr); if (peer_device) kref_put(&connection->kref, drbd_destroy_connection); -- 2.35.1