linux-kernel - Re: [PATCH v3 3/3] dax: Wake up all waiters after invalidating dax entry

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: <20210421092645.GO8706@quack2.suse.cz>
Date:   Wed, 21 Apr 2021 11:26:45 +0200
From:   Jan Kara <jack@...e.cz>
To:     Vivek Goyal <vgoyal@...hat.com>
Cc:     linux-fsdevel@...r.kernel.org, dan.j.williams@...el.com,
        jack@...e.cz, willy@...radead.org, virtio-fs@...hat.com,
        slp@...hat.com, miklos@...redi.hu, linux-nvdimm@...ts.01.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/3] dax: Wake up all waiters after invalidating dax
 entry

On Mon 19-04-21 17:36:36, Vivek Goyal wrote:
> I am seeing missed wakeups which ultimately lead to a deadlock when I am
> using virtiofs with DAX enabled and running "make -j". I had to mount
> virtiofs as rootfs and also reduce to dax window size to 256M to reproduce
> the problem consistently.
> 
> So here is the problem. put_unlocked_entry() wakes up waiters only
> if entry is not null as well as !dax_is_conflict(entry). But if I
> call multiple instances of invalidate_inode_pages2() in parallel,
> then I can run into a situation where there are waiters on
> this index but nobody will wait these.
> 
> invalidate_inode_pages2()
>   invalidate_inode_pages2_range()
>     invalidate_exceptional_entry2()
>       dax_invalidate_mapping_entry_sync()
>         __dax_invalidate_entry() {
>                 xas_lock_irq(&xas);
>                 entry = get_unlocked_entry(&xas, 0);
>                 ...
>                 ...
>                 dax_disassociate_entry(entry, mapping, trunc);
>                 xas_store(&xas, NULL);
>                 ...
>                 ...
>                 put_unlocked_entry(&xas, entry);
>                 xas_unlock_irq(&xas);
>         }
> 
> Say a fault in in progress and it has locked entry at offset say "0x1c".
> Now say three instances of invalidate_inode_pages2() are in progress
> (A, B, C) and they all try to invalidate entry at offset "0x1c". Given
> dax entry is locked, all tree instances A, B, C will wait in wait queue.
> 
> When dax fault finishes, say A is woken up. It will store NULL entry
> at index "0x1c" and wake up B. When B comes along it will find "entry=0"
> at page offset 0x1c and it will call put_unlocked_entry(&xas, 0). And
> this means put_unlocked_entry() will not wake up next waiter, given
> the current code. And that means C continues to wait and is not woken
> up.
> 
> This patch fixes the issue by waking up all waiters when a dax entry
> has been invalidated. This seems to fix the deadlock I am facing
> and I can make forward progress.
> 
> Reported-by: Sergio Lopez <slp@...hat.com>
> Fixes: ac401cc78242 ("dax: New fault locking")
> Suggested-by: Dan Williams <dan.j.williams@...el.com>
> Signed-off-by: Vivek Goyal <vgoyal@...hat.com>

Looks good to me. Thanks for fixing this! Feel free to add:

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

								Honza

> ---
>  fs/dax.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/dax.c b/fs/dax.c
> index f19d76a6a493..cc497519be83 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -676,7 +676,7 @@ static int __dax_invalidate_entry(struct address_space *mapping,
>  	mapping->nrexceptional--;
>  	ret = 1;
>  out:
> -	put_unlocked_entry(&xas, entry, WAKE_NEXT);
> +	put_unlocked_entry(&xas, entry, WAKE_ALL);
>  	xas_unlock_irq(&xas);
>  	return ret;
>  }
> -- 
> 2.25.4
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR