[PATCH] mm: page_counter: reject empty string in page_counter_memparse()

[PATCH] mm: page_counter: reject empty string in page_counter_memparse()

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

memparse() consumes no characters on an empty input and leaves the
end pointer at the terminating NUL.  The only validation in
page_counter_memparse() checks for trailing characters, so an empty
input slips through and the limit becomes 0.

All limit write callbacks of the memory controller strstrip() the
input before calling this helper, so a script that writes an unset
variable hits this path:

  LIMIT=
  echo "$LIMIT" > $CG/memory.max
  echo $?
  0
  cat $CG/memory.max
  0

Nothing reports the mistake: the limit is now 0 and the OOM killer
goes after every task in the cgroup.  The same happens for
memory.min, memory.low, memory.high, memory.swap.high,
memory.swap.max and memory.zswap.max, where 0 silently removes the
protection or disables swap and zswap.

Reject the input when no characters were consumed, which is the one
case the trailing-character check cannot catch.

Fixes: 3e32cb2e0a12 ("mm: memcontrol: lockless page counters")
Signed-off-by: Tao Cui <cuitao@kylinos.cn>
---
 mm/page_counter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/page_counter.c b/mm/page_counter.c
index 661e0f2a5127..d14db705b04f 100644
--- a/mm/page_counter.c
+++ b/mm/page_counter.c
@@ -281,7 +281,7 @@ int page_counter_memparse(const char *buf, const char *max,
 	}
 
 	bytes = memparse(buf, &end);
-	if (*end != '\0')
+	if (*end != '\0' || end == buf)
 		return -EINVAL;
 
 	*nr_pages = min(bytes / PAGE_SIZE, (u64)PAGE_COUNTER_MAX);
-- 
2.43.0

Re: [PATCH] mm: page_counter: reject empty string in page_counter_memparse()

[Shakeel Butt]

On Mon, Aug 17, 2026 at 12:26:52PM +0800, Tao Cui wrote:
> From: Tao Cui <cuitao@kylinos.cn>
> 
> memparse() consumes no characters on an empty input and leaves the
> end pointer at the terminating NUL.  The only validation in
> page_counter_memparse() checks for trailing characters, so an empty
> input slips through and the limit becomes 0.
> 
> All limit write callbacks of the memory controller strstrip() the
> input before calling this helper, so a script that writes an unset
> variable hits this path:
> 
>   LIMIT=
>   echo "$LIMIT" > $CG/memory.max
>   echo $?
>   0
>   cat $CG/memory.max
>   0
> 
> Nothing reports the mistake: the limit is now 0 and the OOM killer
> goes after every task in the cgroup.  The same happens for
> memory.min, memory.low, memory.high, memory.swap.high,
> memory.swap.max and memory.zswap.max, where 0 silently removes the
> protection or disables swap and zswap.
> 
> Reject the input when no characters were consumed, which is the one
> case the trailing-character check cannot catch.
> 
> Fixes: 3e32cb2e0a12 ("mm: memcontrol: lockless page counters")
> Signed-off-by: Tao Cui <cuitao@kylinos.cn>
> ---
>  mm/page_counter.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/page_counter.c b/mm/page_counter.c
> index 661e0f2a5127..d14db705b04f 100644
> --- a/mm/page_counter.c
> +++ b/mm/page_counter.c
> @@ -281,7 +281,7 @@ int page_counter_memparse(const char *buf, const char *max,
>  	}
>  
>  	bytes = memparse(buf, &end);
> -	if (*end != '\0')
> +	if (*end != '\0' || end == buf)
>  		return -EINVAL;

I wonder if someone started depending on this behavior. In that case it is
better to return error instead of silently ignore, so we will hear complains
loudly. This looks good to me.

Acked-by: Shakeel Butt <shakeel.butt@linux.dev>


>  
>  	*nr_pages = min(bytes / PAGE_SIZE, (u64)PAGE_COUNTER_MAX);
> -- 
> 2.43.0
> 

