On Thu, Sep 12, 2024 at 11:02 PM Suren Baghdasaryan <sur...@google.com> wrote:
> Add helper functions to speculatively perform operations without
> read-locking mmap_lock, expecting that mmap_lock will not be
> write-locked and mm is not modified from under us.
I think this is okay now, except for some comments that should be
fixed up. (Plus my gripe about the sequence count being 32-bit.)
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 6e3bdf8e38bc..5d8cdebd42bc 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -887,6 +887,9 @@ struct mm_struct {
> * Roughly speaking, incrementing the sequence number is
> * equivalent to releasing locks on VMAs; reading the sequence
> * number can be part of taking a read lock on a VMA.
> + * Incremented every time mmap_lock is write-locked/unlocked.
> + * Initialized to 0, therefore odd values indicate mmap_lock
> + * is write-locked and even values that it's released.
FWIW, I would still feel happier if this was a 64-bit number, though I
guess at least with uprobes the attack surface is not that large even
if you can wrap that counter... 2^31 counter increments are not all
that much, especially if someone introduces a kernel path in the
future that lets you repeatedly take the mmap_lock for writing within
a single syscall without doing much work, or maybe on some machine
where syscalls are really fast. I really don't like hinging memory
safety on how fast or slow some piece of code can run, unless we can
make strong arguments about it based on how many memory writes a CPU
core is capable of doing per second or stuff like that.
> diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h
> index de9dc20b01ba..a281519d0c12 100644
> --- a/include/linux/mmap_lock.h
> +++ b/include/linux/mmap_lock.h
> @@ -71,39 +71,86 @@ static inline void mmap_assert_write_locked(const struct
> mm_struct *mm)
> }
>
> #ifdef CONFIG_PER_VMA_LOCK
> +static inline void init_mm_lock_seq(struct mm_struct *mm)
> +{
> + mm->mm_lock_seq = 0;
> +}
> +
> /*
> - * Drop all currently-held per-VMA locks.
> - * This is called from the mmap_lock implementation directly before releasing
> - * a write-locked mmap_lock (or downgrading it to read-locked).
> - * This should normally NOT be called manually from other places.
> - * If you want to call this manually anyway, keep in mind that this will
> release
> - * *all* VMA write locks, including ones from further up the stack.
> + * Increment mm->mm_lock_seq when mmap_lock is write-locked (ACQUIRE
> semantics)
> + * or write-unlocked (RELEASE semantics).
> */
> -static inline void vma_end_write_all(struct mm_struct *mm)
> +static inline void inc_mm_lock_seq(struct mm_struct *mm, bool acquire)
> {
> mmap_assert_write_locked(mm);
Not a memory barriers thing, but maybe you could throw in some kind of
VM_WARN_ON() in the branches below that checks that the sequence
number is odd/even as expected, just to make extra sure...
> /*
> * Nobody can concurrently modify mm->mm_lock_seq due to exclusive
> * mmap_lock being held.
> - * We need RELEASE semantics here to ensure that preceding stores into
> - * the VMA take effect before we unlock it with this store.
> - * Pairs with ACQUIRE semantics in vma_start_read().
> */
> - smp_store_release(&mm->mm_lock_seq, mm->mm_lock_seq + 1);
> +
> + if (acquire) {
> + WRITE_ONCE(mm->mm_lock_seq, mm->mm_lock_seq + 1);
> + /*
> + * For ACQUIRE semantics we should ensure no following stores
> are
> + * reordered to appear before the mm->mm_lock_seq
> modification.
> + */
> + smp_wmb();
This is not really a full ACQUIRE; smp_wmb() only orders *stores*, not
loads, while a real ACQUIRE also prevents reads from being reordered
up above the atomic access. Please reword the comment to make it clear
that we don't have a full ACQUIRE here.
We can still have subsequent loads reordered up before the
mm->mm_lock_seq increment. But I guess that's probably fine as long as
nobody does anything exceedingly weird that involves lockless users
*writing* data that we have to read consistently, which wouldn't
really make sense...
So yeah, I guess this is probably fine, and it matches what
do_raw_write_seqcount_begin() is doing.
> + } else {
> + /*
> + * We need RELEASE semantics here to ensure that preceding
> stores
> + * into the VMA take effect before we unlock it with this
> store.
> + * Pairs with ACQUIRE semantics in vma_start_read().
> + */
> + smp_store_release(&mm->mm_lock_seq, mm->mm_lock_seq + 1);
> + }
> +}
> +
> +static inline bool mmap_lock_speculation_start(struct mm_struct *mm, int
> *seq)
> +{
> + /* Pairs with RELEASE semantics in inc_mm_lock_seq(). */
> + *seq = smp_load_acquire(&mm->mm_lock_seq);
> + /* Allow speculation if mmap_lock is not write-locked */
> + return (*seq & 1) == 0;
> +}
> +
> +static inline bool mmap_lock_speculation_end(struct mm_struct *mm, int seq)
> +{
> + /* Pairs with ACQUIRE semantics in inc_mm_lock_seq(). */
(see above, it's not actually a full ACQUIRE)
> + smp_rmb();
> + return seq == READ_ONCE(mm->mm_lock_seq);
> }
> +
> #else
> -static inline void vma_end_write_all(struct mm_struct *mm) {}
> +static inline void init_mm_lock_seq(struct mm_struct *mm) {}
> +static inline void inc_mm_lock_seq(struct mm_struct *mm, bool acquire) {}
> +static inline bool mmap_lock_speculation_start(struct mm_struct *mm, int
> *seq) { return false; }
> +static inline bool mmap_lock_speculation_end(struct mm_struct *mm, int seq)
> { return false; }
> #endif
>
> +/*
> + * Drop all currently-held per-VMA locks.
> + * This is called from the mmap_lock implementation directly before releasing
> + * a write-locked mmap_lock (or downgrading it to read-locked).
> + * This should normally NOT be called manually from other places.
> + * If you want to call this manually anyway, keep in mind that this will
> release
> + * *all* VMA write locks, including ones from further up the stack.
Outdated comment - now you are absolutely not allowed to call
vma_end_write_all() manually anymore, it would mess up the odd/even
state of the counter.
> + */
> +static inline void vma_end_write_all(struct mm_struct *mm)
> +{
> + inc_mm_lock_seq(mm, false);
> +}
