linux-kernel - Re: [RFC PATCH] vfs: add inode lockdep assertions

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: <20230901-begehbar-schubsen-4f59559ad581@brauner>
Date:   Fri, 1 Sep 2023 14:43:07 +0200
From:   Christian Brauner <brauner@...nel.org>
To:     Mateusz Guzik <mjguzik@...il.com>
Cc:     viro@...iv.linux.org.uk, linux-kernel@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, bernd.schubert@...tmail.fm
Subject: Re: [RFC PATCH] vfs: add inode lockdep assertions

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.