From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f172.google.com (mail-pl1-f172.google.com [209.85.214.172]) (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 1B28D1917FB for ; Thu, 4 Jun 2026 18:04:24 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.214.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780596266; cv=none; b=FM/6uDx32ty2vC3MQuwB6VGaSZ+4UzC8NLxNBM1S3Uc7oBBdCucW58ya0vicWVE8747Xd2ohtjTQGYgBU1IabT7xIRfnNQuUSCShVIejdPQsRbe9G4jZ+ujf3/xmtvXDhvYjC6QIDBLekv3v4yzC5Z0aixOOdRzI4Vs2eXbdDvo= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780596266; c=relaxed/simple; bh=qU4p0nfLvtyHzhUKU+F6fvh/nUsV7OGJRHaoh2iJQJo=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=Wj0XKXbuTWXSMj0/eqMvFHw/EFJqPfVT42rkNv5Wx5WckvtsBKSxTXOE1GnqF3LGeVCX1E463FR5+jxCBpr1q4KtI+deJUbS62Fcr1ByaJVMOlMBBBvg2VnUk9sYzsxckCES2efnJOB0Yiq6CWHZLVBq8TmSz20/9PC193YDyqg= 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=Gr9dl5yC; arc=none smtp.client-ip=209.85.214.172 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="Gr9dl5yC" Received: by mail-pl1-f172.google.com with SMTP id d9443c01a7336-2c0c32f6ce1so7469875ad.2 for ; Thu, 04 Jun 2026 11:04:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1780596263; x=1781201063; 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=1qwelWfl3EltZO++bcisuE3FN0OidgHsQxpE0HCuQEc=; b=Gr9dl5yCk1ufFo9qbOmF48Mk5wM6tvyNxoAjnj1jhcL/31dkWkRWy4xTI+9WpAwfAk bnz7InYfSg/WkHw1CNqrhkRfAZ9PmJqZynxvGgNe9RaVIGRnQXK0p3hQwlRDIfxXLAxs Dw+v3Gu6whEO7m4hS7VGU1XlBg2MqenoSe8behfRaggPtEcytNlVGSQfz3t8nOdX6q7+ 5IDo4voj8sbg1b3fwEXg9xt0sHzmAylXTpSzH4KHHPFfLRvQGdKQSLI7s395ANaEQH0r 40/MDnRlMW6Uo2xJFzTuhPeXAyqFsGtGuNJyd/kIgqSqc0gU1hyLLReURnyh+rVgkDSC z8zg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1780596263; x=1781201063; 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=1qwelWfl3EltZO++bcisuE3FN0OidgHsQxpE0HCuQEc=; b=I0iJq8U7XB56T/rRzH1KdkWQdFwgP/mr+2rHsFuuevTlS+wgMWbGaM1eUy0KACcIZM PbuctE+Zu+0OTbvFDvQ8Z403B/od42BUyLlvOog8eIRo4D8iAtm7WXzupqwo9mCLLEQV bU9FV0mKNmjPb529UU9g2uYy+ZHxTT0X/wM0cRrXSl1EwtCDT9a8mJ3DOK3uBm5DmR/n fLcaDwBbC5BmVHOp8Gp//TScMQOND7/gtcqi8y/urr+QOjSAmb0nn3Go2x/OjgF6/uR7 l/gyYBdMAeIxO/egpfwVHmYGrVMm7VibHpIy/MaR9WQql9SFlF3cIglCQgApeEHjize5 FGng== X-Forwarded-Encrypted: i=1; AFNElJ8B85cTb6TEo1ogT2KVWdKMa10eJoX4d4jQ4mRqGPQwS/pGG/LxYYXCH/uDb+ZN0HwG7LuUZ16bRONcQa4=@vger.kernel.org X-Gm-Message-State: AOJu0YzxbtYYSooVhc4GxicxWgIWSmXPhAbKnjgkAOCTl817iOqOj6Wo qsTJc/AtXW4l0quZhEnNebJ9oBxUqN+r0TJAzjQNsvRPjKvzsjm7XBBqCSnBvSeDSBobdg== X-Gm-Gg: Acq92OGrc4IP7lZIhQNAfIc6KgBtnOTNTnOnbVsWHTMvM+OVDPRAQ/ZBMCjeR9fSMAK rYrxOtOFYrRv4udg0wHx4FqBPwexM4UAmGomPZW77x41FWvdvts9EqRRqlxR/u4S3F7e5yg7+an ZJ+Zlq5u0GfbzUW8WmU17Hh5tYBIPgSIBFs0uyvuNFKejIFbGFjjtLByrloSMUP6YIxMDJ4pLUB PodQk5w5jPo1Es2PPETb4/007nj/uo/EzFxg0zpmn4MOMJcmzo5L4WSrCJMW1LnxSFsTurcszGl gtjtg3N5dmGhzNWsPucvVMi/4/1lLoVvkghysUxFiRv4KG9UY9YMjdFmPYmyJZnyKvdiksl5ewJ 23xJGJ/Uv/YUmC+79R72irqW1MtTXatNlzI2gA9VyEUqIBveABsqXnU1Qt70qcxhSqOQpCvw= X-Received: by 2002:a17:903:1b64:b0:2c1:13b5:6c24 with SMTP id d9443c01a7336-2c163d85f20mr90804335ad.20.1780596263155; Thu, 04 Jun 2026 11:04:23 -0700 (PDT) Received: from gmail.com ([117.129.60.30]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-2c16649293esm68631925ad.68.2026.06.04.11.04.19 (version=TLS1_3 cipher=TLS_CHACHA20_POLY1305_SHA256 bits=256/256); Thu, 04 Jun 2026 11:04:22 -0700 (PDT) From: Yue Sun To: slava@dubeyko.com, glaubitz@physik.fu-berlin.de, frank.li@vivo.com Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] hfs: prevent MDB and bitmap buffer_head aliasing Date: Fri, 5 Jun 2026 02:02:38 +0800 Message-ID: <20260604180306.18672-1-samsun1006219@gmail.com> X-Mailer: git-send-email 2.54.0 In-Reply-To: <19becc5c5ccd8f22215eb2a1fbc832bed73933f3.camel@dubeyko.com> References: <19becc5c5ccd8f22215eb2a1fbc832bed73933f3.camel@dubeyko.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit On Thu, Jun 4, 2026 at 12:25 AM wrote: > > Could you please take a deeper look into my diff that I've shared with > you before? I don't see the point to review your suggestion if you > completely ignored my diff. From my point of view you suggested > completely the same approach but in more complicated manner. You are right. I am sorry for the confusion. I should have looked more carefully at your diff and based my reply on it. Adding an HFS-level MDB mutex and avoiding holding mdb_bh across the whole hfs_mdb_commit() path is the right direction. My previous reply mixed that with extra cleanups and a separate corrupted-layout check, which made it look like I was proposing a different approach. That was my mistake. I am not very familiar with HFS internals, so please treat the following only as a small suggestion. After re-reading your diff, the only detail I am still unsure about is the HFS_FLG_ALT_MDB_DIRTY path. In that path, hfs_inode_write_fork() gets MDB fields as output buffers: hfs_inode_write_fork(..., mdb->drXTExtRec, &mdb->drXTFlSize, NULL); hfs_inode_write_fork(..., mdb->drCTExtRec, &mdb->drCTFlSize, NULL); Since mdb points into mdb_bh->b_data, these calls seem to update fields in the primary MDB buffer. So I was thinking that we could keep your locking scheme, but move unlock_buffer(mdb_bh) slightly later, after these primary MDB updates and mark_buffer_dirty(mdb_bh). The alternate MDB copy/sync and the bitmap writeback would still happen after mdb_bh is unlocked, so the main deadlock fix remains the same. If this concern is not valid, or if the current HFS update rules already make your shorter window sufficient, please ignore this. I defer to your judgment here. Below is your diff with only that small adjustment. Thanks, Yue diff --git a/fs/hfs/hfs_fs.h b/fs/hfs/hfs_fs.h index ac0e83f77a0f..00651f42ba38 100644 --- a/fs/hfs/hfs_fs.h +++ b/fs/hfs/hfs_fs.h @@ -66,6 +66,7 @@ struct hfs_inode_info { * The HFS-specific part of a Linux (struct super_block) */ struct hfs_sb_info { + struct mutex mdb_lock; /* MDB operations lock */ struct buffer_head *mdb_bh; /* The hfs_buffer holding the real superblock (aka VIB diff --git a/fs/hfs/mdb.c b/fs/hfs/mdb.c index a97cea35ca2e..d81e3545a045 100644 --- a/fs/hfs/mdb.c +++ b/fs/hfs/mdb.c @@ -287,6 +287,7 @@ int hfs_mdb_get(struct super_block *sb) void hfs_mdb_commit(struct super_block *sb) { struct hfs_mdb *mdb = HFS_SB(sb)->mdb; + bool write_backup = false; if (sb_rdonly(sb)) return; @@ -314,11 +315,17 @@ void hfs_mdb_commit(struct super_block *sb) * files grow. */ if (test_and_clear_bit(HFS_FLG_ALT_MDB_DIRTY, &HFS_SB(sb)->flags) && HFS_SB(sb)->alt_mdb) { + write_backup = true; hfs_inode_write_fork(HFS_SB(sb)->ext_tree->inode, mdb->drXTExtRec, &mdb->drXTFlSize, NULL); hfs_inode_write_fork(HFS_SB(sb)->cat_tree->inode, mdb->drCTExtRec, &mdb->drCTFlSize, NULL); + mark_buffer_dirty(HFS_SB(sb)->mdb_bh); + } + unlock_buffer(HFS_SB(sb)->mdb_bh); + + if (write_backup) { lock_buffer(HFS_SB(sb)->alt_mdb_bh); memcpy(HFS_SB(sb)->alt_mdb, HFS_SB(sb)->mdb, HFS_SECTOR_SIZE); HFS_SB(sb)->alt_mdb->drAtrb |= cpu_to_be16(HFS_SB_ATTRIB_UNMNT); @@ -360,7 +367,6 @@ void hfs_mdb_commit(struct super_block *sb) size -= len; } } - unlock_buffer(HFS_SB(sb)->mdb_bh); } void hfs_mdb_close(struct super_block *sb) diff --git a/fs/hfs/super.c b/fs/hfs/super.c index a4f2a2bfa6d3..06ab904b3a42 100644 --- a/fs/hfs/super.c +++ b/fs/hfs/super.c @@ -35,7 +35,11 @@ MODULE_LICENSE("GPL"); static int hfs_sync_fs(struct super_block *sb, int wait) { is_hfs_cnid_counts_valid(sb); + + mutex_lock(&HFS_SB(sb)->mdb_lock); hfs_mdb_commit(sb); + mutex_unlock(&HFS_SB(sb)->mdb_lock); + return 0; } @@ -68,7 +72,9 @@ static void flush_mdb(struct work_struct *work) is_hfs_cnid_counts_valid(sb); + mutex_lock(&sbi->mdb_lock); hfs_mdb_commit(sb); + mutex_unlock(&sbi->mdb_lock); } void hfs_mark_mdb_dirty(struct super_block *sb) @@ -339,9 +345,12 @@ static int hfs_fill_super(struct super_block *sb, struct fs_context *fc) sb->s_op = &hfs_super_operations; sb->s_xattr = hfs_xattr_handlers; sb->s_flags |= SB_NODIRATIME; + mutex_init(&sbi->mdb_lock); mutex_init(&sbi->bitmap_lock); + mutex_lock(&sbi->mdb_lock); res = hfs_mdb_get(sb); + mutex_unlock(&sbi->mdb_lock); if (res) { if (!silent) pr_warn("can't find a HFS filesystem on dev %s\n",