Re: [PATCH] fs: Prevent non-symlinks from entering pick link

[Al Viro <viro@zeniv.linux.org.uk>]

On Wed, Jun 18, 2025 at 01:34:18PM +0800, Edward Adam Davis wrote:
> On Wed, 18 Jun 2025 06:27:47 +0100, Al Viro wrote:
> > Note that anything that calls __d_add(dentry, inode) with is_bad_inode(inode)
> > (or d_add(), or d_instantiate(), or d_splice_alias() under the same conditions)
> > is also FUBAR.
> > 
> > So's anything that calls make_bad_inode() on a struct inode that might be
> > in process of being passed to one of those functions by another thread.
> > 
> > This is fundamentally wrong; bad inodes are not supposed to end up attached
> > to dentries.
> As far as I know, pick_link() is used to resolve the target path of a
> symbolic link (symlink). Can you explain why pick_link() is executed on
> a directory or a regular file?

Because the inode_operations of that thing contains ->get_link().  Which means
"symlink" to dcache.  Again, there is code all over the place written in
assumption that no dentry will ever have ->d_inode pointing to any of those.

No, we are not going to paper over that in __d_add() or __d_instantiate() either;
it's fundamentally a losing game.  _Maybe_ a couple of WARN_ON() when built with
CONFIG_DEBUG_VFS or something similar, but that would only make for slightly
more specific diagnostics; not all that useful, since you can literally grep for
_ntfs_bad_inode to pick the location of actual underlying bugs.

Again, the underlying bug is that make_bad_inode() is called on a live inode.
In some cases it's "icache lookup finds a normal inode, d_splice_alias() is called
to attach it to dentry, while another thread decides to call make_bad_inode() on
it - that would evict it from icache, but we'd already found it there earlier".
In some it's outright "we have an inode attached to dentry - that's how we got
it in the first place; let's call make_bad_inode() on it just for shits and giggles".
Either is a bug.

_ntfs_bad_inode() uses are completely broken.  Matter of fact, we probably ought to
retire make_bad_inode() - there are few callers and most of them don't actually
need anything other than remove_inode_hash() (e.g. iget_failed()).  In any case,
whether there is a case for several new helpers or not, the kind of use
_ntfs_bad_inode() gets is right out.