Re: [PATCH 05/44] ipc/msg: Fix saturation of percpu counts in msgctl_info()

[David Laight]

On Wed, 28 Oct 2026 21:04:54 -0700
Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:

> On Wed, 28 Oct 2026 18:15:02 -0700 Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> 
> > > diff --git a/ipc/msg.c b/ipc/msg.c
> > > index ee6af4fe52bf..34e607add006 100644
> > > --- a/ipc/msg.c
> > > +++ b/ipc/msg.c
> > > @@ -501,11 +501,9 @@ static int msgctl_info(struct ipc_namespace *ns, int msqid,
> > >  	max_idx = ipc_get_maxidx(&msg_ids(ns));
> > >  	up_read(&msg_ids(ns).rwsem);
> > >  	if (cmd == MSG_INFO) {
> > > -		msginfo->msgmap = min_t(int,
> > > -				     percpu_counter_sum(&ns->percpu_msg_hdrs),
> > > +		msginfo->msgmap = min(percpu_counter_sum(&ns->percpu_msg_hdrs),
> > >  				     INT_MAX);
> > > -		msginfo->msgtql = min_t(int,
> > > -		                     percpu_counter_sum(&ns->percpu_msg_bytes),
> > > +		msginfo->msgtql = min(percpu_counter_sum(&ns->percpu_msg_bytes),
> > >  				     INT_MAX);
> > >  	} else {
> > >  		msginfo->msgmap = MSGMAP;  
> > 
> > Added, thanks.
> >   
> 
> LLM found another bug in there.  Please review:

Seems reasonable, if unlikely to ever happen.
Description is overlong ...

Might be worth doing a scan of all uses of percpu_counter_sum(),
I can imagine it is a common problem.

Oh and the code could use clamp(per_cpu_counter_sum(...), 0, INT_MAX);
which is exactly the same but maybe clearer?

> 
> From: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Subject: ipc/msg: clamp negative MSG_INFO usage counts
> Date: Thu, 29 Oct 2026 01:43:24 +0000
> 
> MSG_INFO can report negative message and byte counts during ordinary queue
> traffic.  For example, ipcs -q -u prints a negative header count and can
> convert a negative byte count into an enormous unsigned used-space value.
> 
> percpu_counter_sum() does not take an atomic snapshot of all CPUs.  While
> a third CPU sums the counters, it can read the sending CPU's local count
> before an enqueue and the receiving CPU's local count after the matching
> dequeue.  The sampled total is then negative, even if the queue was
> initially empty and is empty again.  The sum's lock serializes folding
> into the global count, but not ordinary local counter updates.  Queue
> locking does not serialize these updates with the statistics reader.
> 
> Use percpu_counter_sum_positive() for both usage counters before applying
> the upper clamp.  Keep the comparison in s64 so the result is not narrowed
> before saturation.
> 
> This bounds the reported usage without making the statistics an atomic
> snapshot.  Message delivery and queue-limit enforcement are unchanged.
> 
> This is a follow-up to David Laight's "ipc/msg: Fix saturation of percpu
> counts in msgctl_info()".
> 
> Validated with a source-extraction regression harness, including a forced
> three-CPU sampling trace and failing old-code controls.  No running-kernel
> reproduction or kernel build was performed.
> 
> Backporting: kernels such as v6.1 and v6.6 require matching min() operand
> types.  Use min_t(s64, ...) instead of min() on those kernels.  For trees
> still using min_t(int, ...), also retain the s64 upper clamp; the separate
> standalone variant includes both adjustments.
> 
> Fixes: 72d1e611082e ("ipc/msg: mitigate the lock contention with percpu counter")
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: David Laight <david.laight.linux@xxxxxxxxx>
> Cc: Davidlohr Bueso <dave@xxxxxxxxxxxx>
> Cc: Manfred Spraul <manfred@xxxxxxxxxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx>
> ---
> 
>  ipc/msg.c |   10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> --- a/ipc/msg.c~ipc-msg-clamp-negative-statistics
> +++ a/ipc/msg.c
> @@ -501,10 +501,12 @@ static int msgctl_info(struct ipc_namesp
>  	max_idx = ipc_get_maxidx(&msg_ids(ns));
>  	up_read(&msg_ids(ns).rwsem);
>  	if (cmd == MSG_INFO) {
> -		msginfo->msgmap = min(percpu_counter_sum(&ns->percpu_msg_hdrs),
> -				     INT_MAX);
> -		msginfo->msgtql = min(percpu_counter_sum(&ns->percpu_msg_bytes),
> -				     INT_MAX);
> +		msginfo->msgmap = min(
> +			percpu_counter_sum_positive(&ns->percpu_msg_hdrs),
> +			INT_MAX);
> +		msginfo->msgtql = min(
> +			percpu_counter_sum_positive(&ns->percpu_msg_bytes),
> +			INT_MAX);

checkpatch won't like those line breaks!

David

>  	} else {
>  		msginfo->msgmap = MSGMAP;
>  		msginfo->msgpool = MSGPOOL;
> _
>