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;
From:   Uladzislau Rezki <urezki@gmail.com>
Date:   Thu, 30 Mar 2023 14:07:52 +0200
To:     =?utf-8?B?5Luj5a2Q5Li6IChaaXdlaSBEYWkp?= <Ziwei.Dai@unisoc.com>
Cc:     "paulmck@kernel.org" <paulmck@kernel.org>,
        "frederic@kernel.org" <frederic@kernel.org>,
        "quic_neeraju@quicinc.com" <quic_neeraju@quicinc.com>,
        "josh@joshtriplett.org" <josh@joshtriplett.org>,
        "rostedt@goodmis.org" <rostedt@goodmis.org>,
        "mathieu.desnoyers@efficios.com" <mathieu.desnoyers@efficios.com>,
        "jiangshanlai@gmail.com" <jiangshanlai@gmail.com>,
        "joel@joelfernandes.org" <joel@joelfernandes.org>,
        "rcu@vger.kernel.org" <rcu@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        =?utf-8?B?546L5Y+MIChTaHVhbmcgV2FuZyk=?= <shuang.wang@unisoc.com>,
        =?utf-8?B?6L6b5L6d5YehIChZaWZhbiBYaW4p?= <Yifan.Xin@unisoc.com>,
        =?utf-8?B?546L56eRIChLZSBXYW5nKQ==?= <Ke.Wang@unisoc.com>,
        =?utf-8?B?6Zer5a2m5paHIChYdWV3ZW4gWWFuKQ==?= 
        <Xuewen.Yan@unisoc.com>,
        =?utf-8?B?54mb5b+X5Zu9IChaaGlndW8gTml1KQ==?= 
        <Zhiguo.Niu@unisoc.com>,
        =?utf-8?B?6buE5pyd6ZizIChaaGFveWFuZyBIdWFuZyk=?= 
        <zhaoyang.huang@unisoc.com>
Subject: Re: =?utf-8?B?562U5aSNOiBbUEFUQ0hdIHJjdTog?=
 =?utf-8?Q?Make_sure_new_krc?= =?utf-8?Q?p?= free business is handled after
 the wanted rcu grace period.
Message-ID: <ZCV7mPHkSUeFXDPP@pc636>
References: <1680168440-20511-1-git-send-email-ziwei.dai@unisoc.com>
 <76db405712174b20a1caef47acd1cda7@BJMBX01.spreadtrum.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <76db405712174b20a1caef47acd1cda7@BJMBX01.spreadtrum.com>
Precedence: bulk

Hello, Ziwei Dai!

