Re: [PATCH v2] sched/eevdf: Prevent vlag from going out of bounds when reweight_eevdf

[Yujie Liu]

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@xxxxxxxxxxxxxxx/
> 
> A fix tag might be needed.
> Fixes: eab03c23c2a1 ("sched/eevdf: Fix vruntime adjustment on reweight")
>  
> > Reported-by: Sergei Trofimovich <slyich@xxxxxxxxx>
> > Closes: https://lore.kernel.org/all/ZhuYyrh3mweP_Kd8@xxxxxxx/
> > Reported-by: Igor Raits <igor@xxxxxxxxxxxx>
> > Closes: https://lore.kernel.org/all/CA+9S74ih+45M_2TPUY_mPPVDhNvyYfy1J1ftSix+KjiTVxg8nw@xxxxxxxxxxxxxx/
> > Reported-by: Breno Leitao <leitao@xxxxxxxxxx>
> > Reported-by: kernel test robot <oliver.sang@xxxxxxxxx>
> > Closes: https://lore.kernel.org/lkml/202401301012.2ed95df0-oliver.sang@xxxxxxxxx/
> > Reported-by: Yujie Liu <yujie.liu@xxxxxxxxx>
> > Signed-off-by: Xuewen Yan <xuewen.yan@xxxxxxxxxx>
> > ---
> 
> 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@xxxxxxxxx>
> 
> 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@xxxxxxxxx>

=========================================================================================
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@xxxxxxxxx>)
> > -remork the changelog (<yu.c.chen@xxxxxxxxx>)
> > -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@xxxxxxxxx/
> > https://lore.kernel.org/all/20240130080643.1828-1-xuewen.yan@xxxxxxxxxx/
> > ---
> > ---
> >  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
> >