linux-kernel - Re: [PATCH] vfs: handle __wait_on_freeing

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: <20240718204012.x4ysnjmvjh5v2zf3@quack3>
Date: Thu, 18 Jul 2024 22:40:12 +0200
From: Jan Kara <jack@...e.cz>
To: Mateusz Guzik <mjguzik@...il.com>
Cc: brauner@...nel.org, viro@...iv.linux.org.uk, jack@...e.cz,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	Dominique Martinet <asmadeus@...ewreck.org>,
	Jakub Kicinski <kuba@...nel.org>, v9fs@...ts.linux.dev
Subject: Re: [PATCH] vfs: handle __wait_on_freeing_inode() and evict() race

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@kernel.org/
> Reported-by: Dominique Martinet <asmadeus@...ewreck.org>
> Fixes: 7180f8d91fcb ("vfs: add rcu-based find_inode variants for iget ops")
> Signed-off-by: Mateusz Guzik <mjguzik@...il.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@...e.cz>

								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@...e.com>
SUSE Labs, CR