linux-kernel - Re: linux-5.13.2: warning from kernel/rcu/tree

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: <11144384.Jnp629F0a1@natalenko.name>
Date:   Mon, 19 Jul 2021 13:17:07 +0200
From:   Oleksandr Natalenko <oleksandr@...alenko.name>
To:     Boqun Feng <boqun.feng@...il.com>,
        Miaohe Lin <linmiaohe@...wei.com>
Cc:     Zhouyi Zhou <zhouzhouyi@...il.com>, paulmck@...nel.org,
        linux-kernel <linux-kernel@...r.kernel.org>,
        stable@...r.kernel.org, Chris Clayton <chris2553@...glemail.com>,
        Chris Rankin <rankincj@...il.com>,
        Josh Triplett <josh@...htriplett.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        Joel Fernandes <joel@...lfernandes.org>,
        rcu <rcu@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linux-MM <linux-mm@...ck.org>,
        "Huang, Ying" <ying.huang@...el.com>,
        Matthew Wilcox <willy@...radead.org>
Subject: Re: linux-5.13.2: warning from kernel/rcu/tree_plugin.h:359

Hello.

On pondělí 19. července 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:
> >>> 
> >>> struct swap_info_struct *get_swap_device(swp_entry_t entry)
> >>> {
> >>> 
> >>>      struct swap_info_struct *si;
> >>>      unsigned long offset;
> >>>      
> >>>      if (!entry.val)
> >>>      
> >>>              goto out;
> >>>     
> >>>     si = swp_swap_info(entry);
> >>>     if (!si)
> >>>     
> >>>        goto bad_nofile;
> >>>    
> >>>    rcu_read_lock();
> >>>   
> >>>   if (data_race(!(si->flags & SWP_VALID)))
> >>>   
> >>>      goto unlock_out;
> >>>   
> >>>   offset = swp_offset(entry);
> >>>   if (offset >= si->max)
> >>>   
> >>>    goto unlock_out;
> >>>   
> >>>   return si;
> >>> 
> >>> bad_nofile:
> >>>   pr_err("%s: %s%08lx\n", __func__, Bad_file, entry.val);
> >>> 
> >>> out:
> >>>   return NULL;
> >>> 
> >>> unlock_out:
> >>>   rcu_read_unlock();
> >>>   return NULL;
> >>> 
> >>> }
> >>> I guess the function "return si" without a rcu_read_unlock.
> >> 
> >> Yes, but the caller is supposed to call put_swap_device() which
> >> calls rcu_read_unlock().  See commit eb085574a752.
> > 
> > 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
> > 
> > happened:
> > 	do_swap_page():
> > 	  si = get_swap_device():
> > 	    rcu_read_lock();
> > 	  
> > 	  lock_page_or_retry():
> > 	    might_sleep(); // call a sleepable function inside RCU read-side 
c.s.
> > 	    
> > 	    __lock_page_or_retry():
> > 	      wait_on_page_bit_common():
> > 	        schedule():
> > 		  rcu_note_context_switch();
> > 		  // Warn here
> > 	  
> > 	  put_swap_device();
> > 	  
> > 	    rcu_read_unlock();
> > 
> > , which introduced by commit 2799e77529c2a
> 
> 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 
63d8620ecf93 was not pulled. Are you suggesting that 63d8620ecf93 should be 
pulled into the stable kernel as well?

Thanks.

-- 
Oleksandr Natalenko (post-factum)