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.
