Re: Patch 'writeback, cgroup: release dying cgwbs by switching attached inodes' leads to kernel crash

[Jan Kara <jack@suse.cz>]

On Thu 15-07-21 09:42:06, Boyang Xue wrote:
> On Thu, Jul 15, 2021 at 7:46 AM Roman Gushchin <guro@fb.com> wrote:
> >
> > On Thu, Jul 15, 2021 at 12:22:28AM +0800, Boyang Xue wrote:
> > > Hi Jan,
> > >
> > > On Wed, Jul 14, 2021 at 5:26 PM Jan Kara <jack@suse.cz> wrote:
> > > >
> > > > On Wed 14-07-21 16:44:33, Boyang Xue wrote:
> > > > > Hi Roman,
> > > > >
> > > > > On Wed, Jul 14, 2021 at 12:12 PM Roman Gushchin <guro@fb.com> wrote:
> > > > > >
> > > > > > On Wed, Jul 14, 2021 at 11:21:12AM +0800, Boyang Xue wrote:
> > > > > > > Hello,
> > > > > > >
> > > > > > > I'm not sure if this is the right place to report this bug, please
> > > > > > > correct me if I'm wrong.
> > > > > > >
> > > > > > > I found kernel-5.14.0-rc1 (built from the Linus tree) crash when it's
> > > > > > > running xfstests generic/256 on ext4 [1]. Looking at the call trace,
> > > > > > > it looks like the bug had been introduced by the commit
> > > > > > >
> > > > > > > c22d70a162d3 writeback, cgroup: release dying cgwbs by switching attached inodes
> > > > > > >
> > > > > > > It only happens on aarch64, not on x86_64, ppc64le and s390x. Testing
> > > > > > > was performed with the latest xfstests, and the bug can be reproduced
> > > > > > > on ext{2, 3, 4} with {1k, 2k, 4k} block sizes.
> > > > > >
> > > > > > Hello Boyang,
> > > > > >
> > > > > > thank you for the report!
> > > > > >
> > > > > > Do you know on which line the oops happens?
> > > > >
> > > > > I was trying to inspect the vmcore with crash utility, but
> > > > > unfortunately it doesn't work.
> > > >
> > > > Thanks for report!  Have you tried addr2line utility? Looking at the oops I
> > > > can see:
> > >
> > > Thanks for the tips!
> > >
> > > It's unclear to me that where to find the required address in the
> > > addr2line command line, i.e.
> > >
> > > addr2line -e /usr/lib/debug/lib/modules/5.14.0-0.rc1.15.bx.el9.aarch64/vmlinux
> > > <what address here?>
> >
> > You can use $nm <vmlinux> to get an address of cleanup_offline_cgwbs_workfn()
> > and then add 0x320.
> 
> Thanks! Hope the following helps:

Thanks for the data! 

> static void cleanup_offline_cgwbs_workfn(struct work_struct *work)
> {
>         struct bdi_writeback *wb;
>         LIST_HEAD(processed);
> 
>         spin_lock_irq(&cgwb_lock);
> 
>         while (!list_empty(&offline_cgwbs)) {
>                 wb = list_first_entry(&offline_cgwbs, struct bdi_writeback,
>                                       offline_node);
>                 list_move(&wb->offline_node, &processed);
> 
>                 /*
>                  * If wb is dirty, cleaning up the writeback by switching
>                  * attached inodes will result in an effective removal of any
>                  * bandwidth restrictions, which isn't the goal.  Instead,
>                  * it can be postponed until the next time, when all io
>                  * will be likely completed.  If in the meantime some inodes
>                  * will get re-dirtied, they should be eventually switched to
>                  * a new cgwb.
>                  */
>                 if (wb_has_dirty_io(wb))
>                         continue;
> 
>                 if (!wb_tryget(wb))  <=== line#679
>                         continue;

Aha, interesting. So it seems we crashed trying to dereference
wb->refcnt->data. So it looks like cgwb_release_workfn() raced with
cleanup_offline_cgwbs_workfn() and percpu_ref_exit() got called from
cgwb_release_workfn() and then cleanup_offline_cgwbs_workfn() called
wb_tryget(). I think the proper fix is to move:

        spin_lock_irq(&cgwb_lock);
        list_del(&wb->offline_node);
        spin_unlock_irq(&cgwb_lock);

in cgwb_release_workfn() to the beginning of that function so that we are
sure even cleanup_offline_cgwbs_workfn() cannot be working with the wb when
it is being released. Roman?

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR