linux-kernel - Re: [PATCH v3 2/5] mm: kmem: add direct objcg pointer to task

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALvZod5bxWQ8nwxyxv=ySN-75FEgtCKWussyNyFeDadURQ2XYw@mail.gmail.com>
Date:   Wed, 18 Oct 2023 08:29:14 -0700
From:   Shakeel Butt <shakeelb@...gle.com>
To:     Vlastimil Babka <vbabka@...e.cz>
Cc:     Roman Gushchin <roman.gushchin@...ux.dev>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, cgroups@...r.kernel.org,
        Johannes Weiner <hannes@...xchg.org>,
        Michal Hocko <mhocko@...nel.org>,
        Muchun Song <muchun.song@...ux.dev>,
        Dennis Zhou <dennis@...nel.org>,
        David Rientjes <rientjes@...gle.com>,
        Naresh Kamboju <naresh.kamboju@...aro.org>
Subject: Re: [PATCH v3 2/5] mm: kmem: add direct objcg pointer to task_struct

On Wed, Oct 18, 2023 at 2:52 AM Vlastimil Babka <vbabka@...e.cz> wrote:
[...]
> >
> > +static struct obj_cgroup *current_objcg_update(void)
> > +{
> > +     struct mem_cgroup *memcg;
> > +     struct obj_cgroup *old, *objcg = NULL;
> > +
> > +     do {
> > +             /* Atomically drop the update bit. */
> > +             old = xchg(&current->objcg, NULL);
> > +             if (old) {
> > +                     old = (struct obj_cgroup *)
> > +                             ((unsigned long)old & ~CURRENT_OBJCG_UPDATE_FLAG);
> > +                     if (old)
> > +                             obj_cgroup_put(old);
> > +
> > +                     old = NULL;
> > +             }
> > +
> > +             /* Obtain the new objcg pointer. */
> > +             rcu_read_lock();
> > +             memcg = mem_cgroup_from_task(current);
> > +             /*
> > +              * The current task can be asynchronously moved to another
> > +              * memcg and the previous memcg can be offlined. So let's
> > +              * get the memcg pointer and try get a reference to objcg
> > +              * under a rcu read lock.
> > +              */
> > +             for (; memcg != root_mem_cgroup; memcg = parent_mem_cgroup(memcg)) {
> > +                     objcg = rcu_dereference(memcg->objcg);
> > +                     if (likely(objcg && obj_cgroup_tryget(objcg)))
>
> So IIUC here we increase objcg refcount.
>
> > +                             break;
> > +                     objcg = NULL;
> > +             }
> > +             rcu_read_unlock();
> > +
> > +             /*
> > +              * Try set up a new objcg pointer atomically. If it
> > +              * fails, it means the update flag was set concurrently, so
> > +              * the whole procedure should be repeated.
> > +              */
> > +     } while (!try_cmpxchg(&current->objcg, &old, objcg));
>
> And if this fails we throw objcg away and try again, but we should do
> obj_cgroup_put(objcg) first, as otherwise it would cause a leak?
>

Indeed there is a reference leak here.