Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Oleksandr Natalenko <oleksandr@natalenko.name>
To:     Boqun Feng <boqun.feng@gmail.com>,
        Miaohe Lin <linmiaohe@huawei.com>
Cc:     Zhouyi Zhou <zhouzhouyi@gmail.com>, paulmck@kernel.org,
        linux-kernel <linux-kernel@vger.kernel.org>,
        stable@vger.kernel.org, Chris Clayton <chris2553@googlemail.com>,
        Chris Rankin <rankincj@gmail.com>,
        Josh Triplett <josh@joshtriplett.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        Lai Jiangshan <jiangshanlai@gmail.com>,
        Joel Fernandes <joel@joelfernandes.org>,
        rcu <rcu@vger.kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Linux-MM <linux-mm@kvack.org>,
        "Huang, Ying" <ying.huang@intel.com>,
        Matthew Wilcox <willy@infradead.org>
Subject: Re: linux-5.13.2: warning from kernel/rcu/tree_plugin.h:359
Date:   Mon, 19 Jul 2021 13:17:07 +0200
Message-ID: <11144384.Jnp629F0a1@natalenko.name>
In-Reply-To: <08803f78-3e99-6b3f-e809-5828fe47cf06@huawei.com>
References: <c9fd1311-662c-f993-c8ef-54af036f2f78@googlemail.com> <YPVQfaamqwu1PRrK@boqun-archlinux> <08803f78-3e99-6b3f-e809-5828fe47cf06@huawei.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hello.

On pond=C4=9Bl=C3=AD 19. =C4=8Dervence 2021 13:12:58 CEST Miaohe Lin wrote:
> On 2021/7/19 18:14, Boqun Feng wrote:
> > On Mon, Jul 19, 2021 at 03:43:00AM +0100, Matthew Wilcox wrote:
> >> On Mon, Jul 19, 2021 at 10:24:18AM +0800, Zhouyi Zhou wrote:
> >>> Meanwhile, I examined the 5.12.17 by naked eye, and found a suspicious
> >>> place that could possibly trigger that problem:
> >>>=20
> >>> struct swap_info_struct *get_swap_device(swp_entry_t entry)
> >>> {
> >>>=20
> >>>      struct swap_info_struct *si;
> >>>      unsigned long offset;
> >>>     =20
> >>>      if (!entry.val)
> >>>     =20
> >>>              goto out;
> >>>    =20
> >>>     si =3D swp_swap_info(entry);
> >>>     if (!si)
> >>>    =20
> >>>        goto bad_nofile;
> >>>   =20
> >>>    rcu_read_lock();
> >>>  =20
> >>>   if (data_race(!(si->flags & SWP_VALID)))
> >>>  =20
> >>>      goto unlock_out;
> >>>  =20
> >>>   offset =3D swp_offset(entry);
> >>>   if (offset >=3D si->max)
> >>>  =20
> >>>    goto unlock_out;
> >>>  =20
> >>>   return si;
> >>>=20
> >>> bad_nofile:
> >>>   pr_err("%s: %s%08lx\n", __func__, Bad_file, entry.val);
> >>>=20
> >>> out:
> >>>   return NULL;
> >>>=20
> >>> unlock_out:
> >>>   rcu_read_unlock();
> >>>   return NULL;
> >>>=20
> >>> }
> >>> I guess the function "return si" without a rcu_read_unlock.
> >>=20
> >> Yes, but the caller is supposed to call put_swap_device() which
> >> calls rcu_read_unlock().  See commit eb085574a752.
> >=20
> > Right, but we need to make sure there is no sleepable function called
> > before put_swap_device() called, and the call trace showed the following
> >=20
> > happened:
> > 	do_swap_page():
> > 	  si =3D get_swap_device():
> > 	    rcu_read_lock();
> > 	 =20
> > 	  lock_page_or_retry():
> > 	    might_sleep(); // call a sleepable function inside RCU read-side=20
c.s.
> > 	   =20
> > 	    __lock_page_or_retry():
> > 	      wait_on_page_bit_common():
> > 	        schedule():
> > 		  rcu_note_context_switch();
> > 		  // Warn here
> > 	 =20
> > 	  put_swap_device();
> > 	 =20
> > 	    rcu_read_unlock();
> >=20
> > , which introduced by commit 2799e77529c2a
>=20
> When in the commit 2799e77529c2a, we're using the percpu_ref to serialize
> against concurrent swapoff, i.e. there's percpu_ref inside
> get_swap_device() instead of rcu_read_lock(). Please see commit
> 63d8620ecf93 ("mm/swapfile: use percpu_ref to serialize against concurrent
> swapoff") for detail.

The problem here is that 2799e77529c2a got pulled into stable, but=20
63d8620ecf93 was not pulled. Are you suggesting that 63d8620ecf93 should be=
=20
pulled into the stable kernel as well?

Thanks.

=2D-=20
Oleksandr Natalenko (post-factum)