Re: [PATCH] mm: page_counter: reject empty string in page_counter_memparse()

[Michal Hocko]

On Mon 17-08-26 09:16:40, Shakeel Butt wrote:
> On Mon, Aug 17, 2026 at 12:26:52PM +0800, Tao Cui wrote:
> > From: Tao Cui <cuitao@kylinos.cn>
> > 
> > memparse() consumes no characters on an empty input and leaves the
> > end pointer at the terminating NUL.  The only validation in
> > page_counter_memparse() checks for trailing characters, so an empty
> > input slips through and the limit becomes 0.
> > 
> > All limit write callbacks of the memory controller strstrip() the
> > input before calling this helper, so a script that writes an unset
> > variable hits this path:
> > 
> >   LIMIT=
> >   echo "$LIMIT" > $CG/memory.max
> >   echo $?
> >   0
> >   cat $CG/memory.max
> >   0
> > 
> > Nothing reports the mistake: the limit is now 0 and the OOM killer
> > goes after every task in the cgroup.  The same happens for
> > memory.min, memory.low, memory.high, memory.swap.high,
> > memory.swap.max and memory.zswap.max, where 0 silently removes the
> > protection or disables swap and zswap.
> > 
> > Reject the input when no characters were consumed, which is the one
> > case the trailing-character check cannot catch.
> > 
> > Fixes: 3e32cb2e0a12 ("mm: memcontrol: lockless page counters")
> > Signed-off-by: Tao Cui <cuitao@kylinos.cn>
> > ---
> >  mm/page_counter.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/mm/page_counter.c b/mm/page_counter.c
> > index 661e0f2a5127..d14db705b04f 100644
> > --- a/mm/page_counter.c
> > +++ b/mm/page_counter.c
> > @@ -281,7 +281,7 @@ int page_counter_memparse(const char *buf, const char *max,
> >  	}
> >  
> >  	bytes = memparse(buf, &end);
> > -	if (*end != '\0')
> > +	if (*end != '\0' || end == buf)
> >  		return -EINVAL;
> 
> I wonder if someone started depending on this behavior. In that case it is
> better to return error instead of silently ignore, so we will hear complains
> loudly. This looks good to me.

This is backward incompatible change and I am wondering why should we
even risk regression.
 
> Acked-by: Shakeel Butt <shakeel.butt@linux.dev>

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: page_counter: reject empty string in page_counter_memparse()

[Shakeel Butt]

On Tue, Aug 18, 2026 at 09:45:56AM +0200, Michal Hocko wrote:
> On Mon 17-08-26 09:16:40, Shakeel Butt wrote:
> > On Mon, Aug 17, 2026 at 12:26:52PM +0800, Tao Cui wrote:
> > > From: Tao Cui <cuitao@kylinos.cn>
> > > 
> > > memparse() consumes no characters on an empty input and leaves the
> > > end pointer at the terminating NUL.  The only validation in
> > > page_counter_memparse() checks for trailing characters, so an empty
> > > input slips through and the limit becomes 0.
> > > 
> > > All limit write callbacks of the memory controller strstrip() the
> > > input before calling this helper, so a script that writes an unset
> > > variable hits this path:
> > > 
> > >   LIMIT=
> > >   echo "$LIMIT" > $CG/memory.max
> > >   echo $?
> > >   0
> > >   cat $CG/memory.max
> > >   0
> > > 
> > > Nothing reports the mistake: the limit is now 0 and the OOM killer
> > > goes after every task in the cgroup.  The same happens for
> > > memory.min, memory.low, memory.high, memory.swap.high,
> > > memory.swap.max and memory.zswap.max, where 0 silently removes the
> > > protection or disables swap and zswap.
> > > 
> > > Reject the input when no characters were consumed, which is the one
> > > case the trailing-character check cannot catch.
> > > 
> > > Fixes: 3e32cb2e0a12 ("mm: memcontrol: lockless page counters")
> > > Signed-off-by: Tao Cui <cuitao@kylinos.cn>
> > > ---
> > >  mm/page_counter.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/mm/page_counter.c b/mm/page_counter.c
> > > index 661e0f2a5127..d14db705b04f 100644
> > > --- a/mm/page_counter.c
> > > +++ b/mm/page_counter.c
> > > @@ -281,7 +281,7 @@ int page_counter_memparse(const char *buf, const char *max,
> > >  	}
> > >  
> > >  	bytes = memparse(buf, &end);
> > > -	if (*end != '\0')
> > > +	if (*end != '\0' || end == buf)
> > >  		return -EINVAL;
> > 
> > I wonder if someone started depending on this behavior. In that case it is
> > better to return error instead of silently ignore, so we will hear complains
> > loudly. This looks good to me.
> 
> This is backward incompatible change and I am wondering why should we
> even risk regression.

