linux-kernel - Re: [PATCH v2 1/2] fuse: Fix possible deadlock when writing back dirty pages

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: <CAJfpegvTX9rS0D6TXUUz3urrPFHng_1OntSWah+CU-7Fo5F-7g@mail.gmail.com>
Date:   Wed, 14 Apr 2021 11:47:52 +0200
From:   Miklos Szeredi <miklos@...redi.hu>
To:     Baolin Wang <baolin.wang@...ux.alibaba.com>
Cc:     Peng Tao <tao.peng@...ux.alibaba.com>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] fuse: Fix possible deadlock when writing back
 dirty pages

On Wed, Apr 14, 2021 at 11:22 AM Baolin Wang
<baolin.wang@...ux.alibaba.com> wrote:
>
>
>
> 在 2021/4/14 17:02, Miklos Szeredi 写道:
> > On Wed, Apr 14, 2021 at 10:42 AM Baolin Wang
> > <baolin.wang@...ux.alibaba.com> wrote:
> >
> >> Sorry I missed this patch before, and I've tested this patch, it seems
> >> can solve the deadlock issue I met before.
> >
> > Great, thanks for testing.
> >
> >> But look at this patch in detail, I think this patch only reduced the
> >> deadlock window, but did not remove the possible deadlock scenario
> >> completely like I explained in the commit log.
> >>
> >> Since the fuse_fill_write_pages() can still lock the partitail page in
> >> your patch, and will be wait for the partitail page waritehack is
> >> completed if writeback is set in fuse_send_write_pages().
> >>
> >> But at the same time, a writeback worker thread may be waiting for
> >> trying to lock the partitail page to write a bunch of dirty pages by
> >> fuse_writepages().
> >
> > As you say, fuse_fill_write_pages() will lock a partial page.  This
> > page cannot become dirty, only after being read completely, which
> > first requires the page lock.  So dirtying this page can only happen
> > after the writeback of the fragment was completed.
>
> What I mean is the writeback worker had looked up the dirty pages in
> write_cache_pages() and stored them into a temporary pagevec, then try
> to lock dirty page one by one and write them.
>
> For example, suppose it looked up 2 dirty pages (named page 1 and page
> 2), and writed down page 1 by fuse_writepages_fill(), unlocked page 1.
> Then try to lock page 2.
>
> At the same time, suppose the fuse_fill_write_pages() will write the
> same page 1 and partitail page 2, and it will lock partital page 2 and
> wait for the page 1's writeback is completed. But page 1's writeback can
> not be completed, since the writeback worker is waiting for locking page
> 2, which was already locked by fuse_fill_write_pages().

How would page2 become not uptodate, when it was already collected by
write_cache_pages()?  I.e. page2 is a dirty page, hence it must be
uptodate, and fuse_writepages_fill() will not keep it locked.

Your patch may make sense regardless, but it needs to have a clear
analysis about why the  fuse_wait_on_page_writeback() was needed in
the first place (it's not clear from the history) or why it's okay to
move it.

Thanks,
Miklos