[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <op.178te0tbwjvjmi@hhuan26-mobl.amr.corp.intel.com>
Date: Mon, 17 Jul 2023 15:19:38 -0500
From: "Haitao Huang" <haitao.huang@...ux.intel.com>
To: "Jarkko Sakkinen" <jarkko@...nel.org>, "Tejun Heo" <tj@...nel.org>,
"Haitao Huang" <haitao.huang@...ux.intel.com>
Cc: dave.hansen@...ux.intel.com, linux-kernel@...r.kernel.org,
linux-sgx@...r.kernel.org, cgroups@...r.kernel.org,
"Zefan Li" <lizefan.x@...edance.com>,
"Johannes Weiner" <hannes@...xchg.org>, vipinsh@...gle.com,
kai.huang@...el.com, reinette.chatre@...el.com,
zhiquan1.li@...el.com, kristen@...ux.intel.com
Subject: Re: [PATCH] cgroup/misc: Fix an overflow
On Mon, 17 Jul 2023 14:01:03 -0500, Haitao Huang
<haitao.huang@...ux.intel.com> wrote:
> On Mon, 17 Jul 2023 13:57:59 -0500, Tejun Heo <tj@...nel.org> wrote:
>
>> On Mon, Jul 17, 2023 at 06:55:32PM +0000, Jarkko Sakkinen wrote:
>>> On Mon Jul 17, 2023 at 6:47 PM UTC, Haitao Huang wrote:
>>> > The variable 'new_usage' in misc_cg_try_charge() may overflow if it
>>> > becomes above INT_MAX. This was observed when I implement the new SGX
>>> > EPC cgroup[1] as a misc cgroup and test on a platform with large SGX
>>> EPC
>>> > sizes.
>>> >
>>> > Change type of new_usage to long from int and check overflow.
>>> >
>>> > Fixes: a72232eabdfcf ("cgroup: Add misc cgroup controller")
>>> > Signed-off-by: Haitao Huang <haitao.huang@...ux.intel.com>
>>> >
>>> > [1]
>>> https://lore.kernel.org/linux-sgx/20230712230202.47929-1-haitao.huang@linux.intel.com/
>>> > ---
>>> > kernel/cgroup/misc.c | 6 +++---
>>> > 1 file changed, 3 insertions(+), 3 deletions(-)
>>> >
>>> > diff --git a/kernel/cgroup/misc.c b/kernel/cgroup/misc.c
>>> > index fe3e8a0eb7ed..ff9f900981a3 100644
>>> > --- a/kernel/cgroup/misc.c
>>> > +++ b/kernel/cgroup/misc.c
>>> > @@ -143,7 +143,7 @@ int misc_cg_try_charge(enum misc_res_type type,
>>> struct misc_cg *cg,
>>> > struct misc_cg *i, *j;
>>> > int ret;
>>> > struct misc_res *res;
>>> > - int new_usage;
>>> > + long new_usage;
>>> >
>>> > if (!(valid_type(type) && cg &&
>>> READ_ONCE(misc_res_capacity[type])))
>>> > return -EINVAL;
>>> > @@ -153,10 +153,10 @@ int misc_cg_try_charge(enum misc_res_type
>>> type, struct misc_cg *cg,
>>> >
>>> > for (i = cg; i; i = parent_misc(i)) {
>>> > res = &i->res[type];
>>> > -
>>>
>>> This is extra noise in the patch, please remove the change.
>>
>> Lemme just revert it. Haitao, can you instead make the resource
>> counters and
>> all related variables explicit 64bit instead?
>>
>
> Will do.
Actually, we are using atomic_long_t for 'current' which is the same width
as long defined by arch/compiler. So new_usage should be long to be
consistent?
ditto for event counter. Only max is plain unsigned long but I think it is
also OK as it only compared with 'current' without any arithmetic ops
involved.
Did I miss something here?
Thanks
Haitao
Powered by blists - more mailing lists