Mainly I was wondering if this is intentional or unintentional. If this us
unintentional, can we fix it without anyone noticing?

However if we are ok with this then let's make is formal and make this a
documented behavior. I don't have any strong opinion either way but I think you
are saying it safer to just assume this is intentional. Fine with me.

Tao, can you send a patch to Documentation/admin-guide/cgroup-v2.rst which
explicitly mention this behavior?

Re: [PATCH] mm: page_counter: reject empty string in page_counter_memparse()

[Michal Hocko]

On Tue 18-08-26 08:06:22, Shakeel Butt wrote:
> On Tue, Aug 18, 2026 at 09:45:56AM +0200, Michal Hocko wrote:
> > On Mon 17-08-26 09:16:40, Shakeel Butt wrote:
> > > On Mon, Aug 17, 2026 at 12:26:52PM +0800, Tao Cui wrote:
> > > > From: Tao Cui <cuitao@kylinos.cn>
> > > > 
> > > > memparse() consumes no characters on an empty input and leaves the
> > > > end pointer at the terminating NUL.  The only validation in
> > > > page_counter_memparse() checks for trailing characters, so an empty
> > > > input slips through and the limit becomes 0.
> > > > 
> > > > All limit write callbacks of the memory controller strstrip() the
> > > > input before calling this helper, so a script that writes an unset
> > > > variable hits this path:
> > > > 
> > > >   LIMIT=
> > > >   echo "$LIMIT" > $CG/memory.max
> > > >   echo $?
> > > >   0
> > > >   cat $CG/memory.max
> > > >   0
> > > > 
> > > > Nothing reports the mistake: the limit is now 0 and the OOM killer
> > > > goes after every task in the cgroup.  The same happens for
> > > > memory.min, memory.low, memory.high, memory.swap.high,
> > > > memory.swap.max and memory.zswap.max, where 0 silently removes the
> > > > protection or disables swap and zswap.
> > > > 
> > > > Reject the input when no characters were consumed, which is the one
> > > > case the trailing-character check cannot catch.
> > > > 
> > > > Fixes: 3e32cb2e0a12 ("mm: memcontrol: lockless page counters")
> > > > Signed-off-by: Tao Cui <cuitao@kylinos.cn>
> > > > ---
> > > >  mm/page_counter.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/mm/page_counter.c b/mm/page_counter.c
> > > > index 661e0f2a5127..d14db705b04f 100644
> > > > --- a/mm/page_counter.c
> > > > +++ b/mm/page_counter.c
> > > > @@ -281,7 +281,7 @@ int page_counter_memparse(const char *buf, const char *max,
> > > >  	}
> > > >  
> > > >  	bytes = memparse(buf, &end);
> > > > -	if (*end != '\0')
> > > > +	if (*end != '\0' || end == buf)
> > > >  		return -EINVAL;
> > > 
> > > I wonder if someone started depending on this behavior. In that case it is
> > > better to return error instead of silently ignore, so we will hear complains
> > > loudly. This looks good to me.
> > 
> > This is backward incompatible change and I am wondering why should we
> > even risk regression.
> 
> Mainly I was wondering if this is intentional or unintentional. If this us
> unintentional, can we fix it without anyone noticing?

