Received-SPF: pass (google.com: domain of linux-kernel+bounces-154393-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 23 Apr 2024 09:26:34 +0800
From: Yujie Liu <yujie.liu@intel.com>
To: Chen Yu <yu.c.chen@intel.com>
CC: Xuewen Yan <xuewen.yan@unisoc.com>, <peterz@infradead.org>,
	<mingo@redhat.com>, <juri.lelli@redhat.com>, <vincent.guittot@linaro.org>,
	<dietmar.eggemann@arm.com>, <rostedt@goodmis.org>, <bsegall@google.com>,
	<mgorman@suse.de>, <bristot@redhat.com>, <vschneid@redhat.com>,
	<ke.wang@unisoc.com>, <xuewen.yan94@gmail.com>, <di.shen@unisoc.com>,
	<linux-kernel@vger.kernel.org>, Sergei Trofimovich <slyich@gmail.com>, "Breno
 Leitao" <leitao@debian.org>, Igor Raits <igor@gooddata.com>, Tim Chen
	<tim.c.chen@intel.com>
Subject: Re: [PATCH v2] sched/eevdf: Prevent vlag from going out of bounds
 when reweight_eevdf
Message-ID: <ZicOSiEWHJJcahi/@yujie-X299>
References: <20240422082238.5784-1-xuewen.yan@unisoc.com>
 <ZiYkI4qn2nvdmktq@chenyu5-mobl2>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ZiYkI4qn2nvdmktq@chenyu5-mobl2>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?sUd2EqudohJ72j/AETG/7L6WsbyRyseo7i2IsOSFDYL+4jN7XejZq0cGShMv?=
 =?us-ascii?Q?tVS2uh3zxZfmtklKp4wWKCuP1WvkRCF9Qx+/VCJOSFARvUvTeKoUTnAXvreC?=
 =?us-ascii?Q?bjm7WpFJDUpmfDgI3j5WVySX4g6KUAPSiXrkG7hb5iiWJvPM22WZb/oQtnQX?=
 =?us-ascii?Q?S6Ie455YicMFd3vxy5iNb8+/f7o+/OABZUFC2PEhdFz+P1gJRu0pPWiqF1u2?=
 =?us-ascii?Q?Bj+ZoSkZ1enGKlorUKQjF0lK0e8dDLn34sjj/0r1G1cb1E311a39Ypggyqe1?=
 =?us-ascii?Q?kWqWqJAQm/Zx+KHpcI2gM4f0OvsDlxfPmILqbib7CVo33OyZThm3xoDzXkuc?=
 =?us-ascii?Q?VKevHXkhy0JVXlbgaeXqqVx157xtgx2R6H/dtkMRf6V1akccn5jc9VbssMbB?=
 =?us-ascii?Q?8T4qdhktPxEsSy8juWh2znBfatr3CYsJomMEHHJDHBJsAzjpItrBktgkYZ7c?=
 =?us-ascii?Q?AnUpIxdu9G7bPlEbfGavrkmE2zYfdRssF0qr8xtu1d4Rsclkvw5CCTkj+UI9?=
 =?us-ascii?Q?r5rZfX6Qv8A0e6VJ/7BS1LMMO8C/yeMPBVmqAvPlWEwBw4/LBHaatNjPw97m?=
 =?us-ascii?Q?kyOIDW6V4HoP6UgnV0a8doJSg0HXN1sG+h4tQRpbleE9zz/ZlKAbNuyjXPp1?=
 =?us-ascii?Q?GzXpg9xMsGzUWRRMb5vRmOMslLIeaz6tVcjJB1dN6fLkhVtAsjJFuTf/Cdld?=
 =?us-ascii?Q?EPAvPz/32jlqhV9pAcDmaNq4YYkom9fRmpSrLOrtHJEER2dTnqgniOwFW/YJ?=
 =?us-ascii?Q?1ySwiCLW3aOXmaiNBcRkFdXF6lDmUmh2UiQQW4bj19bf2ak7H67BZCg48geE?=
 =?us-ascii?Q?Cl6gWkqCsFdMcuMxkD5BnxvB0PncQoacUZDdTtGxzzFDSeT6HbIuy6c4Kx33?=
 =?us-ascii?Q?+ay9HZEQ9BCEf8slAohYgT5CbCaJo7ov/JZRWLqLaCAHZlLH0XAtaH1sqN8n?=
 =?us-ascii?Q?MwTGmLBjRG0D3GcVByQybcOsMciOuMGot5MxF+5TzPVmrAjOF0YlLRycsh24?=
 =?us-ascii?Q?o+IC3+UUuMegO5hWVhMZRhguDqp/E7tqPDiFJjd6yssSwbsZbCPA4ZWEAAJZ?=
 =?us-ascii?Q?NBcvKMD032ozvqJZQSWapBYjha9B3ZN5Q8orlkPSEOsw9kKNED3KwhVKbf6J?=
 =?us-ascii?Q?Q0/aT8TrSw0w8cZMQbHgO6Bd4aso2UQwBOqdzteux9IqZrl/kVSK6WmE3M4/?=
 =?us-ascii?Q?6M/pwLGa81/j52cT+0Iomr1H2+ZyzefisQCc2BZFEyCZ7TcM/N6YSf8Jn6/v?=
 =?us-ascii?Q?vC5Wz70WNG8MnslIKtUyw7V3K5YZp56sTavbp07lNFcxWxNxQrc0bxp6h2+b?=
 =?us-ascii?Q?wiz3MLyXjPs399h8nG3XtnG0/iajmm0SBbfgKrYZdzvyuplxBo6TjNuZ1xA7?=
 =?us-ascii?Q?eK0pVsxti7vpB9Wz1jwTDN5HggdiMohczFxCyIeDC0D2f7pWphO4YQDwMRDE?=
 =?us-ascii?Q?v67bbDSFzzG6b5LAg90Sn7+B2TVs6vQ3HwSVQhOrdC8YvM1/dWmPEZkcQf7U?=
 =?us-ascii?Q?aAFtwHb1isEE/5bmSC5fxEHp5yBWBW9uPMsn9RCDRYJ5rTIMM1Ctud33s/bm?=
 =?us-ascii?Q?nmSu6iJhuEp/nGhHNbuja8c9f15OVl7P7R8PJJZP?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 0f4234ab-e712-463e-8a5b-08dc633555bd
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB8393.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Apr 2024 01:33:10.5543
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 8TZbFkCbefBRrTD8wXU70OPlYSZTrn7Y4Yai1TaKMNGlOHI7g6JOx1ZBEFXLuEkSHkHt3UpUA6Zyyy9gAJePGg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR11MB6258
X-OriginatorOrg: intel.com

On Mon, Apr 22, 2024 at 04:47:31PM +0800, Chen Yu wrote:
> On 2024-04-22 at 16:22:38 +0800, Xuewen Yan wrote:
> > kernel encounters the following error when running workload:
> > 
> > BUG: kernel NULL pointer dereference, address: 0000002c
> > EIP: set_next_entity (fair.c:?)
> > 
> > which was caused by NULL pointer returned by pick_eevdf().
> > 
> > Further investigation has shown that, the entity_eligible() has an
> > false-negative issue when the entity's vruntime is far behind the
> > cfs_rq.min_vruntime that, the (vruntime - cfs_rq->min_vruntime) * load
> > caused a s64 overflow, thus every entity on the rb-tree is not
> > eligible, which results in a NULL candidate.
> > 
> > The reason why entity's vruntime is far behind the cfs_rq.min_vruntime
> > is because during a on_rq task group's update_cfs_group()->reweight_eevdf(),
> > there is no limit on the new entity's vlag. If the new weight is much
> > smaller than the old one,
> > 
> > vlag = div_s64(vlag * old_weight, weight)
> > 
> > generates a huge vlag, and results in very small(negative) vruntime.
> > 
> > Thus limit the range of vlag accordingly.
> >
> 
> Thanks for the fix.
> 
> Might also add comments from Tim suggested here:
> https://lore.kernel.org/lkml/ec479251e6245148b89b226f734192f6d5343bbc.camel@linux.intel.com/
> 
> A fix tag might be needed.
> Fixes: eab03c23c2a1 ("sched/eevdf: Fix vruntime adjustment on reweight")
>  
> > Reported-by: Sergei Trofimovich <slyich@gmail.com>
> > Closes: https://lore.kernel.org/all/ZhuYyrh3mweP_Kd8@nz.home/
> > Reported-by: Igor Raits <igor@gooddata.com>
> > Closes: https://lore.kernel.org/all/CA+9S74ih+45M_2TPUY_mPPVDhNvyYfy1J1ftSix+KjiTVxg8nw@mail.gmail.com/
> > Reported-by: Breno Leitao <leitao@debian.org>
> > Reported-by: kernel test robot <oliver.sang@intel.com>
> > Closes: https://lore.kernel.org/lkml/202401301012.2ed95df0-oliver.sang@intel.com/
> > Reported-by: Yujie Liu <yujie.liu@intel.com>
> > Signed-off-by: Xuewen Yan <xuewen.yan@unisoc.com>
> > ---
> 
> Cced Sergei, Igor, Breno who have encountered this NULL pointer issue before.
> 
> From my testing, with this applied I did not see the NULL pointer exception
> after running trinity for 100 cycles, so
> 
> Reviewed-and-tested-by: Chen Yu <yu.c.chen@intel.com>
> 
> thanks,
> Chenyu
> 

From 0-Day testing, with this patch applied on v6.9-rc5, we no longer
see the NULL pointer issue in 999 cycles of trinity test.

Tested-by: Yujie Liu <yujie.liu@intel.com>

=========================================================================================
tbox_group/testcase/rootfs/kconfig/compiler/runtime/group/nr_groups:
  vm-snb/trinity/debian-11.1-i386-20220923.cgz/i386-randconfig-004-20240122/clang-17/300s/group-03/5

commit:
  v6.9-rc5
  v6.9-rc5+patch ("sched/eevdf: Prevent vlag from going out of bounds when reweight_eevdf")

        v6.9-rc5              v6.9-rc5+patch
---------------- ---------------------------
       fail:runs  %reproduction    fail:runs
           |             |             |
         41:999         -4%            :999   dmesg.BUG:kernel_NULL_pointer_dereference,address
         24:999         -2%            :999   dmesg.EIP:pick_next_task_fair
         17:999         -2%            :999   dmesg.EIP:set_next_entity
         41:999         -4%            :999   dmesg.Kernel_panic-not_syncing:Fatal_exception
         41:999         -4%            :999   dmesg.Oops:#[##]

> > changes of v2:
> > -add reported-by (suggested by <yu.c.chen@intel.com>)
> > -remork the changelog (<yu.c.chen@intel.com>)
> > -remove the judgement of fork (Peter)
> > -remove the !on_rq case. (Peter)
> > ---
> > Previous discussion link:
> > https://lore.kernel.org/lkml/20240226082349.302363-1-yu.c.chen@intel.com/
> > https://lore.kernel.org/all/20240130080643.1828-1-xuewen.yan@unisoc.com/
> > ---
> > ---
> >  kernel/sched/fair.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> > 
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 03be0d1330a6..64826f406d6d 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -696,15 +696,21 @@ u64 avg_vruntime(struct cfs_rq *cfs_rq)
> >   *
> >   * XXX could add max_slice to the augmented data to track this.
> >   */
> > -static void update_entity_lag(struct cfs_rq *cfs_rq, struct sched_entity *se)
> > +static s64 entity_lag(u64 avruntime, struct sched_entity *se)
> >  {
> > -	s64 lag, limit;
> > +	s64 vlag, limit;
> > +
> > +	vlag = avruntime - se->vruntime;
> > +	limit = calc_delta_fair(max_t(u64, 2*se->slice, TICK_NSEC), se);
> > +
> > +	return clamp(vlag, -limit, limit);
> > +}
> >  
> > +static void update_entity_lag(struct cfs_rq *cfs_rq, struct sched_entity *se)
> > +{
> >  	SCHED_WARN_ON(!se->on_rq);
> > -	lag = avg_vruntime(cfs_rq) - se->vruntime;
> >  
> > -	limit = calc_delta_fair(max_t(u64, 2*se->slice, TICK_NSEC), se);
> > -	se->vlag = clamp(lag, -limit, limit);
> > +	se->vlag = entity_lag(avg_vruntime(cfs_rq), se);
> >  }
> >  
> >  /*
> > @@ -3761,7 +3767,7 @@ static void reweight_eevdf(struct cfs_rq *cfs_rq, struct sched_entity *se,
> >  	 *	   = V  - vl'
> >  	 */
> >  	if (avruntime != se->vruntime) {
> > -		vlag = (s64)(avruntime - se->vruntime);
> > +		vlag = entity_lag(avruntime, se);
> >  		vlag = div_s64(vlag * old_weight, weight);
> >  		se->vruntime = avruntime - vlag;
> >  	}
> > -- 
> > 2.25.1
> >