From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 96537388E57 for ; Fri, 29 May 2026 23:23:29 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.129.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780097011; cv=none; b=ktjSUIBbJXJPPHU1moDOoirOJyLjhBBHQiQ3q9NfsRQTGvEUi3tKoihFMtz065Igad4c4my2zxuw5yN5xHJT2hAVkI5bxaOqaxr8z0q/wFIST6iOCeYHHq1ensvRzoucvirfNT8FLrR0nSNPmORu5LsY0IxFC5d5CLTu20I5jw0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780097011; c=relaxed/simple; bh=XkXt5xvWbeRkGH7vkAfiINhBts1WmxKb9QgFAuL7S/M=; h=Message-ID:Subject:From:To:Cc:Date:In-Reply-To:References: Content-Type:MIME-Version; b=j8kvPP/fvtEAXNeqA2HtVIApoV9iNrAKlNwq+6nU9m7qE0f0mGJ7uety5Tm5sUQjHJDcD6aSWH2453BoFFDBmLEUiJs+MyBTuw+q5572t054d8oVxw8O3/1P6HM/xdMD789HHxMIy/vd0PntWIRbo1SJLcqYPapIFy0oe7R/r90= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=O5dC71IW; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b=rdcG3MUo; arc=none smtp.client-ip=170.10.129.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="O5dC71IW"; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b="rdcG3MUo" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1780097008; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=MN/GG/B80cTeN/1dBywgE/jxTvXiCfeDsRJwHlMLSwI=; b=O5dC71IWzsrJnrtbXya9SziJVk/2R6hbD3OH6xKK3ktTrM4TNiPPrY9B44KtDHpUjQ09Om 74FjuSyOP3jQb3szf4heGslWURNta24hvSZ9dF6JWkM4k6gdQZRAetnFMbkpMIW17s0Z1a 4C6KF0bWKnlcMqj3XVfC5FBQ+zXyH2A= Received: from mail-oa1-f71.google.com (mail-oa1-f71.google.com [209.85.160.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-529-bpjQQQh6MimpdwrFpaJ1tg-1; Fri, 29 May 2026 19:23:27 -0400 X-MC-Unique: bpjQQQh6MimpdwrFpaJ1tg-1 X-Mimecast-MFC-AGG-ID: bpjQQQh6MimpdwrFpaJ1tg_1780097006 Received: by mail-oa1-f71.google.com with SMTP id 586e51a60fabf-43cabbedc29so182609fac.0 for ; Fri, 29 May 2026 16:23:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=google; t=1780097006; x=1780701806; darn=vger.kernel.org; h=mime-version:user-agent:content-transfer-encoding:references :in-reply-to:date:cc:to:from:subject:message-id:from:to:cc:subject :date:message-id:reply-to; bh=MN/GG/B80cTeN/1dBywgE/jxTvXiCfeDsRJwHlMLSwI=; b=rdcG3MUonti+0sllbgFya4EGwzXmDBLyeiizGXTGlhexgUfcanfHn0lNYa2rhJVz5n gi55vUFn4R57XToVRIm5Vgp9FtwOp1f9eo+dwpHK787Xdf9qEHzAFRJhRA/U2wKh77tO /EKBiN/RNsqH9O8CHeRcJviXvLCGpR2KUoTDo1ldT5lsSjK4ogEySdNXyfl33Yn6H8f2 SLfytipB25R29911FE78EdQxUF21f9JeLZe+Bmh/P2sVNb4050nFCpM1Xu+QiMKKmq+v rozruVdWyOyZJ1byPc18EzQI5zzBMohYmt44BObf9x3VhT0QuM4LS06bGtgyLlvLXZvh zF4w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1780097006; x=1780701806; h=mime-version:user-agent:content-transfer-encoding:references :in-reply-to:date:cc:to:from:subject:message-id:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=MN/GG/B80cTeN/1dBywgE/jxTvXiCfeDsRJwHlMLSwI=; b=S1PEuH0dmi9l6JF0raCajKb+ZQcvhs9ytJULZzIiArWFA8FXPaWSIqvjjC8i07Xt9p mQfx200474zCSz7/q7JiedBVUZ3bck6eEtL4zviK87cB/3OqCyKAHgF0qSzlElhi48Bv u1+l4gFnrqD0w6y5G9ey7OANX3wcnkby/2LeWGgEnhHUZ4neNpfaw5vbMrFaq3J/36J1 3LFr71BnrCHlvsFAHD3qBxVyki3x5KSATLilYxa4R9IvuOq7XVfOHOrjejtJfp6P+SLV Ub+XWpzT0PQHciurN8VK3nDP5sMfmD7GgwBH8zt/uDURbWUnsHofHck4Lk7eaEWsbhvM LlEA== X-Forwarded-Encrypted: i=1; AFNElJ8rtiD/ED7/sx2yRpAPmIF4VD66QPZDVYus6sBOCorOU7RNBqm5bSkX/YJp8N4canWs/R0s9W0lDgCkrbY=@vger.kernel.org X-Gm-Message-State: AOJu0YwBZ8GD6OwcSUaeGyTN8S5hHaIifKgs+c9s/jS5rSjMROnCSOpY xXtzA66GmtQPZZJ4+HIa/F9DLWCKo58Hidg/TXdTYxAjqPFAtOi6RA2zCrFjaaduxnIrLTHIJUD s6fwHqxNELHuCwfXL91iaMggfqCwP5qSybHm/uIyPtrIclDk+/FG0Jnh/HgOe+7a6JbudZHOyyA == X-Gm-Gg: Acq92OFuoArkM2+p9O7rSTP3R/qkh9WpUZ5CBJszMpeAYDWVN4a8t3NLNvTkg4vs1dP u0bKkjSVik5FYhP4Mnb8JP8J281Nki7aeQQf1C9ElzUhq1kmsVk8wZQCh1i44KAEiIYQnEuhnsv BR5Fs7NK5lM4SAG0ld83Nnx0AtC/AUhcnT/HYDFK+B9iGgsaKooEAmEf9fZgc0lljhfwChd0G4O OeAelDlSafbAjeoK4MnY1MmNELon18hu3Am0/C05JhKxmOZW5YCuDcgk2/5Tfv9Ua5OP27CiwmH umvClO3R4ZQhdy4IfTnZpuvUIm+UoSz7NyCHQ0g3riqbeD60OCaHrKj5s9v0eXxOxRclMxvg5Nk f7JuTAyJ1XTZOOOvZ3oh1+Qw8BgucV4kX8QeoTZhq6XFAsO6xL0eE8O2uDUumTAI= X-Received: by 2002:a05:6820:2d0b:b0:696:7f3d:a75d with SMTP id 006d021491bc7-69e0ff718femr666155eaf.27.1780097006444; Fri, 29 May 2026 16:23:26 -0700 (PDT) X-Received: by 2002:a05:6820:2d0b:b0:696:7f3d:a75d with SMTP id 006d021491bc7-69e0ff718femr666149eaf.27.1780097005982; Fri, 29 May 2026 16:23:25 -0700 (PDT) Received: from li-4c4c4544-0032-4210-804c-c3c04f423534.ibm.com ([2600:1700:6476:1430::29]) by smtp.gmail.com with ESMTPSA id 586e51a60fabf-43c93a22989sm1979271fac.1.2026.05.29.16.23.24 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 29 May 2026 16:23:25 -0700 (PDT) Message-ID: <1924119775624adb4ce3fd7f3d936eaa2fea3c16.camel@redhat.com> Subject: Re: [EXTERNAL] Re: [PATCH v2] hfs: prevent MDB and bitmap buffer_head aliasing From: Viacheslav Dubeyko To: Yue Sun , slava@dubeyko.com, glaubitz@physik.fu-berlin.de, frank.li@vivo.com Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Date: Fri, 29 May 2026 16:23:24 -0700 In-Reply-To: <09c2bf1e91f7ddabc24240379cd7dd0b4e15dd8d.camel@redhat.com> References: <1d26872db999da42b2d8452ace8a98b79e569cc8.camel@dubeyko.com> <20260529103428.99099-1-samsun1006219@gmail.com> <09c2bf1e91f7ddabc24240379cd7dd0b4e15dd8d.camel@redhat.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.60.1 (3.60.1-1.fc44app2) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 On Fri, 2026-05-29 at 14:29 -0700, Viacheslav Dubeyko wrote: > On Fri, 2026-05-29 at 18:34 +0800, Yue Sun wrote: > > hfs_mdb_commit() writes the volume bitmap while HFS_SB(sb)->mdb_bh is > > locked. A crafted image can set drVBMSt so that the bitmap block resolv= es > > to the same buffer_head as the MDB. When writeback later calls > > lock_buffer() for that bitmap block, the task tries to lock mdb_bh agai= n > > and self-deadlocks in __lock_buffer(). > >=20 > > Reject images whose volume bitmap starts at or before the MDB during > > mount. Also guard the bitmap writeback path itself: if the bitmap block > > would resolve to mdb_bh, force the filesystem read-only and stop bitmap > > writeback before taking the buffer lock. This keeps the deadlock fix in > > the MDB commit path and reuses the existing bitmap size/writeback logic= . > >=20 > > Reported-by: Yue Sun > > Closes: https://urldefense.proofpoint.com/v2/url?u=3Dhttps-3A__lore.ker= nel.org_all_CAEkJfYMB47v1yOWHB8q2dc8kf-3Duj-2DrLO-3D-2ByMyudwPguJ8Kd3jA-40m= ail.gmail.com_&d=3DDwIFaQ&c=3DBSDicqBQBDjDI9RkVyTcHQ&r=3Dq5bIm4AXMzc8NJu1_R= GmnQ2fMWKq4Y4RAkElvUgSs00&m=3D2ATrdgpvRdYnJfz2T53Ih3iQv6Y9wogU2Ba19prTYgchh= 1mh4KI7lo9TYLQnlu3X&s=3DS8XdWVW0SpxB05CfctrqCJtJEXqtWDfLNdtzPHIZAY8&e=3D= =20 > > Signed-off-by: Yue Sun > > --- > > Changes in v2: > > - Add a commit-time guard before locking bitmap buffer_heads. > > - Replace the mount-time byte-range check with a simple drVBMSt check. > > - Reuse the existing bitmap writeback size calculation. > >=20 > > fs/hfs/mdb.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > >=20 > > diff --git a/fs/hfs/mdb.c b/fs/hfs/mdb.c > > index a97cea35ca2e..53cd137892b5 100644 > > --- a/fs/hfs/mdb.c > > +++ b/fs/hfs/mdb.c > > @@ -185,6 +185,11 @@ int hfs_mdb_get(struct super_block *sb) > > sb->s_flags |=3D SB_RDONLY; > > } > > =20 > > + if (be16_to_cpu(mdb->drVBMSt) <=3D HFS_MDB_BLK) { >=20 > Technically speaking, if we are trying to check the overlapping of volume= bitmap > with the main MDB record, then we need to check the overlapping with alte= rnative > MDB record, and with Catalog File and Extents Overflow File. However, it = sounds > like we are trying to add some FSCK logic here. :) >=20 > > + pr_err("volume bitmap overlaps MDB\n"); >=20 > This situation means volume corruption. It makes sense to recommend to ru= n FSCK. >=20 > > + return -EIO; >=20 > This code error is wrong because the read operation was OK. But we have > corrupted volume. Even if we have overlapping of volume bitmap with MDB r= ecord, > then we cannot reject mount operation. We must mount in READ-ONLY mode be= cause, > potentially, the rest of metadata could be completely OK. We simply canno= t mount > in RW mode. >=20 > > + } > > + > > /* TRY to get the alternate (backup) MDB. */ > > sect =3D part_start + part_size - 2; > > bh =3D sb_bread512(sb, sect, mdb2); > > @@ -341,6 +346,11 @@ void hfs_mdb_commit(struct super_block *sb) > > size =3D (HFS_SB(sb)->fs_ablocks + 7) / 8; > > ptr =3D (u8 *)HFS_SB(sb)->bitmap; > > while (size) { > > + if (unlikely(block =3D=3D HFS_SB(sb)->mdb_bh->b_blocknr)) { > > + pr_err("volume bitmap overlaps MDB, forcing read-only\n"); > > + sb->s_flags |=3D SB_RDONLY; > > + break; > > + } >=20 > At this point, we already wrote main MDB and alternative MDB to the volum= e. > Theoretically, it is possible to imagine that if size of volume bitmap is= big > enough, then we could partially process the bitmap too. Probably, we need= to > check the overlapping at the beginning of the method and reject the whole > superblock commit. >=20 > Initial issue was the deadlock. Could we implement some check that buffer= _heads > don't overlap before trying to lock? Does it make sense to you? >=20 By the way, I can see the deadlock in hfs_mdb_commit() even for completely = valid HFS volume during generic/013 test run. This issue takes place because foli= o lock related issue. I remember that somebody has sent the patch related to = this issue but we haven't finished the discussion with some reasonable solution.= I think we need to rework the locking scheme: diff --git a/fs/hfs/hfs_fs.h b/fs/hfs/hfs_fs.h index 97e8d1f96d6d..919798eda0f8 100644 --- a/fs/hfs/hfs_fs.h +++ b/fs/hfs/hfs_fs.h @@ -64,6 +64,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..94497c155d29 100644 --- a/fs/hfs/mdb.c +++ b/fs/hfs/mdb.c @@ -291,9 +291,9 @@ void hfs_mdb_commit(struct super_block *sb) if (sb_rdonly(sb)) return; =20 - lock_buffer(HFS_SB(sb)->mdb_bh); if (test_and_clear_bit(HFS_FLG_MDB_DIRTY, &HFS_SB(sb)->flags)) { /* These parameters may have been modified, so write them back */ + lock_buffer(HFS_SB(sb)->mdb_bh); mdb->drLsMod =3D hfs_mtime(); mdb->drFreeBks =3D cpu_to_be16(HFS_SB(sb)->free_ablocks); mdb->drNxtCNID =3D @@ -304,6 +304,7 @@ void hfs_mdb_commit(struct super_block *sb) cpu_to_be32((u32)atomic64_read(&HFS_SB(sb)- >file_count)); mdb->drDirCnt =3D cpu_to_be32((u32)atomic64_read(&HFS_SB(sb)- >folder_count)); + unlock_buffer(HFS_SB(sb)->mdb_bh); =20 /* write MDB to disk */ mark_buffer_dirty(HFS_SB(sb)->mdb_bh); @@ -360,7 +361,6 @@ void hfs_mdb_commit(struct super_block *sb) size -=3D len; } } - unlock_buffer(HFS_SB(sb)->mdb_bh); } =20 void hfs_mdb_close(struct super_block *sb) diff --git a/fs/hfs/super.c b/fs/hfs/super.c index a466c401f6bb..5d4caf3ddda6 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; } =20 @@ -68,7 +72,9 @@ static void flush_mdb(struct work_struct *work) =20 is_hfs_cnid_counts_valid(sb); =20 + mutex_lock(&sbi->mdb_lock); hfs_mdb_commit(sb); + mutex_unlock(&sbi->mdb_lock); } =20 void hfs_mark_mdb_dirty(struct super_block *sb) @@ -339,9 +345,13 @@ static int hfs_fill_super(struct super_block *sb, stru= ct fs_context *fc) sb->s_op =3D &hfs_super_operations; sb->s_xattr =3D hfs_xattr_handlers; sb->s_flags |=3D SB_NOATIME | SB_NODIRATIME; + mutex_init(&sbi->mdb_lock); mutex_init(&sbi->bitmap_lock); =20 + mutex_lock(&sbi->mdb_lock); res =3D 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", What do you think? Thanks, Slava.