My guess would be this was just omission. Those happen and over years we
have learned that userspace is quite creative at using those.

> However if we are ok with this then let's make is formal and make this a
> documented behavior. I don't have any strong opinion either way but I think you
> are saying it safer to just assume this is intentional. Fine with me.

My main question is why should we even bother to change this in the
first place? Is that reason stronger than a theoretical breakage of
userspace that we might learn much later?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: page_counter: reject empty string in page_counter_memparse()

[Tao Cui]

Hi, Michal, Shakeel

在 2026/8/19 00:43, Michal Hocko 写道:
> On Tue 18-08-26 08:06:22, Shakeel Butt wrote:
>> On Tue, Aug 18, 2026 at 09:45:56AM +0200, Michal Hocko wrote:
>>> On Mon 17-08-26 09:16:40, Shakeel Butt wrote:
>>>> On Mon, Aug 17, 2026 at 12:26:52PM +0800, Tao Cui wrote:
>>>>> From: Tao Cui <cuitao@kylinos.cn>
>>>>>
>>>>> memparse() consumes no characters on an empty input and leaves the
>>>>> end pointer at the terminating NUL.  The only validation in
>>>>> page_counter_memparse() checks for trailing characters, so an empty
>>>>> input slips through and the limit becomes 0.
>>>>>
>>>>> All limit write callbacks of the memory controller strstrip() the
>>>>> input before calling this helper, so a script that writes an unset
>>>>> variable hits this path:
>>>>>
>>>>>   LIMIT=
>>>>>   echo "$LIMIT" > $CG/memory.max
>>>>>   echo $?
>>>>>   0
>>>>>   cat $CG/memory.max
>>>>>   0
>>>>>
>>>>> Nothing reports the mistake: the limit is now 0 and the OOM killer
>>>>> goes after every task in the cgroup.  The same happens for
>>>>> memory.min, memory.low, memory.high, memory.swap.high,
>>>>> memory.swap.max and memory.zswap.max, where 0 silently removes the
>>>>> protection or disables swap and zswap.
>>>>>
>>>>> Reject the input when no characters were consumed, which is the one
>>>>> case the trailing-character check cannot catch.
>>>>>
>>>>> Fixes: 3e32cb2e0a12 ("mm: memcontrol: lockless page counters")
>>>>> Signed-off-by: Tao Cui <cuitao@kylinos.cn>
>>>>> ---
>>>>>  mm/page_counter.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/mm/page_counter.c b/mm/page_counter.c
>>>>> index 661e0f2a5127..d14db705b04f 100644
>>>>> --- a/mm/page_counter.c
>>>>> +++ b/mm/page_counter.c
>>>>> @@ -281,7 +281,7 @@ int page_counter_memparse(const char *buf, const char *max,
>>>>>  	}
>>>>>  
>>>>>  	bytes = memparse(buf, &end);
>>>>> -	if (*end != '\0')
>>>>> +	if (*end != '\0' || end == buf)
>>>>>  		return -EINVAL;
>>>>
>>>> I wonder if someone started depending on this behavior. In that case it is
>>>> better to return error instead of silently ignore, so we will hear complains
>>>> loudly. This looks good to me.
>>>
>>> This is backward incompatible change and I am wondering why should we
>>> even risk regression.
>>
>> Mainly I was wondering if this is intentional or unintentional. If this us
>> unintentional, can we fix it without anyone noticing?
> 
> My guess would be this was just omission. Those happen and over years we
> have learned that userspace is quite creative at using those.
> 
>> However if we are ok with this then let's make is formal and make this a
>> documented behavior. I don't have any strong opinion either way but I think you
>> are saying it safer to just assume this is intentional. Fine with me.
> 
> My main question is why should we even bother to change this in the
> first place? Is that reason stronger than a theoretical breakage of
> userspace that we might learn much later?

Since you asked "why bother", here's how I ran into it.

The patch actually came from a production incident rather than a code
audit.

