From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-yx1-f66.google.com (mail-yx1-f66.google.com [74.125.224.66]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 3654C284B25 for ; Tue, 2 Dec 2025 21:03:35 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=74.125.224.66 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764709418; cv=none; b=ACNc8PsP2NQ/E8L9B5DUJZkvtVYCyY4at68OQqfV0+7EL9fcTbsxHhtxuufed6LlUJAW6NKMSAQwQTGogeNYdN9TzslMgHk5zdWKHGsWFYf6vSwmtf0QwzkUedDE1JUQQ3vFlAikzy/JfyAEO6F6LLwBD25jW77wUGsUjeMQmjA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764709418; c=relaxed/simple; bh=nsE1dcVItFIrKkgY5iEvi5+Y+OLRs1VB0cezp8E3BWA=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=out3nxdf4oL4xvEq8k7kq2aFbBnqJijWZ2CM/TPYsdC5RPiAaaWl/3LHWIQSW9zl9h5AAmUDGSVEBBk+zpe7qfYh6h0IJKwsfVMxIDEdlqG4RWNdHv4vRtjpxfIzOExD4fYDKYlv26PQpmG+qyOQQrO4ZFHhhVivV+UKqWN0Cm8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=FciMMJ/v; arc=none smtp.client-ip=74.125.224.66 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="FciMMJ/v" Received: by mail-yx1-f66.google.com with SMTP id 956f58d0204a3-640f2c9ccbdso5053410d50.1 for ; Tue, 02 Dec 2025 13:03:35 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1764709414; x=1765314214; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=qHqo10RPrXoBjpQijgSJaVgvodoRgdMooXQI8Kn+rbo=; b=FciMMJ/vWs8rZZUTcn2Q5p8fcXMLyY+pTIl14tfOZvXrfunvsA3Qdfl1cUncqp53t4 jRc8k/V7IEkTFDq4VNDzGjj+UG2EOI92etFkvb97eOPzImeVK1gRJrOD9ud2J0RlirK9 kQDqk8GXJ4sswx+gI7sYrJejGhr80ISKRcvwjmI1umN2PDYVkJHPdyqLo4fKQd5TWfiN DQ1LK7hLVp4bTfCpnJYitP73BD62Thf+jkaeecjL1wPRLy0WM2w35dtrcdexJeKzPohx RSu47oR3U9SA9eiVwfHPLCPgtQI//IwbgUjFKd5cXxN9zSIqRxCrrFyf5kk3MQh5JDdc 5a1g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1764709414; x=1765314214; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-gg:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=qHqo10RPrXoBjpQijgSJaVgvodoRgdMooXQI8Kn+rbo=; b=w6181sHll47QmwHO2J5eTQKU7ff2kUWEPYzcREZNiL4KX1s7Y9U58z4NEZ7tc5qa3o 9Ws8A4KpH2G8Gdq/zlY1lqW/2yhWiPxAQisOPmrlKk21OYR80R0o/EDuTIgBysYS1BqY YOY0S/SOrDIlz0LGc5iJwE7QBL3rWqpI6MxZvVA/9U4wJHJS5fgg9B2kXEKf3Cs9IIAT R1pFX1z5/D4euq4yUYIKBFTwOhIwR/2ZK2EgExo3caK/xE9X1JtOnO675MEfABE1YxxG DCDVwBTiOyMuUQtsCzw/ohlLP8wqqLw9VOs5zFzO0CbP3vhVz/I9ItsXw5HBvnGpOIhS flnQ== X-Forwarded-Encrypted: i=1; AJvYcCX3wLh66OMY/9IA2+yLL00VpAa7qnzqe4cI4QvqmSxnQTnixd1rn/c7/MFNsymcy6vb/Bcah5Xf0e9GbQM=@vger.kernel.org X-Gm-Message-State: AOJu0Yz87qHmhybT1XNGo4FopkKYqdy3rIW/GyND8MjnLFQGGbrsnonW MUqKN7f67i21SGGHTdu7CDIQgdDnTqVVW38Mv055wB50v8w+PMK7KakQ X-Gm-Gg: ASbGnctq7+FJbT3dnW6cqhR8MQXlzGxYdpNNPGDqy6nREvJeBXS5Iu7tnN8LIBFASTS SFAfQPE8umQ6BP/sombQzg2dWJXj0EBxg3NIdpv9ARDQauW+GYPDUCz7wW/JmzHcMjnM+LYwoch Gh0XHM34jnl2PzPAYLe9DT0/X7R6WRRtZaufBecr1pdY2yCNMJgiaG7cgTdgd/Vi7JjzOcV57GE 40y4BRCjqhNRJQTUUEUN/NU4PTrRPD4jq/dPPVslmIpkvUWQcmvur0chlWxpGyuHyn/PLVAqHTV QHBzXn7RUYjctuB/FKUj9hychxXQyDnreR/+MiIWS4B28FqSBPV8AdgaEPOkWVAw7ZfrcdrfGej 5bPpPp3tw11RZ3wFGrmxYc8lMt8IkpS3bRuY8wYbTpbtn5qbL3/msF4+AZjrhkI+lygc42LhxaC 1ZodDYdbJ8fju3uUYkUg== X-Google-Smtp-Source: AGHT+IEyRfhMsBmjvsOtgMQ3MW/jmJCsdUgV3EbIDt1I1mGrcnMfQfROj52ogueWWJwWVhxsmp5c7Q== X-Received: by 2002:a05:690e:1909:b0:63f:860f:feda with SMTP id 956f58d0204a3-644370439d3mr114471d50.63.1764709414045; Tue, 02 Dec 2025 13:03:34 -0800 (PST) Received: from localhost ([2a03:2880:25ff:42::]) by smtp.gmail.com with ESMTPSA id 00721157ae682-78ad0d3f5f3sm66735887b3.8.2025.12.02.13.03.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 02 Dec 2025 13:03:33 -0800 (PST) From: Leo Martins To: Filipe Manana Cc: Oliver Sang , oe-lkp@lists.linux.dev, lkp@intel.com, linux-kernel@vger.kernel.org, David Sterba , linux-btrfs@vger.kernel.org Subject: Re: [linus:master] [btrfs] e8513c012d: addition_on#;use-after-free Date: Tue, 2 Dec 2025 13:03:25 -0800 Message-ID: <20251202210330.2705156-1-loemra.dev@gmail.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: References: Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Tue, 2 Dec 2025 19:19:05 +0000 Filipe Manana wrote: > On Tue, Dec 2, 2025 at 5:17 PM Leo Martins wrote: > > > > On Tue, 2 Dec 2025 15:04:51 +0000 Filipe Manana wrote: > > > > > On Tue, Dec 2, 2025 at 8:40 AM Oliver Sang wrote: > > > > > > > > hi, Leo Martins, > > > > > > > > On Mon, Dec 01, 2025 at 04:51:41PM -0800, Leo Martins wrote: > > > > > > > > [...] > > > > > > > > > > > > > > Hello, > > > > > > > > > > I believe I have identified the root cause of the warning. > > > > > However, I'm having some troubles running the reproducer as I > > > > > haven't setup lkp-tests yet. Could you test the patch below > > > > > against your reproducer to see if it fixes the issue? > > > > > > > > we confirmed your patch fixed the issues we reported in origial report. thanks! > > > > > > > > Tested-by: kernel test robot > > > > > > > > > > > > > > ---8<--- > > > > > > > > > > [PATCH] btrfs: fix use-after-free in btrfs_get_or_create_delayed_node > > > > > > > > > > Previously, btrfs_get_or_create_delayed_node sets the delayed_node's > > > > > refcount before acquiring the root->delayed_nodes lock. > > > > > Commit e8513c012de7 ("btrfs: implement ref_tracker for delayed_nodes") > > > > > moves refcount_set inside the critical section which means > > > > > there is no longer a memory barrier between setting the refcount and > > > > > setting btrfs_inode->delayed_node = node. > > > > > > > > > > This allows btrfs_get_or_create_delayed_node to set > > > > > btrfs_inode->delayed_node before setting the refcount. > > > > > A different thread is then able to read and increase the refcount > > > > > of btrfs_inode->delayed_node leading to a refcounting bug and > > > > > a use-after-free warning. > > > > > > > > > > The fix is to move refcount_set back to where it was to take > > > > > advantage of the implicit memory barrier provided by lock > > > > > acquisition. > > > > > > > > > > Fixes: e8513c012de7 ("btrfs: implement ref_tracker for delayed_nodes") > > > > > Reported-by: kernel test robot > > > > > Closes: https://lore.kernel.org/oe-lkp/202511262228.6dda231e-lkp@intel.com > > > > > Signed-off-by: Leo Martins > > > > > --- > > > > > fs/btrfs/delayed-inode.c | 34 ++++++++++++++++++---------------- > > > > > 1 file changed, 18 insertions(+), 16 deletions(-) > > > > > > > > > > diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c > > > > > index 364814642a91..f61f10000e33 100644 > > > > > --- a/fs/btrfs/delayed-inode.c > > > > > +++ b/fs/btrfs/delayed-inode.c > > > > > @@ -152,37 +152,39 @@ static struct btrfs_delayed_node *btrfs_get_or_create_delayed_node( > > > > > return ERR_PTR(-ENOMEM); > > > > > btrfs_init_delayed_node(node, root, ino); > > > > > > > > > > + /* Cached in the inode and can be accessed. */ > > > > > + refcount_set(&node->refs, 2); > > > > > + btrfs_delayed_node_ref_tracker_alloc(node, tracker, GFP_ATOMIC); > > > > > + btrfs_delayed_node_ref_tracker_alloc(node, &node->inode_cache_tracker, GFP_ATOMIC); > > > > > + > > > > > /* Allocate and reserve the slot, from now it can return a NULL from xa_load(). */ > > > > > ret = xa_reserve(&root->delayed_nodes, ino, GFP_NOFS); > > > > > - if (ret == -ENOMEM) { > > > > > - btrfs_delayed_node_ref_tracker_dir_exit(node); > > > > > - kmem_cache_free(delayed_node_cache, node); > > > > > - return ERR_PTR(-ENOMEM); > > > > > - } > > > > > + if (ret == -ENOMEM) > > > > > + goto cleanup; > > > > > + > > > > > xa_lock(&root->delayed_nodes); > > > > > ptr = xa_load(&root->delayed_nodes, ino); > > > > > if (ptr) { > > > > > /* Somebody inserted it, go back and read it. */ > > > > > xa_unlock(&root->delayed_nodes); > > > > > - btrfs_delayed_node_ref_tracker_dir_exit(node); > > > > > - kmem_cache_free(delayed_node_cache, node); > > > > > - node = NULL; > > > > > - goto again; > > > > > + goto cleanup; > > > > > } > > > > > ptr = __xa_store(&root->delayed_nodes, ino, node, GFP_ATOMIC); > > > > > ASSERT(xa_err(ptr) != -EINVAL); > > > > > ASSERT(xa_err(ptr) != -ENOMEM); > > > > > ASSERT(ptr == NULL); > > > > > - > > > > > - /* Cached in the inode and can be accessed. */ > > > > > - refcount_set(&node->refs, 2); > > > > > - btrfs_delayed_node_ref_tracker_alloc(node, tracker, GFP_ATOMIC); > > > > > - btrfs_delayed_node_ref_tracker_alloc(node, &node->inode_cache_tracker, GFP_ATOMIC); > > > > > - > > > > > - btrfs_inode->delayed_node = node; > > > > > + WRITE_ONCE(btrfs_inode->delayed_node, node); > > > > > > Why the WRITE_ONCE() change? > > > > Since there are lockless readers of btrfs_inode->delayed_node all writers > > should be marked with WRITE_ONCE to force the compiler to store atomically. > > If by atomically you mean to avoid store/load tearing, then using the > _ONCE() macros won't do anything because we are dealing with pointers. > This has been discussed in the past, see: > > https://lore.kernel.org/linux-btrfs/cover.1715951291.git.fdmanana@suse.com/ That is what I meant. Missed that discussion, thanks for the link. I do still see some value in using WRITE_ONCE which is for the human reader to realize that there are lockless readers, but that's pretty minor. > > > > > > > > > Can you explain in the changelog why it's being introduced? > > > This seems unrelated and it was not there before the commit mentioned > > > in the Fixes tag. > > > > I'll send out a v2 without the WRITE_ONCE since it is not directly related > > to this bug and send out a separate patch updating writes to use WRITE_ONCE. > > > > Thanks. > > > > > > > > Thanks. > > > > > > > > xa_unlock(&root->delayed_nodes); > > > > > > > > > > return node; > > > > > +cleanup: > > > > > + btrfs_delayed_node_ref_tracker_free(node, tracker); > > > > > + btrfs_delayed_node_ref_tracker_free(node, &node->inode_cache_tracker); > > > > > + btrfs_delayed_node_ref_tracker_dir_exit(node); > > > > > + kmem_cache_free(delayed_node_cache, node); > > > > > + if (ret) > > > > > + return ERR_PTR(ret); > > > > > + goto again; > > > > > } > > > > > > > > > > /* > > > > > -- > > > > > 2.47.3 > > > > > >