Received-SPF: pass (google.com: domain of linux-kernel+bounces-129641-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240401081734.1433755-1-zhaoyang.huang@unisoc.com>
 <736b982a-57c9-441a-812c-87cdee2e096e@redhat.com> <CAGWkznF6NQrB-vwBZCfCF-1WQJv8iwPPrwND7yDsPJw1EbfxQA@mail.gmail.com>
 <2f8af9d1-8c8e-4e1c-a794-76f6bb287b08@redhat.com>
In-Reply-To: <2f8af9d1-8c8e-4e1c-a794-76f6bb287b08@redhat.com>
From: Zhaoyang Huang <huangzhaoyang@gmail.com>
Date: Wed, 3 Apr 2024 19:08:11 +0800
Message-ID: <CAGWkznEcS7rNWMudMBA7e-bjudX-sR4TGofyB5G1E=OmW_0Ckw@mail.gmail.com>
Subject: Re: [PATCHv2 1/1] mm: fix unproperly folio_put by changing API in read_pages
To: David Hildenbrand <david@redhat.com>
Cc: "zhaoyang.huang" <zhaoyang.huang@unisoc.com>, Andrew Morton <akpm@linux-foundation.org>, 
	NeilBrown <neilb@suse.de>, linux-mm@kvack.org, linux-kernel@vger.kernel.org, 
	steve.kang@unisoc.com, Matthew Wilcox <willy@infradead.org>, 
	Christoph Hellwig <hch@infradead.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Apr 3, 2024 at 4:01=E2=80=AFPM David Hildenbrand <david@redhat.com>=
 wrote:
>
> On 03.04.24 07:50, Zhaoyang Huang wrote:
> > On Tue, Apr 2, 2024 at 8:58=E2=80=AFPM David Hildenbrand <david@redhat.=
com> wrote:
> >>
> >> On 01.04.24 10:17, zhaoyang.huang wrote:
> >>> From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> >>>
> >>> An VM_BUG_ON in step 9 of [1] could happen as the refcnt is dropped
> >>> unproperly during the procedure of read_pages()->readahead_folio->fol=
io_put.
> >>> This is introduced by commit 9fd472af84ab ("mm: improve cleanup when
> >>> ->readpages doesn't process all pages")'.
> >>>
> >>> key steps of[1] in brief:
> >>> 2'. Thread_truncate get folio to its local fbatch by find_get_entry i=
n step 2
> >>> 7'. Last refcnt remained which is not as expect as from alloc_pages
> >>>       but from thread_truncate's local fbatch in step 7
> >>> 8'. Thread_reclaim succeed to isolate the folio by the wrong refcnt(n=
ot
> >>>       the value but meaning) in step 8
> >>> 9'. Thread_truncate hit the VM_BUG_ON in step 9
> >>>
> >>> [1]
> >>> Thread_readahead:
> >>> 0. folio =3D filemap_alloc_folio(gfp_mask, 0);
> >>>          (refcount 1: alloc_pages)
> >>> 1. ret =3D filemap_add_folio(mapping, folio, index + i, gfp_mask);
> >>>          (refcount 2: alloc_pages, page_cache)
>
> [not going into all details, just a high-level remark]
>
> page_cache_ra_unbounded() does a filemap_invalidate_lock_shared(), which
> is a down_read_trylock(&mapping->invalidate_lock).
>
> That is, all read_pages() calls in mm/readahead.c happen under
> mapping->invalidate_lock in read mode.
>
> ... and ...
>
> >>>
> >>> Thread_truncate:
> >>> 2. folio =3D find_get_entries(&fbatch_truncate);
> >>>          (refcount 3: alloc_pages, page_cache, fbatch_truncate))
>
> truncation, such as truncate_inode_pages() must be called under
> mapping->invalidate_lock held in write mode. So naive me would have
> thought that readahead and truncate cannot race in that way.
>
> [...]
>
Thanks for the reminder. But I don't find the spot where holding
"mapping->invalidate_lock" by check the callstack of
'kill_bdev()->truncate_inode_pages()->truncate_inode_pages_range()',
or the lock is holded beyond this?
>
> >>
> >> Something that would help here is an actual reproducer that triggersth=
is
> >> issue.
> >>
> >> To me, it's unclear at this point if we are talking about an actual
> >> issue or a theoretical issue?
> > Thanks for feedback. Above callstack is a theoretical issue so far
> > which is arised from an ongoing analysis of a practical livelock issue
> > generated by folio_try_get_rcu which is related to abnormal folio
> > refcnt state. So do you think this callstack makes sense?
>
> I'm not an expert on that code, and only spent 5 min looking into the
> code. So my reasoning about invalidate_lock above might be completely wro=
ng.
>
> It would be a very rare race that was not reported so far in practice.
> And it certainly wouldn't be the easiest one to explain, because the
> call chain above is a bit elaborate and does not explain which locks are
> involved and how they fail to protect us from any such race.
>
> For this case in particular, I think we really need a real reproducer to
> convince people that the actual issue does exist and the fix actually
> resolves the issue.
Sorry, it is theoretically yet according to my understanding.
>
> --
> Cheers,
>
> David / dhildenb
>