| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250911045557.1552002-1-mjguzik@gmail.com>
Date: Thu, 11 Sep 2025 06:55:53 +0200
From: Mateusz Guzik <mjguzik@...il.com>
To: brauner@...nel.org
Cc: viro@...iv.linux.org.uk,
jack@...e.cz,
linux-kernel@...r.kernel.org,
linux-fsdevel@...r.kernel.org,
josef@...icpanda.com,
kernel-team@...com,
amir73il@...il.com,
linux-btrfs@...r.kernel.org,
linux-ext4@...r.kernel.org,
linux-xfs@...r.kernel.org,
ocfs2-devel@...ts.linux.dev,
Mateusz Guzik <mjguzik@...il.com>
Subject: [RFC PATCH v3 0/4] ->i_state accessors
This is generated against:
https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/commit/?h=vfs-6.18.inode.refcount.preliminaries
The first patch in the series is ready to use(tm) and was sent
separately here:
https://lore.kernel.org/linux-fsdevel/20250909082613.1296550-1-mjguzik@gmail.com/T/#u
It is included in this posting as the newly patched routine has to get
patched further due to the i_state accessor thing. Having it here should
make it handier to test for interested.
Compared to v2 this drops I_WILL_FREE removal for the time being and
converts the entirety of the kernel.
Patch rundown:
fs: hide ->i_state handling behind accessors
Provides the new helpers and converts general kernel (fs/*.c and others,
but no specific filesystems). ->i_state temporarily remains accessible
with plain loads/stores.
convert all filesystems to use the new ->i_state accessors
This is a squashed patch to avoid sending a 50-mail churn patchset at
this stage, included for reference for interested how things are going
to look like. This will be one patch per fs in the actual submission.
Conversion generated with coccinelle + manual fixups.
I got a kernel with ext4 running, I have not tested xfs or btrfs yet.
type switch
This hides ->i_state behind a struct, so things nicely fail to compile
if someone open-codes a plain access.
Rationale:
The handling in the stock kernel is highly error prone and with 0 assert
coverage.
The ->i_state field is legally accessible for reads without holding the
inode spinlock. This opens up potential for compiler mischief in code
doing things like:
state = inode->i_state;
if (state & I_FOO)
....
else if (state & I_BAR)
....
Other code may be expecting the value to be stable (only guaranteed with
the spinlock held) and unknowingly being called without that guarantee.
Some places modifying or reading the field use WRITE_ONCE/READ_ONCE
respectively, but others don't.
Modifying flags requires the lock, but right now it is never checked if
such operations are done while it is being held.
Even ignoring that aspect, all current state modifications lack any
validation whether a given change is legal to do. For example there may
be things which are illegal to do once I_FREEING is set, but the current
code fails to check for it in any capacity.
With the assumption the general intent is to tighten up the interfaces,
I think this is a step forward. Note the new routines can assert on
->i_state changes based on other parts of the inode (trivial example:
the routine can validate nobody sets I_FREEING if there is a refcount >
0).
The proposed routines still remain kind of lax in what they do, notably
there are _unchecked variants to bypass any validation (used in special
cases and by XFS). To be tightened up over time.
As sample usage I'm going to quote part of iput_final() with the
infamous I_WILL_FREE flag:
if (!drop) {
inode_state_add(inode, I_WILL_FREE);
spin_unlock(&inode->i_lock);
write_inode_now(inode, 1);
spin_lock(&inode->i_lock);
inode_state_del(inode, I_WILL_FREE);
WARN_ON(inode_state_read(inode) & I_NEW);
}
inode_state_add(inode, I_FREEING);
Lockless loads are a little bit verbose though:
VFS_BUG_ON_INODE(inode_state_read_unstable(inode) & I_CLEAR, inode);
I'm not going to argue about naming, it can be trivially changed with
sed on the patchset.
I do claim whacking open-coded access to ->i_state is a mandatory part
of whipping VFS into a better shape in terms of maintainability. I'm not
going to repeat myself how and why, a description can be found in my
responses to the i_obj_count thread:
https://lore.kernel.org/linux-fsdevel/dpi5ey667awq63mgxrzu6qdfpeinrmeapgbllqidcdjayanz2p@kp3alvfskssp/
https://lore.kernel.org/linux-fsdevel/frffkyu6vac5v7qt5ee36xmg6hrwwdks7mnn2k7krdqecn56mc@3kwx24xtmane/
If this looks good I'll write proper commit messages and whatnot, do
some more testing and properly resend.
Mateusz Guzik (4):
fs: expand dump_inode()
fs: hide ->i_state handling behind accessors
convert all filesystems to use the new ->i_state accessors
type switch
block/bdev.c | 4 +-
drivers/dax/super.c | 2 +-
fs/9p/vfs_inode.c | 2 +-
fs/9p/vfs_inode_dotl.c | 2 +-
fs/affs/inode.c | 2 +-
fs/afs/dynroot.c | 6 +-
fs/afs/inode.c | 8 +-
fs/bcachefs/fs.c | 7 +-
fs/befs/linuxvfs.c | 2 +-
fs/bfs/inode.c | 2 +-
fs/btrfs/inode.c | 10 +--
fs/buffer.c | 4 +-
fs/ceph/cache.c | 2 +-
fs/ceph/crypto.c | 4 +-
fs/ceph/file.c | 4 +-
fs/ceph/inode.c | 28 +++----
fs/coda/cnode.c | 4 +-
fs/cramfs/inode.c | 2 +-
fs/crypto/keyring.c | 2 +-
fs/crypto/keysetup.c | 2 +-
fs/dcache.c | 8 +-
fs/drop_caches.c | 2 +-
fs/ecryptfs/inode.c | 6 +-
fs/efs/inode.c | 2 +-
fs/erofs/inode.c | 2 +-
fs/ext2/inode.c | 2 +-
fs/ext4/inode.c | 10 +--
fs/ext4/orphan.c | 4 +-
fs/f2fs/data.c | 2 +-
fs/f2fs/inode.c | 2 +-
fs/f2fs/namei.c | 4 +-
fs/f2fs/super.c | 2 +-
fs/freevxfs/vxfs_inode.c | 2 +-
fs/fs-writeback.c | 123 ++++++++++++++++---------------
fs/fuse/inode.c | 4 +-
fs/gfs2/file.c | 2 +-
fs/gfs2/glops.c | 2 +-
fs/gfs2/inode.c | 4 +-
fs/gfs2/ops_fstype.c | 2 +-
fs/hfs/btree.c | 2 +-
fs/hfs/inode.c | 2 +-
fs/hfsplus/super.c | 2 +-
fs/hostfs/hostfs_kern.c | 2 +-
fs/hpfs/dir.c | 2 +-
fs/hpfs/inode.c | 2 +-
fs/inode.c | 111 +++++++++++++++-------------
fs/isofs/inode.c | 2 +-
fs/jffs2/fs.c | 4 +-
fs/jfs/file.c | 4 +-
fs/jfs/inode.c | 2 +-
fs/jfs/jfs_txnmgr.c | 2 +-
fs/kernfs/inode.c | 2 +-
fs/libfs.c | 6 +-
fs/minix/inode.c | 2 +-
fs/namei.c | 8 +-
fs/netfs/misc.c | 8 +-
fs/netfs/read_single.c | 6 +-
fs/nfs/inode.c | 2 +-
fs/nfs/pnfs.c | 2 +-
fs/nfsd/vfs.c | 2 +-
fs/nilfs2/cpfile.c | 2 +-
fs/nilfs2/dat.c | 2 +-
fs/nilfs2/ifile.c | 2 +-
fs/nilfs2/inode.c | 10 +--
fs/nilfs2/sufile.c | 2 +-
fs/notify/fsnotify.c | 2 +-
fs/ntfs3/inode.c | 2 +-
fs/ocfs2/dlmglue.c | 2 +-
fs/ocfs2/inode.c | 10 +--
fs/omfs/inode.c | 2 +-
fs/openpromfs/inode.c | 2 +-
fs/orangefs/inode.c | 2 +-
fs/orangefs/orangefs-utils.c | 6 +-
fs/overlayfs/dir.c | 2 +-
fs/overlayfs/inode.c | 6 +-
fs/overlayfs/util.c | 10 +--
fs/pipe.c | 2 +-
fs/qnx4/inode.c | 2 +-
fs/qnx6/inode.c | 2 +-
fs/quota/dquot.c | 2 +-
fs/romfs/super.c | 2 +-
fs/smb/client/cifsfs.c | 2 +-
fs/smb/client/inode.c | 14 ++--
fs/squashfs/inode.c | 2 +-
fs/sync.c | 2 +-
fs/ubifs/file.c | 2 +-
fs/ubifs/super.c | 2 +-
fs/udf/inode.c | 2 +-
fs/ufs/inode.c | 2 +-
fs/xfs/scrub/common.c | 2 +-
fs/xfs/scrub/inode_repair.c | 2 +-
fs/xfs/scrub/parent.c | 2 +-
fs/xfs/xfs_bmap_util.c | 2 +-
fs/xfs/xfs_health.c | 4 +-
fs/xfs/xfs_icache.c | 6 +-
fs/xfs/xfs_inode.c | 6 +-
fs/xfs/xfs_inode_item.c | 4 +-
fs/xfs/xfs_iops.c | 2 +-
fs/xfs/xfs_reflink.h | 2 +-
fs/zonefs/super.c | 4 +-
include/linux/backing-dev.h | 5 +-
include/linux/fs.h | 59 ++++++++++++++-
include/linux/writeback.h | 4 +-
include/trace/events/writeback.h | 8 +-
mm/backing-dev.c | 2 +-
security/landlock/fs.c | 2 +-
106 files changed, 368 insertions(+), 309 deletions(-)
--
2.43.0
Powered by blists - more mailing lists