> Hi Uladzislau and all,
> 
> Sorry for the disclaimer in the original mail.
> Please help comment in this new thread.
> 
> We found this issue at K5.15. We try to fix this issue on K5.15.
> It seems mainline also has this issue.
> 
> Below is the first debug patch on k5.15 device, which is under stress test, issue not reproduce so far.
> ============================================================
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 66951e130c2fc..44759641f7234 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -3342,15 +3342,21 @@ static void kfree_rcu_monitor(struct work_struct *work)
>         // Attempt to start a new batch.
>         for (i = 0; i < KFREE_N_BATCHES; i++) {
>                 struct kfree_rcu_cpu_work *krwp = &(krcp->krw_arr[i]);
> +               bool rcu_work_pending;
> 
>                 // Try to detach bkvhead or head and attach it over any
>                 // available corresponding free channel. It can be that
>                 // a previous RCU batch is in progress, it means that
>                 // immediately to queue another one is not possible so
>                 // in that case the monitor work is rearmed.
> -               if ((krcp->bkvhead[0] && !krwp->bkvhead_free[0]) ||
> -                       (krcp->bkvhead[1] && !krwp->bkvhead_free[1]) ||
> -                               (krcp->head && !krwp->head_free)) {
> +               rcu_work_pending = test_bit(
> +                       WORK_STRUCT_PENDING_BIT,
> +                       work_data_bits(&krwp->rcu_work.work));
> +               // If there is on-going rcu work, continue.
> +               if (rcu_work_pending || krwp->bkvhead_free[0] ||
> +                       krwp->bkvhead_free[1] || krwp->head_free)
> +                       continue;
> +               if (krcp->bkvhead[0] || krcp->bkvhead[1] || krcp->head) {
>                         // Channel 1 corresponds to the SLAB-pointer bulk path.
>                         // Channel 2 corresponds to vmalloc-pointer bulk path.
>                         for (j = 0; j < FREE_N_CHANNELS; j++) {
> 
> As " rcu_work_pending" judgement seems redundant, I made the second patch below on k5.15. We will make stress test.
> ============================================================
> Below is the first debug patch on k5.15 device, which is under stress test, issue not reproduce so far.
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 66951e130c2fc..f219c60a8ec30 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -3343,14 +3343,13 @@ static void kfree_rcu_monitor(struct work_struct *work)
>         for (i = 0; i < KFREE_N_BATCHES; i++) {
>                 struct kfree_rcu_cpu_work *krwp = &(krcp->krw_arr[i]);
> 
> -               // Try to detach bkvhead or head and attach it over any
> -               // available corresponding free channel. It can be that
> -               // a previous RCU batch is in progress, it means that
> -               // immediately to queue another one is not possible so
> -               // in that case the monitor work is rearmed.
> -               if ((krcp->bkvhead[0] && !krwp->bkvhead_free[0]) ||
> -                       (krcp->bkvhead[1] && !krwp->bkvhead_free[1]) ||
> -                               (krcp->head && !krwp->head_free)) {
> +               // Try to detach bulk_head or head and attach it, only when
> +               // all channels are free.  Any channel is not free means at krwp
> +               // there is on-going rcu work to handle krwp's free business.
> +               if (krwp->bkvhead_free[0] || krwp->bkvhead_free[1] ||
> +                       krwp->head_free)
> +                       continue;
> +               if (krcp->bkvhead[0] || krcp->bkvhead[1] || krcp->head) {
>                         // Channel 1 corresponds to the SLAB-pointer bulk path.
>                         // Channel 2 corresponds to vmalloc-pointer bulk path.
>                         for (j = 0; j < FREE_N_CHANNELS; j++) {
> 
> 
> > -----邮件原件-----
> > 发件人: 代子为 (Ziwei Dai) <Ziwei.Dai@unisoc.com>
> > 发送时间: 2023年3月30日 17:27
> > 收件人: paulmck@kernel.org; frederic@kernel.org;
> > quic_neeraju@quicinc.com; josh@joshtriplett.org; rostedt@goodmis.org;
> > mathieu.desnoyers@efficios.com; jiangshanlai@gmail.com;
> > joel@joelfernandes.org; rcu@vger.kernel.org
> > 抄送: linux-kernel@vger.kernel.org; 王双 (Shuang Wang)
> > <shuang.wang@unisoc.com>; 辛依凡 (Yifan Xin) <Yifan.Xin@unisoc.com>;
> > 王科 (Ke Wang) <Ke.Wang@unisoc.com>; 闫学文 (Xuewen Yan)
> > <Xuewen.Yan@unisoc.com>; 牛志国 (Zhiguo Niu) <Zhiguo.Niu@unisoc.com>;
> > 代子为 (Ziwei Dai) <Ziwei.Dai@unisoc.com>; 黄朝阳 (Zhaoyang Huang)
> > <zhaoyang.huang@unisoc.com>
> > 主题: [PATCH] rcu: Make sure new krcp free business is handled after the
> > wanted rcu grace period.
> > 
> > From: 代子为 (Ziwei Dai) <ziwei.dai@ziwei-lenovo.spreadtrum.com>
> > 
> > In kfree_rcu_monitor(), new free business at krcp is attached to any free
> > channel at krwp. kfree_rcu_monitor() is responsible to make sure new free
> > business is handled after the rcu grace period. But if there is any none-free
> > channel at krwp already, that means there is an on-going rcu work, which will
> > cause the kvfree_call_rcu()-triggered free business is done before the wanted
> > rcu grace period ends.
> > 
> > This commit ignores krwp which has non-free channel at kfree_rcu_monitor(),
> > to fix the issue that kvfree_call_rcu() loses effectiveness.
> > 
> > Below is the css_set obj "from_cset" use-after-free issue caused by
> > kvfree_call_rcu() losing effectiveness.
> > Core 0 calls rcu_read_lock(), then use "from_cset", then hard irq comes.
> > Core 1 calls kfree_rcu(cset, rcu_head), willing to free "from_cset" after new
> > gp.
> > Core 2 frees "from_cset" after current gp end. "from_cset" is reallocated.
> > Core 0 references "from_cset"'s member, which causes crash.
> > 
> > Core 0					Core 1				       	Core 2
> > count_memcg_event_mm()
> > |rcu_read_lock()  <---
> > |mem_cgroup_from_task()
> >  |// <css_set ptr> is the "from_cset" mentioned on core 1  |<css_set ptr> =
> > rcu_dereference((task)->cgroups)  |// Hard irq comes, current task is
> > scheduled out.
> > 
> > 			Core 1:
> > 			cgroup_attach_task()
> > 			|cgroup_migrate()
> > 			 |cgroup_migrate_execute()
> > 			  |css_set_move_task(task, from_cset, to_cset, true)
> > 			  |cgroup_move_task(task, to_cset)
> > 			   |rcu_assign_pointer(.., to_cset)
> > 			   |...
> > 			|cgroup_migrate_finish()
> > 			 |put_css_set_locked(from_cset)
> > 			  |from_cset->refcount return 0
> > 			  |kfree_rcu(cset, rcu_head) <--- means to free from_cset
> > after new gp
> > 			   |add_ptr_to_bulk_krc_lock()
> > 			   |schedule_delayed_work(&krcp->monitor_work, ..)
> > 
> > 			kfree_rcu_monitor()
> > 			|krcp->bulk_head[0]'s work attached to
> > krwp->bulk_head_free[]
> > 			|queue_rcu_work(system_wq, &krwp->rcu_work)
> > 			 |if rwork->rcu.work is not in WORK_STRUCT_PENDING_BIT
> > state,
> > 			 |call_rcu(&rwork->rcu, rcu_work_rcufn) <--- request a new gp
> > 
> > 								// There is a perious call_rcu(..,
> > rcu_work_rcufn)
> > 								// gp end, rcu_work_rcufn() is called.
> > 								rcu_work_rcufn()
> > 								|__queue_work(.., rwork->wq,
> > &rwork->work);
> > 								Core 2：
> > 								// or there is a pending
> > kfree_rcu_work() work called.
> > 								|kfree_rcu_work()
> > 								|krwp->bulk_head_free[0] bulk is
> > freed before new gp end!!!
> > 								|The "from_cset" mentioned on core
> > 1 is freed before new gp end.
> > Core 0:
> > // the task is schedule in after many ms.
> >  |<css_set ptr>->subsys[(subsys_id) <--- caused kernel crash, because
> > <css_set ptr>="from_cset" is freed.
> > 
> > Signed-off-by: Ziwei Dai <ziwei.dai@unisoc.com>
> > 
> > :#	modified:   tree.c
> > ---
> >  kernel/rcu/tree.c | 19 ++++++++++---------
> >  1 file changed, 10 insertions(+), 9 deletions(-)
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c index 8e880c0..f6451a8
> > 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -3107,15 +3107,16 @@ static void kfree_rcu_monitor(struct
> > work_struct *work)
> >  	for (i = 0; i < KFREE_N_BATCHES; i++) {
> >  		struct kfree_rcu_cpu_work *krwp = &(krcp->krw_arr[i]);
> > 
> > -		// Try to detach bulk_head or head and attach it over any
> > -		// available corresponding free channel. It can be that
> > -		// a previous RCU batch is in progress, it means that
> > -		// immediately to queue another one is not possible so
> > -		// in that case the monitor work is rearmed.
> > -		if ((!list_empty(&krcp->bulk_head[0]) &&
> > list_empty(&krwp->bulk_head_free[0])) ||
> > -			(!list_empty(&krcp->bulk_head[1]) &&
> > list_empty(&krwp->bulk_head_free[1])) ||
> > -				(READ_ONCE(krcp->head) && !krwp->head_free)) {
> > -
> > +		// Try to detach bulk_head or head and attach it, only when
> > +		// all channels are free.  Any channel is not free means at krwp
> > +		// there is on-going rcu work to handle krwp's free business.
> > +		if (!list_empty(&krwp->bulk_head_free[0]) ||
> > +			!list_empty(&krwp->bulk_head_free[1]) ||
> > +				krwp->head_free)
> > +			continue;
Can we replace it with a new helper, for example: 

+static bool
+can_krcw_offload(struct kfree_rcu_cpu_work *krwp)
+{
+       int i;
+
+       for (i = 0; i < FREE_N_CHANNELS; i++)
+               if (!list_empty(&krwp->bulk_head_free[i]))
+                       return false;
+
+       return (READ_ONCE(krwp->head_free) == NULL);
+}

and then

+               if (!can_krcw_offload(krwp))
                        continue;

>
> > +		if (!list_empty(&krcp->bulk_head[0]) ||
> > +			!list_empty(&krcp->bulk_head[1]) ||
> > +			READ_ONCE(krcp->head)) {
>
Can we replace it with the:

+               // kvfree_rcu_drain_ready() might handle this krcp, if so give up.
+               if (need_offload_krc(krcp)) {

?
because we have a helper that is need_offload_krcp() that does the same.

> >  			// Channel 1 corresponds to the SLAB-pointer bulk path.
> >  			// Channel 2 corresponds to vmalloc-pointer bulk path.
> >  			for (j = 0; j < FREE_N_CHANNELS; j++) {
> > --
> > 1.9.1
> 

Also we need to add Fixes tag, you say you are on 5.15 kernel. So
something was added there. I will try to double check.

And thank you for fixing it!!!

--
Uladzislau Rezki