Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 2 Jun 2021 13:22:07 +0000
From:   Quentin Perret <qperret@google.com>
To:     Xuewen Yan <xuewen.yan94@gmail.com>
Cc:     mingo@redhat.com, peterz@infradead.org, juri.lelli@redhat.com,
        vincent.guittot@linaro.org, dietmar.eggemann@arm.com,
        rostedt@goodmis.org, bsegall@google.com, mgorman@suse.de,
        bristot@redhat.com, linux-kernel@vger.kernel.org,
        zhang.lyra@gmail.com, xuewyan@foxmail.com,
        patrick.bellasi@matbug.net, tj@kernel.org
Subject: Re: [PATCH] sched/uclamp: Avoid setting cpu.uclamp.min bigger than
 cpu.uclamp.max
Message-ID: <YLeF/556Wbvx1Ssc@google.com>
References: <20210602123803.15738-1-xuewen.yan94@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20210602123803.15738-1-xuewen.yan94@gmail.com>
Precedence: bulk

+CC Patrick and Tejun

On Wednesday 02 Jun 2021 at 20:38:03 (+0800), Xuewen Yan wrote:
> From: Xuewen Yan <xuewen.yan@unisoc.com>
> 
> When setting cpu.uclamp.min/max in cgroup, there is no validating
> like uclamp_validate() in __sched_setscheduler(). It may cause the
> cpu.uclamp.min is bigger than cpu.uclamp.max.

ISTR this was intentional. We also allow child groups to ask for
whatever clamps they want, but that is always limited by the parent, and
reflected in the 'effective' values, as per the cgroup delegation model.

> Although there is protection in cpu_util_update_eff():
> “eff[UCLAMP_MIN] = min(eff[UCLAMP_MIN], eff[UCLAMP_MAX])”, it's better
> not to let it happen.
> 
> Judging the uclamp value before setting uclamp_min/max, avoid
> the cpu.uclamp.min is bigger than cpu.uclamp.max.
> 
> Signed-off-by: Xuewen Yan <xuewen.yan@unisoc.com>
> ---
>  kernel/sched/core.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 5226cc26a095..520a2da40dc9 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -8867,6 +8867,30 @@ static ssize_t cpu_uclamp_write(struct kernfs_open_file *of, char *buf,
>  	rcu_read_lock();
>  
>  	tg = css_tg(of_css(of));
> +
> +	switch (clamp_id) {
> +	case UCLAMP_MIN: {
> +		unsigned int uc_req_max = tg->uclamp_req[UCLAMP_MAX].value;
> +
> +		if (req.util > uc_req_max) {
> +			nbytes = -EINVAL;
> +			goto unlock;
> +		}
> +		break;
> +	}
> +	case UCLAMP_MAX: {
> +		unsigned int uc_req_min = tg->uclamp_req[UCLAMP_MIN].value;
> +
> +		if (req.util < uc_req_min) {
> +			nbytes = -EINVAL;
> +			goto unlock;
> +		}
> +		break;
> +	}
> +	default:
> +		nbytes = -EINVAL;
> +		goto unlock;
> +	}
>  	if (tg->uclamp_req[clamp_id].value != req.util)
>  		uclamp_se_set(&tg->uclamp_req[clamp_id], req.util, false);
>  
> @@ -8878,7 +8902,7 @@ static ssize_t cpu_uclamp_write(struct kernfs_open_file *of, char *buf,
>  
>  	/* Update effective clamps to track the most restrictive value */
>  	cpu_util_update_eff(of_css(of));
> -
> +unlock:
>  	rcu_read_unlock();
>  	mutex_unlock(&uclamp_mutex);
>  
> -- 
> 2.25.1
>