On Wed, Mar 30, 2022 at 01:26:46PM -0400, Waiman Long wrote: > Muchun Song found out there could be a race between list_lru_add() > and memcg_reparent_list_lru_node() causing the later function to miss > reparenting of a lru entry as shown below: > > CPU0: CPU1: > list_lru_add() > spin_lock(&nlru->lock) > l = list_lru_from_kmem(memcg) > memcg_reparent_objcgs(memcg) > memcg_reparent_list_lrus(memcg) > memcg_reparent_list_lru() > memcg_reparent_list_lru_node() > if (!READ_ONCE(nlru->nr_items)) > // Miss reparenting > return > // Assume 0->1 > l->nr_items++ > // Assume 0->1 > nlru->nr_items++ > > Though it is not likely that a list_lru_node that has 0 item suddenly > has a newly added lru entry at the end of its life. The race is still > theoretically possible. > > With the lock/unlock pair used within the percpu_ref_kill() which is > the last function call of memcg_reparent_objcgs(), any read issued > in memcg_reparent_list_lru_node() will not be reordered before the > reparenting of objcgs. > > Adding a !spin_is_locked()/smp_rmb()/!READ_ONCE(nlru->nr_items) check > to ensure that either the reading of nr_items is valid or the racing > list_lru_add() will see the reparented objcg. > > Fixes: 405cc51fc104 ("mm/list_lru: optimize memcg_reparent_list_lru_node()") > Reported-by: Muchun Song <songmuchun@xxxxxxxxxxxxx> > Signed-off-by: Waiman Long <longman@xxxxxxxxxx> Acked-by: Roman Gushchin <roman.gushchin@xxxxxxxxx>