Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 2 Sep 2022 13:34:58 +0800
From:   Yue Hu <zbestahu@gmail.com>
To:     Gao Xiang <hsiangkao@linux.alibaba.com>
Cc:     linux-erofs@lists.ozlabs.org, Chao Yu <chao@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] erofs: fix pcluster use-after-free on UP platforms
Message-ID: <20220902133458.000008b0.zbestahu@gmail.com>
In-Reply-To: <20220902045710.109530-1-hsiangkao@linux.alibaba.com>
References: <20220902045710.109530-1-hsiangkao@linux.alibaba.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Fri,  2 Sep 2022 12:57:10 +0800
Gao Xiang <hsiangkao@linux.alibaba.com> wrote:

> During stress testing with CONFIG_SMP disabled, KASAN reports as below:
> 
> ==================================================================
> BUG: KASAN: use-after-free in __mutex_lock+0xe5/0xc30
> Read of size 8 at addr ffff8881094223f8 by task stress/7789
> [ 3482.258885]
> CPU: 0 PID: 7789 Comm: stress Not tainted 6.0.0-rc1-00002-g0d53d2e882f9 #3
> Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
> Call Trace:
>  <TASK>
> ..
>  __mutex_lock+0xe5/0xc30
> ..
>  z_erofs_do_read_page+0x8ce/0x1560
> ..
>  z_erofs_readahead+0x31c/0x580
> ..
> Freed by task 7787
>  kasan_save_stack+0x1e/0x40
>  kasan_set_track+0x20/0x30
>  kasan_set_free_info+0x20/0x40
>  __kasan_slab_free+0x10c/0x190
>  kmem_cache_free+0xed/0x380
>  rcu_core+0x3d5/0xc90
>  __do_softirq+0x12d/0x389
> [ 3482.295630]
> Last potentially related work creation:
>  kasan_save_stack+0x1e/0x40
>  __kasan_record_aux_stack+0x97/0xb0
>  call_rcu+0x3d/0x3f0
>  erofs_shrink_workstation+0x11f/0x210
>  erofs_shrink_scan+0xdc/0x170
>  shrink_slab.constprop.0+0x296/0x530
>  drop_slab+0x1c/0x70
>  drop_caches_sysctl_handler+0x70/0x80
>  proc_sys_call_handler+0x20a/0x2f0
>  vfs_write+0x555/0x6c0
>  ksys_write+0xbe/0x160
>  do_syscall_64+0x3b/0x90
> 
> The root cause is that erofs_workgroup_unfreeze() doesn't reset
> to orig_val thus it causes a race that the pcluster reuses unexpectedly
> before freeing.
> 
> Since UP platforms are quite rare now, such path becomes unnecessary.
> Let's drop such specific-designed path directly instead.
> 
> Fixes: 73f5c66df3e2 ("staging: erofs: fix `erofs_workgroup_{try_to_freeze, unfreeze}'")
> Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
> ---
>  fs/erofs/internal.h | 29 -----------------------------
>  1 file changed, 29 deletions(-)
> 
> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
> index cfee49d33b95..a01cc82795a2 100644
> --- a/fs/erofs/internal.h
> +++ b/fs/erofs/internal.h
> @@ -195,7 +195,6 @@ struct erofs_workgroup {
>  	atomic_t refcount;
>  };
>  
> -#if defined(CONFIG_SMP)
>  static inline bool erofs_workgroup_try_to_freeze(struct erofs_workgroup *grp,
>  						 int val)
>  {
> @@ -224,34 +223,6 @@ static inline int erofs_wait_on_workgroup_freezed(struct erofs_workgroup *grp)
>  	return atomic_cond_read_relaxed(&grp->refcount,
>  					VAL != EROFS_LOCKED_MAGIC);
>  }
> -#else
> -static inline bool erofs_workgroup_try_to_freeze(struct erofs_workgroup *grp,
> -						 int val)
> -{
> -	preempt_disable();
> -	/* no need to spin on UP platforms, let's just disable preemption. */
> -	if (val != atomic_read(&grp->refcount)) {
> -		preempt_enable();
> -		return false;
> -	}
> -	return true;
> -}
> -
> -static inline void erofs_workgroup_unfreeze(struct erofs_workgroup *grp,
> -					    int orig_val)
> -{
> -	preempt_enable();
> -}
> -
> -static inline int erofs_wait_on_workgroup_freezed(struct erofs_workgroup *grp)
> -{
> -	int v = atomic_read(&grp->refcount);
> -
> -	/* workgroup is never freezed on uniprocessor systems */
> -	DBG_BUGON(v == EROFS_LOCKED_MAGIC);
> -	return v;
> -}
> -#endif	/* !CONFIG_SMP */
>  #endif	/* !CONFIG_EROFS_FS_ZIP */
>  

Reviewed-by: Yue Hu <huyue2@coolpad.com>

>  /* we strictly follow PAGE_SIZE and no buffer head yet */