On Thu, Aug 31, 2023 at 05:14:14PM +0200, Mateusz Guzik wrote:
> Thread "Use exclusive lock for file_remove_privs" [1] reports an issue
> which should have been found by asserts -- inode not write locked by the
> caller.
>
> It did not happen because the attempt to do it in notify_change:
> WARN_ON_ONCE(!inode_is_locked(inode));
>
> passes if the inode is only read-locked:
> static inline int rwsem_is_locked(struct rw_semaphore *sem)
> {
> return atomic_long_read(&sem->count) != 0;
> }
>
> According to git blame this regressed from 2 commits:
> 1. 5955102c9984 ("wrappers for ->i_mutex access") which replaced a
> bunch of mutex_is_locked with inode_is_locked
> 2. 9902af79c01a ("parallel lookups: actual switch to rwsem") which
> implemented inode_is_locked as a mere check on the semaphore being
> held in *any* manner
>
> In order to remedy this I'm proposing lockdep-ing the check with 2
> helpers: inode_assert_locked and inode_assert_write_locked
>
> Below I'm adding the helpers and converting *some* of the spots modified
> by the first patch. I boot tested it and nothing blow up on ext4, but
> btrfs should cause a complaint.
>
> I can finish the other spots originally touched by 1 and touch up the 3
> uses I grepped in fs/namei.c, but ultimately filesystem maintainers are
> going to have to patch their code at their leasure. On top of that there
> are probably quite a few places which should assert, but don't.
>
> Comments?
I think this is useful and I would be supportive of this.