A maintenance script on a cluster accidentally wrote an unset variable
into memory.max of a workload cgroup. The write succeeded, and the
workload in the cgroup was subsequently OOM-killed. There was no
indication that the successful write had caused it, so it took quite
some time to trace the OOMs back to that script.

When I checked the documentation, I noticed that the cpuset controller
explicitly documents the semantics of empty writes ("An empty value
indicates that the cgroup is using the same setting as the nearest
cgroup ancestor..."), while the memory controller documentation says
nothing about empty input.

I reproduced the same behavior in isolation on a Kubernetes cluster
(v1.29, cgroup v2, two-container pod, 384M pod limit):

  # LIMIT=
  # echo "$LIMIT" > $CG/memory.max
  # echo $?
  0

  m6demo   0/2     OOMKilled   0

  oom-kill: constraint=CONSTRAINT_MEMCG,
            oom_memcg=/kubepods.slice/.../kubepods-burstable-pod....slice
  Memory cgroup out of memory: Killed process 339529 (sleep) ...
            anon-rss:32kB, file-rss:452kB

The process selected by the OOM killer had less than 1 MB resident
under a 384M pod limit, but the empty write was accepted as 0,
immediately triggering a memcg OOM. From the caller's perspective, the
write simply succeeded.

One caveat is that kubelet reconciles the pod-level memory.max within
seconds, so the window is short there, although container-level files
are not reconciled.

So the patch came out of that incident and the documentation gap it
exposed. My thinking was that rejecting an empty input would make such
mistakes fail immediately instead of silently changing the limit to 0.

Whether that benefit outweighs the compatibility risk is not mine to
decide. If it doesn't, then documenting the current empty-write
behavior, similar to cpuset, would also address the ambiguity that led
me to investigate this in the first place.

Thanks,
Tao

Re: [PATCH] mm: page_counter: reject empty string in page_counter_memparse()

[Michal Hocko]

On Wed 19-08-26 11:00:09, Tao Cui wrote:
> Hi, Michal, Shakeel
> 
> 在 2026/8/19 00:43, Michal Hocko 写道:
> > On Tue 18-08-26 08:06:22, Shakeel Butt wrote:
> >> On Tue, Aug 18, 2026 at 09:45:56AM +0200, Michal Hocko wrote:
> >>> On Mon 17-08-26 09:16:40, Shakeel Butt wrote:
> >>>> On Mon, Aug 17, 2026 at 12:26:52PM +0800, Tao Cui wrote:
> >>>>> From: Tao Cui <cuitao@kylinos.cn>
> >>>>>
> >>>>> memparse() consumes no characters on an empty input and leaves the
> >>>>> end pointer at the terminating NUL.  The only validation in
> >>>>> page_counter_memparse() checks for trailing characters, so an empty
> >>>>> input slips through and the limit becomes 0.
> >>>>>
> >>>>> All limit write callbacks of the memory controller strstrip() the
> >>>>> input before calling this helper, so a script that writes an unset
> >>>>> variable hits this path:
> >>>>>
> >>>>>   LIMIT=
> >>>>>   echo "$LIMIT" > $CG/memory.max
> >>>>>   echo $?
> >>>>>   0
> >>>>>   cat $CG/memory.max
> >>>>>   0
> >>>>>
> >>>>> Nothing reports the mistake: the limit is now 0 and the OOM killer
> >>>>> goes after every task in the cgroup.  The same happens for
> >>>>> memory.min, memory.low, memory.high, memory.swap.high,
> >>>>> memory.swap.max and memory.zswap.max, where 0 silently removes the
> >>>>> protection or disables swap and zswap.
> >>>>>
> >>>>> Reject the input when no characters were consumed, which is the one
> >>>>> case the trailing-character check cannot catch.
> >>>>>
> >>>>> Fixes: 3e32cb2e0a12 ("mm: memcontrol: lockless page counters")
> >>>>> Signed-off-by: Tao Cui <cuitao@kylinos.cn>
> >>>>> ---
> >>>>>  mm/page_counter.c | 2 +-
> >>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/mm/page_counter.c b/mm/page_counter.c
> >>>>> index 661e0f2a5127..d14db705b04f 100644
> >>>>> --- a/mm/page_counter.c
> >>>>> +++ b/mm/page_counter.c
> >>>>> @@ -281,7 +281,7 @@ int page_counter_memparse(const char *buf, const char *max,
> >>>>>  	}
> >>>>>  
> >>>>>  	bytes = memparse(buf, &end);
> >>>>> -	if (*end != '\0')
> >>>>> +	if (*end != '\0' || end == buf)
> >>>>>  		return -EINVAL;
> >>>>
> >>>> I wonder if someone started depending on this behavior. In that case it is
> >>>> better to return error instead of silently ignore, so we will hear complains
> >>>> loudly. This looks good to me.
> >>>
> >>> This is backward incompatible change and I am wondering why should we
> >>> even risk regression.
> >>
> >> Mainly I was wondering if this is intentional or unintentional. If this us
> >> unintentional, can we fix it without anyone noticing?
> > 
> > My guess would be this was just omission. Those happen and over years we
> > have learned that userspace is quite creative at using those.
> > 
> >> However if we are ok with this then let's make is formal and make this a
> >> documented behavior. I don't have any strong opinion either way but I think you
> >> are saying it safer to just assume this is intentional. Fine with me.
> > 
> > My main question is why should we even bother to change this in the
> > first place? Is that reason stronger than a theoretical breakage of
> > userspace that we might learn much later?
> 
> Since you asked "why bother", here's how I ran into it.
> 
> The patch actually came from a production incident rather than a code
> audit.
> 
> A maintenance script on a cluster accidentally wrote an unset variable
> into memory.max of a workload cgroup. The write succeeded, and the
> workload in the cgroup was subsequently OOM-killed. There was no
> indication that the successful write had caused it, so it took quite
> some time to trace the OOMs back to that script.

Understood.

> When I checked the documentation, I noticed that the cpuset controller
> explicitly documents the semantics of empty writes ("An empty value
> indicates that the cgroup is using the same setting as the nearest
> cgroup ancestor..."), while the memory controller documentation says
> nothing about empty input.

yes, this is really unfortunate and mistakes like that happen.

> I reproduced the same behavior in isolation on a Kubernetes cluster
> (v1.29, cgroup v2, two-container pod, 384M pod limit):
> 
>   # LIMIT=
>   # echo "$LIMIT" > $CG/memory.max
>   # echo $?
>   0
> 
>   m6demo   0/2     OOMKilled   0
> 
>   oom-kill: constraint=CONSTRAINT_MEMCG,
>             oom_memcg=/kubepods.slice/.../kubepods-burstable-pod....slice
>   Memory cgroup out of memory: Killed process 339529 (sleep) ...
>             anon-rss:32kB, file-rss:452kB
> 
> The process selected by the OOM killer had less than 1 MB resident
> under a 384M pod limit, but the empty write was accepted as 0,
> immediately triggering a memcg OOM. From the caller's perspective, the
> write simply succeeded.
> 
> One caveat is that kubelet reconciles the pod-level memory.max within
> seconds, so the window is short there, although container-level files
> are not reconciled.
> 
> So the patch came out of that incident and the documentation gap it
> exposed. My thinking was that rejecting an empty input would make such
> mistakes fail immediately instead of silently changing the limit to 0.

Thanks for sharing the story. Next time I would recommend to make that a
part of the changelog.

> Whether that benefit outweighs the compatibility risk is not mine to
> decide.

And I am still not convinced. I do understand your frustration from the
debugging the issue. In any way, if other maintainers decide this change
is worth I will not stand in the way.

> If it doesn't, then documenting the current empty-write
> behavior, similar to cpuset, would also address the ambiguity that led
> me to investigate this in the first place.

Agreed!
-- 
Michal Hocko
SUSE Labs