linux-kernel - Re: [PATCH v5 0/4] hide ->i

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGudoHG6HgXThjeaeDWfngiNCWdikczgN_3Z_T8sKJt4CaR-ow@mail.gmail.com>
Date: Mon, 22 Sep 2025 13:40:43 +0200
From: Mateusz Guzik <mjguzik@...il.com>
To: Jan Kara <jack@...e.cz>
Cc: Russell Haley <yumpusamongus@...il.com>, brauner@...nel.org, viro@...iv.linux.org.uk, 
	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, ceph-devel@...r.kernel.org, 
	linux-unionfs@...r.kernel.org
Subject: Re: [PATCH v5 0/4] hide ->i_state behind accessors

On Mon, Sep 22, 2025 at 1:36 PM Jan Kara <jack@...e.cz> wrote:
>
> On Sat 20-09-25 07:47:46, Mateusz Guzik wrote:
> > On Sat, Sep 20, 2025 at 6:31 AM Russell Haley <yumpusamongus@...il.com> wrote:
> > >
> > > On 9/19/25 10:49 AM, Mateusz Guzik wrote:
> > > > This is generated against:
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/commit/?h=vfs-6.18.inode.refcount.preliminaries
> > > >
> > > > First commit message quoted verbatim with rationable + API:
> > > >
> > > > [quote]
> > > > Open-coded accesses prevent asserting they are done correctly. One
> > > > obvious aspect is locking, but significantly more can checked. For
> > > > example it can be detected when the code is clearing flags which are
> > > > already missing, or is setting flags when it is illegal (e.g., I_FREEING
> > > > when ->i_count > 0).
> > > >
> > > > Given the late stage of the release cycle this patchset only aims to
> > > > hide access, it does not provide any of the checks.
> > > >
> > > > Consumers can be trivially converted. Suppose flags I_A and I_B are to
> > > > be handled, then:
> > > >
> > > > state = inode->i_state        => state = inode_state_read(inode)
> > > > inode->i_state |= (I_A | I_B)         => inode_state_add(inode, I_A | I_B)
> > > > inode->i_state &= ~(I_A | I_B)        => inode_state_del(inode, I_A | I_B)
> > > > inode->i_state = I_A | I_B    => inode_state_set(inode, I_A | I_B)
> > > > [/quote]
> > >
> > > Drive-by bikeshedding: s/set/replace/g
> > >
> > > "replace" removes ambiguity with the concept of setting a bit ( |= ). An
> > > alternative would be "set_only".
> > >
> >
> > I agree _set may be ambiguous here. I was considering something like
> > _assign or _set_value instead.
>
> I agree _assign might be a better option. In fact my favorite variant would
> be:
>
> inode_state_set() - setting bit in state
> inode_state_clear() - clearing bit in state
> inode_state_assign() - assigning value to state
>
> But if you just rename inode_state_set() to inode_state_assign() that would
> be already good.

well renaming is just a matter of sed, so rolling with 3 or 1 does not
make material difference
that said, the set/clear/assign trio sgtm, i should have proposed it
after assign :P