tjh.dev / kernel
Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

cgroup: rstat: use LOCK CMPXCHG in css_rstat_updated

On x86-64, this_cpu_cmpxchg() uses CMPXCHG without LOCK prefix which
means it is only safe for the local CPU and not for multiple CPUs.
Recently the commit 36df6e3dbd7e ("cgroup: make css_rstat_updated nmi
safe") make css_rstat_updated lockless and uses lockless list to allow
reentrancy. Since css_rstat_updated can invoked from process context,
IRQ and NMI, it uses this_cpu_cmpxchg() to select the winner which will
inset the lockless lnode into the global per-cpu lockless list.

However the commit missed one case where lockless node of a cgroup can
be accessed and modified by another CPU doing the flushing. Basically
llist_del_first_init() in css_process_update_tree().

On a cursory look, it can be questioned how css_process_update_tree()
can see a lockless node in global lockless list where the updater is at
this_cpu_cmpxchg() and before llist_add() call in css_rstat_updated().
This can indeed happen in the presence of IRQs/NMI.

Consider this scenario: Updater for cgroup stat C on CPU A in process
context is after llist_on_list() check and before this_cpu_cmpxchg() in
css_rstat_updated() where it get interrupted by IRQ/NMI. In the IRQ/NMI
context, a new updater calls css_rstat_updated() for same cgroup C and
successfully inserts rstatc_pcpu->lnode.

Now concurrently CPU B is running the flusher and it calls
llist_del_first_init() for CPU A and got rstatc_pcpu->lnode of cgroup C
which was added by the IRQ/NMI updater.

Now imagine CPU B calling init_llist_node() on cgroup C's
rstatc_pcpu->lnode of CPU A and on CPU A, the process context updater
calling this_cpu_cmpxchg(rstatc_pcpu->lnode) concurrently.

The CMPXCNG without LOCK on CPU A is not safe and thus we need LOCK
prefix.

In Meta's fleet running the kernel with the commit 36df6e3dbd7e, we are
observing on some machines the memcg stats are getting skewed by more
than the actual memory on the system. On close inspection, we noticed
that lockless node for a workload for specific CPU was in the bad state
and thus all the updates on that CPU for that cgroup was being lost.

To confirm if this skew was indeed due to this CMPXCHG without LOCK in
css_rstat_updated(), we created a repro (using AI) at [1] which shows
that CMPXCHG without LOCK creates almost the same lnode corruption as
seem in Meta's fleet and with LOCK CMPXCHG the issue does not
reproduces.

Link: http://lore.kernel.org/efiagdwmzfwpdzps74fvcwq3n4cs36q33ij7eebcpssactv3zu@se4hqiwxcfxq [1]
Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
Cc: stable@vger.kernel.org # v6.17+
Fixes: 36df6e3dbd7e ("cgroup: make css_rstat_updated nmi safe")
Signed-off-by: Tejun Heo <tj@kernel.org>

authored by

Shakeel Butt and committed by
Tejun Heo 3309b63a c2f2b01b

+8 -5
+8 -5
kernel/cgroup/rstat.c
··· 71 71 { 72 72 struct llist_head *lhead; 73 73 struct css_rstat_cpu *rstatc; 74 - struct css_rstat_cpu __percpu *rstatc_pcpu; 75 74 struct llist_node *self; 76 75 77 76 /* ··· 103 104 /* 104 105 * This function can be renentered by irqs and nmis for the same cgroup 105 106 * and may try to insert the same per-cpu lnode into the llist. Note 106 - * that llist_add() does not protect against such scenarios. 107 + * that llist_add() does not protect against such scenarios. In addition 108 + * this same per-cpu lnode can be modified through init_llist_node() 109 + * from css_rstat_flush() running on a different CPU. 107 110 * 108 111 * To protect against such stacked contexts of irqs/nmis, we use the 109 112 * fact that lnode points to itself when not on a list and then use 110 - * this_cpu_cmpxchg() to atomically set to NULL to select the winner 113 + * try_cmpxchg() to atomically set to NULL to select the winner 111 114 * which will call llist_add(). The losers can assume the insertion is 112 115 * successful and the winner will eventually add the per-cpu lnode to 113 116 * the llist. 117 + * 118 + * Please note that we can not use this_cpu_cmpxchg() here as on some 119 + * archs it is not safe against modifications from multiple CPUs. 114 120 */ 115 121 self = &rstatc->lnode; 116 - rstatc_pcpu = css->rstat_cpu; 117 - if (this_cpu_cmpxchg(rstatc_pcpu->lnode.next, self, NULL) != self) 122 + if (!try_cmpxchg(&rstatc->lnode.next, &self, NULL)) 118 123 return; 119 124 120 125 lhead = ss_lhead_cpu(css->ss, cpu);