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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 15 Apr 2021 20:30:40 +0800
From:   Peng Tao <bergwolf@...il.com>
To:     Miklos Szeredi <miklos@...redi.hu>
Cc:     Baolin Wang <baolin.wang@...ux.alibaba.com>,
        Peng Tao <tao.peng@...ux.alibaba.com>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        Linux Kernel Mailing List <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 9:20 PM Miklos Szeredi <miklos@...redi.hu> wrote:
>
> On Wed, Apr 14, 2021 at 2:22 PM Peng Tao <bergwolf@...il.com> wrote:
> >
>
> > > --- a/fs/fuse/file.c
> > > +++ b/fs/fuse/file.c
> > > @@ -1117,17 +1117,12 @@ static ssize_t fuse_send_write_pages(str
> > >       count = ia->write.out.size;
> > >       for (i = 0; i < ap->num_pages; i++) {
> > >               struct page *page = ap->pages[i];
> > > +             bool page_locked = ap->page_locked && (i == ap->num_pages - 1);
> > Any reason for just handling the last locked page in the page array?
> > To be specific, it look like the first page in the array can also be
> > partial dirty and locked?
>
> In that case the first partial page will be locked, and it'll break
> out of the loop...
>
> > >
> > > -             if (!err && !offset && count >= PAGE_SIZE)
> > > -                     SetPageUptodate(page);
> > > -
> > > -             if (count > PAGE_SIZE - offset)
> > > -                     count -= PAGE_SIZE - offset;
> > > -             else
> > > -                     count = 0;
> > > -             offset = 0;
> > > -
> > > -             unlock_page(page);
> > > +             if (err)
> > > +                     ClearPageUptodate(page);
> > > +             if (page_locked)
> > > +                     unlock_page(page);
> > >               put_page(page);
> > >       }
> > >
> > > @@ -1191,6 +1186,16 @@ static ssize_t fuse_fill_write_pages(str
> > >               if (offset == PAGE_SIZE)
> > >                       offset = 0;
> > >
> > > +             /* If we copied full page, mark it uptodate */
> > > +             if (tmp == PAGE_SIZE)
> > > +                     SetPageUptodate(page);
> > > +
> > > +             if (PageUptodate(page)) {
> > > +                     unlock_page(page);
> > > +             } else {
> > > +                     ap->page_locked = true;
> > > +                     break;
>
> ... here, and send it as a separate WRITE request.
>
> So the multi-page case with a partial & non-uptodate head page will
> always result in the write request being split into two (even if
> there's no partial tail page).

Ah, good point! Thanks for the explanation. I agree that it can fix
the deadlock issue here.

One thing I'm still uncertain about is that fuse used to fill the
page, wait for page writeback, and send it to userspace all with the
page locked, which is kind of like a stable page mechanism for FUSE.
With the above change, we no longer lock a PG_uptodate page when
waiting for its writeback and sending it to userspace. Then the page
can be modified when being sent to userspace. Is it acceptable?

Cheers,
Tao
-- 
Into Sth. Rich & Strange