linux-kernel - Re: [PATCH 14/19] mm: Introduce a cgroup for pinned memory

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: <Y/UEkNn0O65Pfi4e@nvidia.com>
Date:   Tue, 21 Feb 2023 13:51:12 -0400
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Tejun Heo <tj@...nel.org>
Cc:     Michal Hocko <mhocko@...e.com>,
        Yosry Ahmed <yosryahmed@...gle.com>,
        Alistair Popple <apopple@...dia.com>, linux-mm@...ck.org,
        cgroups@...r.kernel.org, linux-kernel@...r.kernel.org,
        jhubbard@...dia.com, tjmercier@...gle.com, hannes@...xchg.org,
        surenb@...gle.com, mkoutny@...e.com, daniel@...ll.ch,
        "Daniel P . Berrange" <berrange@...hat.com>,
        Alex Williamson <alex.williamson@...hat.com>,
        Zefan Li <lizefan.x@...edance.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH 14/19] mm: Introduce a cgroup for pinned memory

On Tue, Feb 21, 2023 at 07:29:18AM -1000, Tejun Heo wrote:
> On Tue, Feb 21, 2023 at 01:25:59PM -0400, Jason Gunthorpe wrote:
> > On Tue, Feb 21, 2023 at 06:51:48AM -1000, Tejun Heo wrote:
> > > cgroup, right? It makes little sense to me to separate the owner of the
> > > memory page and the pinner of it. They should be one and the same.
> > 
> > The owner and pinner are not always the same entity or we could just
> > use the page's cgroup.
> 
> Yeah, so, what I'm trying to say is that that might be the source of the
> problem. Is the current page ownership attribution correct 

It should be correct.

This mechanism is driven by pin_user_page(), (as it is the only API
that can actually create a pin) so the cgroup owner of the page is
broadly related to the "owner" of the VMA's inode.

The owner of the pin is the caller of pin_user_page(), which is
initated by some FD/proces that is not necessarily related to the
VMA's inode.

Eg concretely, something like io_uring will do something like:
  buffer = mmap()     <- Charge memcg for the pages
  fd = io_uring_setup(..)
  io_uring_register(fd,xx,buffer,..);   <- Charge the pincg for the pin

If mmap is a private anonymous VMA created by the same process then it
is likely the pages will have the same cgroup as io_uring_register and
the FD.

Otherwise the page cgroup is unconstrained. MAP_SHARED mappings will
have the page cgroup point at whatever cgroup was first to allocate
the page for the VMA's inode.

AFAIK there are few real use cases to establish a pin on MAP_SHARED
mappings outside your cgroup. However, it is possible, the APIs allow
it, and for security sandbox purposes we can't allow a process inside
a cgroup to triger a charge on a different cgroup. That breaks the
sandbox goal.

If memcg could support multiple owners then it would be logical that
the pinner would be one of the memcg owners.

> for whatever reason is determining the pinning ownership or should the page
> ownership be attributed the same way too? If they indeed need to differ,
> that probably would need pretty strong justifications.

It is inherent to how pin_user_pages() works. It is an API that
establishs pins on existing pages. There is nothing about it that says
who the page's memcg owner is.

I don't think we can do anything about this without breaking things.

Jason