lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <20859F67-A80C-4FD0-990C-40C70905E55B@linux.dev>
Date: Mon, 22 Jul 2024 15:53:57 +0800
From: Muchun Song <muchun.song@...ux.dev>
To: Chengming Zhou <chengming.zhou@...ux.dev>
Cc: Muchun Song <songmuchun@...edance.com>,
 Johannes Weiner <hannes@...xchg.org>,
 Michal Hocko <mhocko@...nel.org>,
 Roman Gushchin <roman.gushchin@...ux.dev>,
 shakeel.butt@...ux.dev,
 Andrew Morton <akpm@...ux-foundation.org>,
 cgroups@...r.kernel.org,
 Linux Memory Management List <linux-mm@...ck.org>,
 LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mm: kmem: add lockdep assertion to obj_cgroup_memcg



> On Jul 22, 2024, at 15:46, Chengming Zhou <chengming.zhou@...ux.dev> wrote:
> 
> On 2024/7/22 15:08, Muchun Song wrote:
>> The obj_cgroup_memcg() is supposed to safe to prevent the returned
>> memory cgroup from being freed only when the caller is holding the
>> rcu read lock or objcg_lock or cgroup_mutex. It is very easy to
>> ignore thoes conditions when users call some upper APIs which call
>> obj_cgroup_memcg() internally like mem_cgroup_from_slab_obj() (See
>> the link below). So it is better to add lockdep assertion to
>> obj_cgroup_memcg() to find those issues ASAP.
> 
> Yeah, some users care about the lifetime of returned memcg, while
> some other users maybe not.
> 
> Maybe a dumb question, can we just make objcg hold the refcount of
> its pointed memcg? So the users of that objcg don't need to care
> about the refcount of memcg? (We could switch the refcount from
> old memcg to the new memcg when objcg switch memcg pointer, right?)

You mean the memcg is pinned if objcg is pinned, right? If yes, in
which case, reparenting of memcg cannot make memcg being freed ASAP.

> 
> Thanks.
> 
>> Because there is no user of obj_cgroup_memcg() holding objcg_lock
>> to make the returned memory cgroup safe, do not add objcg_lock
>> assertion (We should export objcg_lock if we really want to do)
>> and leave a comment to indicate it is intentional.
>> Some users like __mem_cgroup_uncharge() do not care the lifetime
>> of the returned memory cgroup, which just want to know if the
>> folio is charged to a memory cgroup, therefore, they do not need
>> to hold the needed locks. In which case, introduce a new helper
>> folio_memcg_charged() to do this. Compare it to folio_memcg(), it
>> could eliminate a memory access of objcg->memcg for kmem, actually,
>> a really small gain.
>> Link: https://lore.kernel.org/all/20240718083607.42068-1-songmuchun@bytedance.com/
>> Signed-off-by: Muchun Song <songmuchun@...edance.com>
>> ---
>>  include/linux/memcontrol.h | 22 +++++++++++++++++++---
>>  mm/memcontrol.c            |  6 +++---
>>  2 files changed, 22 insertions(+), 6 deletions(-)
>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>> index fc94879db4dff..d616c50025098 100644
>> --- a/include/linux/memcontrol.h
>> +++ b/include/linux/memcontrol.h
>> @@ -360,11 +360,13 @@ static inline bool folio_memcg_kmem(struct folio *folio);
>>   * After the initialization objcg->memcg is always pointing at
>>   * a valid memcg, but can be atomically swapped to the parent memcg.
>>   *
>> - * The caller must ensure that the returned memcg won't be released:
>> - * e.g. acquire the rcu_read_lock or css_set_lock.
>> + * The caller must ensure that the returned memcg won't be released.
>>   */
>>  static inline struct mem_cgroup *obj_cgroup_memcg(struct obj_cgroup *objcg)
>>  {
>> + WARN_ON_ONCE(!rcu_read_lock_held() &&
>> +  /* !lockdep_is_held(&objcg_lock) && */
>> +     !lockdep_is_held(&cgroup_mutex));
>>   return READ_ONCE(objcg->memcg);
>>  }
>>  @@ -438,6 +440,19 @@ static inline struct mem_cgroup *folio_memcg(struct folio *folio)
>>   return __folio_memcg(folio);
>>  }
>>  +/*
>> + * folio_memcg_charged - If a folio is charged to a memory cgroup.
>> + * @folio: Pointer to the folio.
>> + *
>> + * Returns true if folio is charged to a memory cgroup, otherwise returns false.
>> + */
>> +static inline bool folio_memcg_charged(struct folio *folio)
>> +{
>> + if (folio_memcg_kmem(folio))
>> + return __folio_objcg(folio) != NULL;
>> + return __folio_memcg(folio) != NULL;
>> +}
>> +
>>  /**
>>   * folio_memcg_rcu - Locklessly get the memory cgroup associated with a folio.
>>   * @folio: Pointer to the folio.
>> @@ -454,7 +469,6 @@ static inline struct mem_cgroup *folio_memcg_rcu(struct folio *folio)
>>   unsigned long memcg_data = READ_ONCE(folio->memcg_data);
>>     VM_BUG_ON_FOLIO(folio_test_slab(folio), folio);
>> - WARN_ON_ONCE(!rcu_read_lock_held());
>>     if (memcg_data & MEMCG_DATA_KMEM) {
>>   struct obj_cgroup *objcg;
>> @@ -463,6 +477,8 @@ static inline struct mem_cgroup *folio_memcg_rcu(struct folio *folio)
>>   return obj_cgroup_memcg(objcg);
>>   }
>>  + WARN_ON_ONCE(!rcu_read_lock_held());
>> +
>>   return (struct mem_cgroup *)(memcg_data & ~OBJEXTS_FLAGS_MASK);
>>  }
>>  diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 622d4544edd24..3da0284573857 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -2366,7 +2366,7 @@ void mem_cgroup_cancel_charge(struct mem_cgroup *memcg, unsigned int nr_pages)
>>    static void commit_charge(struct folio *folio, struct mem_cgroup *memcg)
>>  {
>> - VM_BUG_ON_FOLIO(folio_memcg(folio), folio);
>> + VM_BUG_ON_FOLIO(folio_memcg_charged(folio), folio);
>>   /*
>>   * Any of the following ensures page's memcg stability:
>>   *
>> @@ -4617,7 +4617,7 @@ void __mem_cgroup_uncharge(struct folio *folio)
>>   struct uncharge_gather ug;
>>     /* Don't touch folio->lru of any random page, pre-check: */
>> - if (!folio_memcg(folio))
>> + if (!folio_memcg_charged(folio))
>>   return;
>>     uncharge_gather_clear(&ug);
>> @@ -4662,7 +4662,7 @@ void mem_cgroup_replace_folio(struct folio *old, struct folio *new)
>>   return;
>>     /* Page cache replacement: new folio already charged? */
>> - if (folio_memcg(new))
>> + if (folio_memcg_charged(new))
>>   return;
>>     memcg = folio_memcg(old);


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