Re: [PATCH] vfs: handle __wait_on_freeing_inode() and evict() race

[Jan Kara]

On Thu 18-07-24 17:18:37, Mateusz Guzik wrote:
> Lockless hash lookup can find and lock the inode after it gets the
> I_FREEING flag set, at which point it blocks waiting for teardown in
> evict() to finish.
> 
> However, the flag is still set even after evict() wakes up all waiters.
> 
> This results in a race where if the inode lock is taken late enough, it
> can happen after both hash removal and wakeups, meaning there is nobody
> to wake the racing thread up.
> 
> This worked prior to RCU-based lookup because the entire ordeal was
> synchronized with the inode hash lock.
> 
> Since unhashing requires the inode lock, we can safely check whether it
> happened after acquiring it.
> 
> Link: https://lore.kernel.org/v9fs/20240717102458.649b60be@xxxxxxxxxx/
> Reported-by: Dominique Martinet <asmadeus@xxxxxxxxxxxxx>
> Fixes: 7180f8d91fcb ("vfs: add rcu-based find_inode variants for iget ops")
> Signed-off-by: Mateusz Guzik <mjguzik@xxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
> 
> The 'fixes' tag is contingent on testing by someone else. :>
> 
> I have 0 experience with 9pfs and the docs failed me vs getting it
> running on libvirt+qemu, so I gave up on trying to test it myself.
> 
> Dominique, you offered to narrow things down here, assuming the offer
> stands I would appreciate if you got this sorted out :)
> 
> Even if the patch in the current form does not go in, it should be
> sufficient to confirm the problem diagnosis is correct.
> 
> A debug printk can be added to validate the problematic condition was
> encountered, for example:
> 
> > diff --git a/fs/inode.c b/fs/inode.c
> > index 54e0be80be14..8f61fad0bc69 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -2308,6 +2308,7 @@ static void __wait_on_freeing_inode(struct inode *inode, bool locked)
> >         if (unlikely(inode_unhashed(inode))) {
> >                 BUG_ON(locked);
> >                 spin_unlock(&inode->i_lock);
> > +               printk(KERN_EMERG "%s: got unhashed inode %p\n", __func__, inode);
> >                 return;
> >         }
> 
> 
>  fs/inode.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index f356fe2ec2b6..54e0be80be14 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -676,6 +676,16 @@ static void evict(struct inode *inode)
>  
>  	remove_inode_hash(inode);
>  
> +	/*
> +	 * Wake up waiters in __wait_on_freeing_inode().
> +	 *
> +	 * Lockless hash lookup may end up finding the inode before we removed
> +	 * it above, but only lock it *after* we are done with the wakeup below.
> +	 * In this case the potential waiter cannot safely block.
> +	 *
> +	 * The inode being unhashed after the call to remove_inode_hash() is
> +	 * used as an indicator whether blocking on it is safe.
> +	 */
>  	spin_lock(&inode->i_lock);
>  	wake_up_bit(&inode->i_state, __I_NEW);
>  	BUG_ON(inode->i_state != (I_FREEING | I_CLEAR));
> @@ -2291,6 +2301,16 @@ static void __wait_on_freeing_inode(struct inode *inode, bool locked)
>  {
>  	wait_queue_head_t *wq;
>  	DEFINE_WAIT_BIT(wait, &inode->i_state, __I_NEW);
> +
> +	/*
> +	 * Handle racing against evict(), see that routine for more details.
> +	 */
> +	if (unlikely(inode_unhashed(inode))) {
> +		BUG_ON(locked);
> +		spin_unlock(&inode->i_lock);
> +		return;
> +	}
> +
>  	wq = bit_waitqueue(&inode->i_state, __I_NEW);
>  	prepare_to_wait(wq, &wait.wq_entry, TASK_UNINTERRUPTIBLE);
>  	spin_unlock(&inode->i_lock);
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR