linux-kernel - Re: [PATCH 1/2] fs/dcache: introduce d_alloc_parallel_check

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] [day] [month] [year] [list]

Message-ID: <87frqwwjua.fsf@mailhost.krisman.be>
Date: Thu, 22 Aug 2024 13:25:33 -0400
From: Gabriel Krisman Bertazi <krisman@...e.de>
To: Al Viro <viro@...iv.linux.org.uk>
Cc: Eugen Hristev <eugen.hristev@...labora.com>,  brauner@...nel.org,
  tytso@....edu,  linux-ext4@...r.kernel.org,  jack@...e.cz,
  adilger.kernel@...ger.ca,  linux-fsdevel@...r.kernel.org,
  linux-kernel@...r.kernel.org,  kernel@...labora.com,
  shreeya.patel@...labora.com
Subject: Re: [PATCH 1/2] fs/dcache: introduce d_alloc_parallel_check_existing

Al Viro <viro@...iv.linux.org.uk> writes:

> On Wed, Aug 21, 2024 at 07:22:39PM -0400, Gabriel Krisman Bertazi wrote:
>
>> Would it be acceptable to just change the dentry->d_name here in a new
>> flavor of d_add_ci used only by these filesystems? We are inside the
>> creation path, so the dentry has never been hashed.  Concurrent lookups
>> will be stuck in d_wait_lookup() until we are done and will never become
>> invalid after the change because the lookup was already done
>> case-insensitively, so they all match the same dentry, per-definition,
>> and we know there is no other matching dentries in the directory.  We'd
>> only need to be careful not to expose partial names to concurrent
>> parallel lookups.
>
> *Ow*
>
> ->d_name stability rules are already convoluted as hell; that would make
> them even more painful.
>
> What locking are you going to use there?

Since we are in the ->d_lookup() during the rename, and we use the
dcache-insensitively for the filesystems that will do the rename, we
know there is nothing in the dcache and the dentry is still in the
parallel lookup table.  So we are not racing with a creation of the same
name in the same directory.  A parallel lookup will either find that
dentry (old or new name, doesn't matter) or not find anything, in case
it sees a partial ->d_name.  Therefore, the only possible problem is a
false negative/positive in parent->d_in_lookup_hash.

Can we extend the rename_lock seqlock protection that already exists in
d_alloc_parallel to include the d_in_lookup_hash walk?  d_add_ci then
acquires the rename_lock before writing ->d_name and d_alloc_parallel
will see it changed after iterating over d_in_lookup_hash, in case it
didn't find anything, and retry the entire sequence.

Case-inexact lookups are not supposed to be frequent. Most lookups
should be done in a case-exact way, so the extra acquisition of
rename_lock shouldn't create more contention on the rename_lock for the
regular path or for non-case-insensitive filesystems.  The overhead in
d_alloc_parallel is another read_seqretry() that is done only in the
case where the dentry is not found anywhere and should be created.

-- 
Gabriel Krisman Bertazi