| 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
| ||
|
Message-ID: <CAJuCfpGJcrCkzOtaZDH98_oQK01+HNxHzzsf7SS95cXVRyXUPg@mail.gmail.com>
Date: Thu, 12 Dec 2024 20:48:52 -0800
From: Suren Baghdasaryan <surenb@...gle.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: Matthew Wilcox <willy@...radead.org>, akpm@...ux-foundation.org, liam.howlett@...cle.com,
lorenzo.stoakes@...cle.com, mhocko@...e.com, vbabka@...e.cz,
hannes@...xchg.org, mjguzik@...il.com, oliver.sang@...el.com,
mgorman@...hsingularity.net, david@...hat.com, peterx@...hat.com,
oleg@...hat.com, dave@...olabs.net, paulmck@...nel.org, brauner@...nel.org,
dhowells@...hat.com, hdanton@...a.com, hughd@...gle.com, minchan@...gle.com,
jannh@...gle.com, shakeel.butt@...ux.dev, souravpanda@...gle.com,
pasha.tatashin@...een.com, linux-mm@...ck.org, linux-kernel@...r.kernel.org,
kernel-team@...roid.com
Subject: Re: [PATCH 3/4] mm: replace rw_semaphore with atomic_t in vma_lock
On Thu, Dec 12, 2024 at 6:19 AM Suren Baghdasaryan <surenb@...gle.com> wrote:
>
> On Thu, Dec 12, 2024 at 6:17 AM Suren Baghdasaryan <surenb@...gle.com> wrote:
> >
> > On Thu, Dec 12, 2024 at 1:17 AM Peter Zijlstra <peterz@...radead.org> wrote:
> > >
> > > On Wed, Dec 11, 2024 at 07:01:16PM -0800, Suren Baghdasaryan wrote:
> > >
> > > > > > > I think your proposal should work. Let me try to code it and see if
> > > > > > > something breaks.
> > > >
> > > > Ok, I tried it out and things are a bit more complex:
> > > > 1. We should allow write-locking a detached VMA, IOW vma_start_write()
> > > > can be called when vm_refcnt is 0.
> > >
> > > This sounds dodgy, refcnt being zero basically means the object is dead
> > > and you shouldn't be touching it no more. Where does this happen and
> > > why?
> > >
> > > Notably, it being 0 means it is no longer in the mas tree and can't be
> > > found anymore.
> >
> > It happens when a newly created vma that was not yet attached
> > (vma->vm_refcnt = 0) is write-locked before being added into the vma
> > tree. For example:
> > mmap()
> > mmap_write_lock()
> > vma = vm_area_alloc() // vma->vm_refcnt = 0 (detached)
> > //vma attributes are initialized
> > vma_start_write() // write 0x8000 0001 into vma->vm_refcnt
> > mas_store_gfp()
> > vma_mark_attached()
> > mmap_write_lock() // vma_end_write_all()
>
> s/mmap_write_lock()/mmap_write_unlock()
> >
> > In this scenario, we write-lock the VMA before adding it into the tree
> > to prevent readers (pagefaults) from using it until we drop the
> > mmap_write_lock(). In your proposal, the first thing vma_start_write()
> > does is add(0x8000'0001) and that will trigger a warning.
> > For now instead of add(0x8000'0001) I can play this game to avoid the warning:
> >
> > if (refcount_inc_not_zero(&vma->vm_refcnt))
> > refcount_add(0x80000000, &vma->vm_refcnt);
> > else
> > refcount_set(&vma->vm_refcnt, 0x80000001);
> >
> > this refcount_set() works because vma with vm_refcnt==0 could not be
> > found by readers. I'm not sure this will still work when we add
> > TYPESAFE_BY_RCU and introduce vma reuse possibility.
> >
> > >
> > > > 2. Adding 0x80000000 saturates refcnt, so I have to use a lower bit
> > > > 0x40000000 to denote writers.
> > >
> > > I'm confused, what? We're talking about atomic_t, right?
> >
> > I thought you suggested using refcount_t. According to
> > https://elixir.bootlin.com/linux/v6.13-rc2/source/include/linux/refcount.h#L22
> > valid values would be [0..0x7fff_ffff] and 0x80000000 is outside of
> > that range. What am I missing?
> >
> > >
> > > > 3. Currently vma_mark_attached() can be called on an already attached
> > > > VMA. With vma->detached being a separate attribute that works fine but
> > > > when we combine it with the vm_lock things break (extra attach would
> > > > leak into lock count). I'll see if I can catch all the cases when we
> > > > do this and clean them up (not call vma_mark_attached() when not
> > > > necessary).
> > >
> > > Right, I hadn't looked at that thing in detail, that sounds like it
> > > needs a wee cleanup like you suggest.
> >
> > Yes, I'll embark on that today. Will see how much of a problem that is.
Ok, I think I was able to implement this in a way that ignores
duplicate attach/detach calls. One issue that I hit and don't know a
good way to fix is a circular dependency in the header files once I
try adding rcuwait into mm_struct. Once I include rcuwait.h into
mm_types.h leads to the following cycle:
In file included from ./arch/x86/include/asm/uaccess.h:12,
from ./include/linux/uaccess.h:12,
from ./include/linux/sched/task.h:13,
from ./include/linux/sched/signal.h:9,
from ./include/linux/rcuwait.h:6,
from ./include/linux/mm_types.h:22,
./arch/x86/include/asm/uaccess.h includes mm_types.h. But in fact
there is a shorter cycle:
rcuwait.h needs signal.h since it uses uses inlined signal_pending_state()
signal.h needs mm_types.h since it uses vm_fault_t
The way I worked around it for now is by removing signal.h include
from rcuwait.h and wrapping signal_pending_state() into a non-inlined
function so I can forward-declare it. That requires adding
linux/sched/signal.h or linux/sched/task.h into many other places:
arch/x86/coco/sev/core.c | 1 +
arch/x86/kernel/fpu/xstate.c | 1 +
block/blk-lib.c | 1 +
block/ioctl.c | 1 +
drivers/accel/qaic/qaic_control.c | 1 +
drivers/base/firmware_loader/main.c | 1 +
drivers/block/ublk_drv.c | 1 +
drivers/crypto/ccp/sev-dev.c | 1 +
drivers/dma-buf/heaps/cma_heap.c | 1 +
drivers/dma-buf/heaps/system_heap.c | 1 +
drivers/gpio/gpio-sloppy-logic-analyzer.c | 1 +
drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 1 +
drivers/iommu/iommufd/ioas.c | 1 +
drivers/iommu/iommufd/pages.c | 1 +
.../staging/vc04_services/interface/vchiq_arm/vchiq_dev.c | 1 +
drivers/tty/n_tty.c | 1 +
fs/bcachefs/fs-io-buffered.c | 1 +
fs/bcachefs/journal_reclaim.c | 1 +
fs/bcachefs/thread_with_file.c | 1 +
fs/bcachefs/util.c | 1 +
fs/btrfs/defrag.h | 1 +
fs/btrfs/fiemap.c | 2 ++
fs/btrfs/free-space-cache.h | 1 +
fs/btrfs/reflink.c | 1 +
fs/exfat/balloc.c | 1 +
fs/gfs2/ops_fstype.c | 1 +
fs/kernel_read_file.c | 1 +
fs/netfs/buffered_write.c | 1 +
fs/zonefs/file.c | 1 +
include/linux/fs.h | 2 +-
include/linux/rcuwait.h | 4 ++--
io_uring/io_uring.h | 1 +
kernel/dma/map_benchmark.c | 1 +
kernel/futex/core.c | 1 +
kernel/futex/pi.c | 1 +
kernel/rcu/update.c | 5 +++++
kernel/task_work.c | 1 +
lib/kunit/user_alloc.c | 1 +
mm/damon/vaddr.c | 1 +
mm/memcontrol-v1.c | 1 +
mm/shrinker_debug.c | 1 +
net/dns_resolver/dns_key.c | 1 +
42 files changed, 48 insertions(+), 3 deletions(-)
I'm not sure if this is the best way to deal with this circular
dependency. Any other ideas?
Powered by blists - more mailing lists