* Re: [PATCH] RDMA/mlx5: Use DIV_ROUND_UP_ULL macro to allow 32 bit to build
From: Michal Kubecek @ 2019-05-23 6:58 UTC (permalink / raw)
To: Steven Rostedt
Cc: LKML, Leon Romanovsky, Doug Ledford, Jason Gunthorpe, linux-rdma,
Linus Torvalds
In-Reply-To: <20190522145450.25ff483d@gandalf.local.home>
On Wed, May 22, 2019 at 02:54:50PM -0400, Steven Rostedt wrote:
>
> From: Steven Rostedt (VMware) <rostedt@goodmis.org>
>
> When testing 32 bit x86, my build failed with:
>
> ERROR: "__udivdi3" [drivers/infiniband/hw/mlx5/mlx5_ib.ko] undefined!
>
> It appears that a few non-ULL roundup() calls were made, which uses a
> normal division against a 64 bit number. This is fine for x86_64, but
> on 32 bit x86, it causes the compiler to look for a helper function
> __udivdi3, which we do not have in the kernel, and thus fails to build.
>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
...
> diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
> index abac70ad5c7c..40d4c5f7ea43 100644
> --- a/drivers/infiniband/hw/mlx5/main.c
> +++ b/drivers/infiniband/hw/mlx5/main.c
> @@ -2344,7 +2344,7 @@ static int handle_alloc_dm_sw_icm(struct ib_ucontext *ctx,
> /* Allocation size must a multiple of the basic block size
> * and a power of 2.
> */
> - act_size = roundup(attr->length, MLX5_SW_ICM_BLOCK_SIZE(dm_db->dev));
> + act_size = DIV_ROUND_UP_ULL(attr->length, MLX5_SW_ICM_BLOCK_SIZE(dm_db->dev));
> act_size = roundup_pow_of_two(act_size);
>
> dm->size = act_size;
This seems wrong: roundup() rounds up to a multiple of second argument
but DIV_ROUND_UP_ULL() would divide with rounding up.
Michal Kubecek
^ permalink raw reply
* Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
From: Jason Gunthorpe @ 2019-05-23 0:20 UTC (permalink / raw)
To: Dave Martin
Cc: Catalin Marinas, Mark Rutland, kvm, Szabolcs Nagy, Will Deacon,
dri-devel, linux-mm, linux-kselftest, Felix Kuehling,
Vincenzo Frascino, Jacob Bramley, Leon Romanovsky, linux-rdma,
amd-gfx, Dmitry Vyukov, Evgeniy Stepanov, linux-media,
Kevin Brodsky, Kees Cook, Ruben Ayrapetyan, Andrey Konovalov,
Ramana
In-Reply-To: <20190522134925.GV28398@e103592.cambridge.arm.com>
On Wed, May 22, 2019 at 02:49:28PM +0100, Dave Martin wrote:
> On Tue, May 21, 2019 at 03:48:56PM -0300, Jason Gunthorpe wrote:
> > On Fri, May 17, 2019 at 03:49:31PM +0100, Catalin Marinas wrote:
> >
> > > The tagged pointers (whether hwasan or MTE) should ideally be a
> > > transparent feature for the application writer but I don't think we can
> > > solve it entirely and make it seamless for the multitude of ioctls().
> > > I'd say you only opt in to such feature if you know what you are doing
> > > and the user code takes care of specific cases like ioctl(), hence the
> > > prctl() proposal even for the hwasan.
> >
> > I'm not sure such a dire view is warrented..
> >
> > The ioctl situation is not so bad, other than a few special cases,
> > most drivers just take a 'void __user *' and pass it as an argument to
> > some function that accepts a 'void __user *'. sparse et al verify
> > this.
> >
> > As long as the core functions do the right thing the drivers will be
> > OK.
> >
> > The only place things get dicy is if someone casts to unsigned long
> > (ie for vma work) but I think that reflects that our driver facing
> > APIs for VMAs are compatible with static analysis (ie I have no
> > earthly idea why get_user_pages() accepts an unsigned long), not that
> > this is too hard.
>
> If multiple people will care about this, perhaps we should try to
> annotate types more explicitly in SYSCALL_DEFINEx() and ABI data
> structures.
>
> For example, we could have a couple of mutually exclusive modifiers
>
> T __object *
> T __vaddr * (or U __vaddr)
>
> In the first case the pointer points to an object (in the C sense)
> that the call may dereference but not use for any other purpose.
How would you use these two differently?
So far the kernel has worked that __user should tag any pointer that
is from userspace and then you can't do anything with it until you
transform it into a kernel something
> to tell static analysers the real type of pointers smuggled through
> UAPI disguised as other types (*cough* KVM, etc.)
Yes, that would help alot, we often have to pass pointers through a
u64 in the uAPI, and there is no static checker support to make sure
they are run through the u64_to_user_ptr() helper.
Jason
^ permalink raw reply
* Re: [PATCH v4 0/1] Use HMM for ODP v4
From: Jason Gunthorpe @ 2019-05-22 23:57 UTC (permalink / raw)
To: Jerome Glisse
Cc: linux-kernel, linux-rdma, Leon Romanovsky, Doug Ledford,
Artemy Kovalyov, Moni Shoua, Mike Marciniszyn, Kaike Wan,
Dennis Dalessandro
In-Reply-To: <20190522174852.GA23038@redhat.com>
On Wed, May 22, 2019 at 01:48:52PM -0400, Jerome Glisse wrote:
> > > So attached is a rebase on top of 5.2-rc1, i have tested with pingpong
> > > (prefetch and not and different sizes). Seems to work ok.
> >
> > Urk, it already doesn't apply to the rdma tree :(
> >
> > The conflicts are a little more extensive than I'd prefer to handle..
> > Can I ask you to rebase it on top of this branch please:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/log/?h=wip/jgg-for-next
> >
> > Specifically it conflicts with this patch:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/commit/?h=wip/jgg-for-next&id=d2183c6f1958e6b6dfdde279f4cee04280710e34
There is at least one more serious blocker here:
config ARCH_HAS_HMM_MIRROR
bool
default y
depends on (X86_64 || PPC64)
depends on MMU && 64BIT
I can't loose ARM64 support for ODP by merging this, that is too
serious of a regression.
Can you fix it?
Thanks,
Jason
^ permalink raw reply
* Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
From: enh @ 2019-05-22 23:09 UTC (permalink / raw)
To: Evgenii Stepanov
Cc: Kees Cook, Catalin Marinas, Andrey Konovalov, Khalid Aziz,
Linux ARM, Linux Memory Management List, LKML, amd-gfx, dri-devel,
linux-rdma, linux-media, kvm, open list:KERNEL SELFTEST FRAMEWORK,
Vincenzo Frascino, Will Deacon, Mark Rutland, Andrew Morton,
Greg Kroah-Hartman, Yishai Hadas
In-Reply-To: <CAFKCwrjOjdJAbcABp3qxwyYy+hgfyQirvmqGkDSJVJe5pSz0Uw@mail.gmail.com>
On Wed, May 22, 2019 at 4:03 PM Evgenii Stepanov <eugenis@google.com> wrote:
>
> On Wed, May 22, 2019 at 1:47 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Wed, May 22, 2019 at 05:35:27PM +0100, Catalin Marinas wrote:
> > > The two hard requirements I have for supporting any new hardware feature
> > > in Linux are (1) a single kernel image binary continues to run on old
> > > hardware while making use of the new feature if available and (2) old
> > > user space continues to run on new hardware while new user space can
> > > take advantage of the new feature.
> >
> > Agreed! And I think the series meets these requirements, yes?
> >
> > > For MTE, we just can't enable it by default since there are applications
> > > who use the top byte of a pointer and expect it to be ignored rather
> > > than failing with a mismatched tag. Just think of a hwasan compiled
> > > binary where TBI is expected to work and you try to run it with MTE
> > > turned on.
> >
> > Ah! Okay, here's the use-case I wasn't thinking of: the concern is TBI
> > conflicting with MTE. And anything that starts using TBI suddenly can't
> > run in the future because it's being interpreted as MTE bits? (Is that
> > the ABI concern? I feel like we got into the weeds about ioctl()s and
> > one-off bugs...)
> >
> > So there needs to be some way to let the kernel know which of three
> > things it should be doing:
> > 1- leaving userspace addresses as-is (present)
> > 2- wiping the top bits before using (this series)
> > 3- wiping the top bits for most things, but retaining them for MTE as
> > needed (the future)
> >
> > I expect MTE to be the "default" in the future. Once a system's libc has
> > grown support for it, everything will be trying to use MTE. TBI will be
> > the special case (but TBI is effectively a prerequisite).
> >
> > AFAICT, the only difference I see between 2 and 3 will be the tag handling
> > in usercopy (all other places will continue to ignore the top bits). Is
> > that accurate?
> >
> > Is "1" a per-process state we want to keep? (I assume not, but rather it
> > is available via no TBI/MTE CONFIG or a boot-time option, if at all?)
> >
> > To choose between "2" and "3", it seems we need a per-process flag to
> > opt into TBI (and out of MTE). For userspace, how would a future binary
> > choose TBI over MTE? If it's a library issue, we can't use an ELF bit,
> > since the choice may be "late" after ELF load (this implies the need
> > for a prctl().) If it's binary-only ("built with HWKASan") then an ELF
> > bit seems sufficient. And without the marking, I'd expect the kernel to
> > enforce MTE when there are high bits.
> >
> > > I would also expect the C library or dynamic loader to check for the
> > > presence of a HWCAP_MTE bit before starting to tag memory allocations,
> > > otherwise it would get SIGILL on the first MTE instruction it tries to
> > > execute.
> >
> > I've got the same question as Elliot: aren't MTE instructions just NOP
> > to older CPUs? I.e. if the CPU (or kernel) don't support it, it just
> > gets entirely ignored: checking is only needed to satisfy curiosity
> > or behavioral expectations.
>
> MTE instructions are not NOP. Most of them have side effects (changing
> register values, zeroing memory).
no, i meant "they're encoded in a space that was previously no-ops, so
running on MTE code on old hardware doesn't cause SIGILL".
> This only matters for stack tagging, though. Heap tagging is a runtime
> decision in the allocator.
>
> If an image needs to run on old hardware, it will have to do heap tagging only.
>
> > To me, the conflict seems to be using TBI in the face of expecting MTE to
> > be the default state of the future. (But the internal changes needed
> > for TBI -- this series -- is a prereq for MTE.)
> >
> > --
> > Kees Cook
^ permalink raw reply
* Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
From: Evgenii Stepanov @ 2019-05-22 23:03 UTC (permalink / raw)
To: Kees Cook
Cc: Catalin Marinas, enh, Andrey Konovalov, Khalid Aziz, Linux ARM,
Linux Memory Management List, LKML, amd-gfx, dri-devel,
linux-rdma, linux-media, kvm, open list:KERNEL SELFTEST FRAMEWORK,
Vincenzo Frascino, Will Deacon, Mark Rutland, Andrew Morton,
Greg Kroah-Hartman, Yishai Hadas, Felix
In-Reply-To: <201905221316.865581CF@keescook>
On Wed, May 22, 2019 at 1:47 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, May 22, 2019 at 05:35:27PM +0100, Catalin Marinas wrote:
> > The two hard requirements I have for supporting any new hardware feature
> > in Linux are (1) a single kernel image binary continues to run on old
> > hardware while making use of the new feature if available and (2) old
> > user space continues to run on new hardware while new user space can
> > take advantage of the new feature.
>
> Agreed! And I think the series meets these requirements, yes?
>
> > For MTE, we just can't enable it by default since there are applications
> > who use the top byte of a pointer and expect it to be ignored rather
> > than failing with a mismatched tag. Just think of a hwasan compiled
> > binary where TBI is expected to work and you try to run it with MTE
> > turned on.
>
> Ah! Okay, here's the use-case I wasn't thinking of: the concern is TBI
> conflicting with MTE. And anything that starts using TBI suddenly can't
> run in the future because it's being interpreted as MTE bits? (Is that
> the ABI concern? I feel like we got into the weeds about ioctl()s and
> one-off bugs...)
>
> So there needs to be some way to let the kernel know which of three
> things it should be doing:
> 1- leaving userspace addresses as-is (present)
> 2- wiping the top bits before using (this series)
> 3- wiping the top bits for most things, but retaining them for MTE as
> needed (the future)
>
> I expect MTE to be the "default" in the future. Once a system's libc has
> grown support for it, everything will be trying to use MTE. TBI will be
> the special case (but TBI is effectively a prerequisite).
>
> AFAICT, the only difference I see between 2 and 3 will be the tag handling
> in usercopy (all other places will continue to ignore the top bits). Is
> that accurate?
>
> Is "1" a per-process state we want to keep? (I assume not, but rather it
> is available via no TBI/MTE CONFIG or a boot-time option, if at all?)
>
> To choose between "2" and "3", it seems we need a per-process flag to
> opt into TBI (and out of MTE). For userspace, how would a future binary
> choose TBI over MTE? If it's a library issue, we can't use an ELF bit,
> since the choice may be "late" after ELF load (this implies the need
> for a prctl().) If it's binary-only ("built with HWKASan") then an ELF
> bit seems sufficient. And without the marking, I'd expect the kernel to
> enforce MTE when there are high bits.
>
> > I would also expect the C library or dynamic loader to check for the
> > presence of a HWCAP_MTE bit before starting to tag memory allocations,
> > otherwise it would get SIGILL on the first MTE instruction it tries to
> > execute.
>
> I've got the same question as Elliot: aren't MTE instructions just NOP
> to older CPUs? I.e. if the CPU (or kernel) don't support it, it just
> gets entirely ignored: checking is only needed to satisfy curiosity
> or behavioral expectations.
MTE instructions are not NOP. Most of them have side effects (changing
register values, zeroing memory).
This only matters for stack tagging, though. Heap tagging is a runtime
decision in the allocator.
If an image needs to run on old hardware, it will have to do heap tagging only.
> To me, the conflict seems to be using TBI in the face of expecting MTE to
> be the default state of the future. (But the internal changes needed
> for TBI -- this series -- is a prereq for MTE.)
>
> --
> Kees Cook
^ permalink raw reply
* Re: [PATCH v4 0/1] Use HMM for ODP v4
From: Jason Gunthorpe @ 2019-05-22 22:52 UTC (permalink / raw)
To: Jerome Glisse
Cc: linux-mm, linux-kernel, linux-rdma, Leon Romanovsky, Doug Ledford,
Artemy Kovalyov, Moni Shoua, Mike Marciniszyn, Kaike Wan,
Dennis Dalessandro
In-Reply-To: <20190522224211.GF20179@redhat.com>
On Wed, May 22, 2019 at 06:42:11PM -0400, Jerome Glisse wrote:
> > > The way race is avoided is because mm->hmm will either be NULL or
> > > point to another hmm struct before an existing hmm is free.
> >
> > There is no locking on mm->hmm so it is useless to prevent races.
>
> There is locking on mm->hmm
Not in mm_get_hmm()
> > So we agree this patch is necessary? Can you test it an ack it please?
>
> A slightly different patch than this one is necessary i will work on
> it tomorrow.
I think if you want something different you should give feedback on
this patch. You haven't rasied any defects with it.
Jason
^ permalink raw reply
* Re: [PATCH v4 0/1] Use HMM for ODP v4
From: Jason Gunthorpe @ 2019-05-22 22:43 UTC (permalink / raw)
To: Jerome Glisse
Cc: linux-kernel, linux-rdma, Leon Romanovsky, Doug Ledford,
Artemy Kovalyov, Moni Shoua, Mike Marciniszyn, Kaike Wan,
Dennis Dalessandro, linux-mm
In-Reply-To: <20190522214917.GA20179@redhat.com>
On Wed, May 22, 2019 at 05:49:18PM -0400, Jerome Glisse wrote:
> > > > So why is mm suddenly guarenteed valid? It was a bug report that
> > > > triggered the race the mmget_not_zero is fixing, so I need a better
> > > > explanation why it is now safe. From what I see the hmm_range_fault
> > > > is doing stuff like find_vma without an active mmget??
> > >
> > > So the mm struct can not go away as long as we hold a reference on
> > > the hmm struct and we hold a reference on it through both hmm_mirror
> > > and hmm_range struct. So struct mm can not go away and thus it is
> > > safe to try to take its mmap_sem.
> >
> > This was always true here, though, so long as the umem_odp exists the
> > the mm has a grab on it. But a grab is not a get..
> >
> > The point here was the old code needed an mmget() in order to do
> > get_user_pages_remote()
> >
> > If hmm does not need an external mmget() then fine, we delete this
> > stuff and rely on hmm.
> >
> > But I don't think that is true as we have:
> >
> > CPU 0 CPU1
> > mmput()
> > __mmput()
> > exit_mmap()
> > down_read(&mm->mmap_sem);
> > hmm_range_dma_map(range, device,..
> > ret = hmm_range_fault(range, block);
> > if (hmm->mm == NULL || hmm->dead)
> > mmu_notifier_release()
> > hmm->dead = true
> > vma = find_vma(hmm->mm, start);
> > .. rb traversal .. while (vma) remove_vma()
> >
> > *goes boom*
> >
> > I think this is violating the basic constraint of the mm by acting on
> > a mm's VMA's without holding a mmget() to prevent concurrent
> > destruction.
> >
> > In other words, mmput() destruction does not respect the mmap_sem - so
> > holding the mmap sem alone is not enough locking.
> >
> > The unlucked hmm->dead simply can't save this. Frankly every time I
> > look a struct with 'dead' in it, I find races like this.
> >
> > Thus we should put the mmget_notzero back in.
>
> So for some reason i thought exit_mmap() was setting the mm_rb
> to empty node and flushing vmacache so that find_vma() would
> fail.
It would still be racy without locks.
> Note that right before find_vma() there is also range->valid
> check which will also intercept mm release.
There is no locking on range->valid so it is just moves the race
around. You can't solve races with unlocked/non-atomic variables.
> Anyway the easy fix is to get ref on mm user in range_register.
Yes a mmget_not_zero inside range_register would be fine.
How do you want to handle that patch?
> > I saw some other funky looking stuff in hmm as well..
> >
> > > Hence it is safe to take mmap_sem and it is safe to call in hmm, if
> > > mm have been kill it will return EFAULT and this will propagate to
> > > RDMA.
> >
> > > As per_mm i removed the per_mm->mm = NULL from release so that it is
> > > always safe to use that field even in face of racing mm "killing".
> >
> > Yes, that certainly wasn't good.
> >
> > > > > - * An array of the pages included in the on-demand paging umem.
> > > > > - * Indices of pages that are currently not mapped into the device will
> > > > > - * contain NULL.
> > > > > + * An array of the pages included in the on-demand paging umem. Indices
> > > > > + * of pages that are currently not mapped into the device will contain
> > > > > + * 0.
> > > > > */
> > > > > - struct page **page_list;
> > > > > + uint64_t *pfns;
> > > >
> > > > Are these actually pfns, or are they mangled with some shift? (what is range->pfn_shift?)
> > >
> > > They are not pfns they have flags (hence range->pfn_shift) at the
> > > bottoms i just do not have a better name for this.
> >
> > I think you need to have a better name then
>
> Suggestion ? i have no idea for a better name, it has pfn value
> in it.
pfn_flags?
Jason
^ permalink raw reply
* Re: [PATCH v4 0/1] Use HMM for ODP v4
From: Jerome Glisse @ 2019-05-22 22:42 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: linux-mm, linux-kernel, linux-rdma, Leon Romanovsky, Doug Ledford,
Artemy Kovalyov, Moni Shoua, Mike Marciniszyn, Kaike Wan,
Dennis Dalessandro
In-Reply-To: <20190522223906.GA15389@ziepe.ca>
On Wed, May 22, 2019 at 07:39:06PM -0300, Jason Gunthorpe wrote:
> On Wed, May 22, 2019 at 06:04:20PM -0400, Jerome Glisse wrote:
> > On Wed, May 22, 2019 at 05:12:47PM -0300, Jason Gunthorpe wrote:
> > > On Wed, May 22, 2019 at 01:48:52PM -0400, Jerome Glisse wrote:
> > >
> > > > static void put_per_mm(struct ib_umem_odp *umem_odp)
> > > > {
> > > > struct ib_ucontext_per_mm *per_mm = umem_odp->per_mm;
> > > > @@ -325,9 +283,10 @@ static void put_per_mm(struct ib_umem_odp *umem_odp)
> > > > up_write(&per_mm->umem_rwsem);
> > > >
> > > > WARN_ON(!RB_EMPTY_ROOT(&per_mm->umem_tree.rb_root));
> > > > - mmu_notifier_unregister_no_release(&per_mm->mn, per_mm->mm);
> > > > + hmm_mirror_unregister(&per_mm->mirror);
> > > > put_pid(per_mm->tgid);
> > > > - mmu_notifier_call_srcu(&per_mm->rcu, free_per_mm);
> > > > +
> > > > + kfree(per_mm);
> > >
> > > Notice that mmu_notifier only uses SRCU to fence in-progress ops
> > > callbacks, so I think hmm internally has the bug that this ODP
> > > approach prevents.
> > >
> > > hmm should follow the same pattern ODP has and 'kfree_srcu' the hmm
> > > struct, use container_of in the mmu_notifier callbacks, and use the
> > > otherwise vestigal kref_get_unless_zero() to bail:
> > >
> > > From 0cb536dc0150ba964a1d655151d7b7a84d0f915a Mon Sep 17 00:00:00 2001
> > > From: Jason Gunthorpe <jgg@mellanox.com>
> > > Date: Wed, 22 May 2019 16:52:52 -0300
> > > Subject: [PATCH] hmm: Fix use after free with struct hmm in the mmu notifiers
> > >
> > > mmu_notifier_unregister_no_release() is not a fence and the mmu_notifier
> > > system will continue to reference hmm->mn until the srcu grace period
> > > expires.
> > >
> > > CPU0 CPU1
> > > __mmu_notifier_invalidate_range_start()
> > > srcu_read_lock
> > > hlist_for_each ()
> > > // mn == hmm->mn
> > > hmm_mirror_unregister()
> > > hmm_put()
> > > hmm_free()
> > > mmu_notifier_unregister_no_release()
> > > hlist_del_init_rcu(hmm-mn->list)
> > > mn->ops->invalidate_range_start(mn, range);
> > > mm_get_hmm()
> > > mm->hmm = NULL;
> > > kfree(hmm)
> > > mutex_lock(&hmm->lock);
> > >
> > > Use SRCU to kfree the hmm memory so that the notifiers can rely on hmm
> > > existing. Get the now-safe hmm struct through container_of and directly
> > > check kref_get_unless_zero to lock it against free.
> >
> > It is already badly handled with BUG_ON()
>
> You can't crash the kernel because userspace forced a race, and no it
> isn't handled today because there is no RCU locking in mm_get_hmm nor
> is there a kfree_rcu for the struct hmm to make the
> kref_get_unless_zero work without use-after-free.
>
> > i just need to convert those to return and to use
> > mmu_notifier_call_srcu() to free hmm struct.
>
> Isn't that what this patch does?
Yes but other chunk just need to replace BUG_ON with return
>
> > The way race is avoided is because mm->hmm will either be NULL or
> > point to another hmm struct before an existing hmm is free.
>
> There is no locking on mm->hmm so it is useless to prevent races.
There is locking on mm->hmm
>
> > Also if range_start/range_end use kref_get_unless_zero() but right
> > now this is BUG_ON if it turn out to be NULL, it should just return
> > on NULL.
>
> Still needs rcu.
>
> Also the container_of is necessary to avoid some race where you could
> be doing:
>
> CPU0 CPU1 CPU2
> hlist_for_each ()
> mmu_notifier_unregister_no_release(hmm1)
> spin_lock(&mm->page_table_lock);
> mm->hmm = NULL
> spin_unlock(&mm->page_table_lock);
> hmm2 = hmm_get_or_create()
> mn == hmm1->mn
> mn->ops->invalidate_range_start(mn, range)
> mm_get_mm() == hmm2
> hist_for_each con't
> mn == hmm2->mn
> mn->ops->invalidate_range_start(mn, range)
> mm_get_mm() == hmm2
>
> Now we called the same notifier twice on hmm2. Ooops.
>
> There is no reason to risk this confusion just to avoid container_of.
>
> So we agree this patch is necessary? Can you test it an ack it please?
A slightly different patch than this one is necessary i will work on
it tomorrow.
Cheers,
Jérôme
^ permalink raw reply
* Re: [PATCH v4 0/1] Use HMM for ODP v4
From: Jason Gunthorpe @ 2019-05-22 22:39 UTC (permalink / raw)
To: Jerome Glisse
Cc: linux-mm, linux-kernel, linux-rdma, Leon Romanovsky, Doug Ledford,
Artemy Kovalyov, Moni Shoua, Mike Marciniszyn, Kaike Wan,
Dennis Dalessandro
In-Reply-To: <20190522220419.GB20179@redhat.com>
On Wed, May 22, 2019 at 06:04:20PM -0400, Jerome Glisse wrote:
> On Wed, May 22, 2019 at 05:12:47PM -0300, Jason Gunthorpe wrote:
> > On Wed, May 22, 2019 at 01:48:52PM -0400, Jerome Glisse wrote:
> >
> > > static void put_per_mm(struct ib_umem_odp *umem_odp)
> > > {
> > > struct ib_ucontext_per_mm *per_mm = umem_odp->per_mm;
> > > @@ -325,9 +283,10 @@ static void put_per_mm(struct ib_umem_odp *umem_odp)
> > > up_write(&per_mm->umem_rwsem);
> > >
> > > WARN_ON(!RB_EMPTY_ROOT(&per_mm->umem_tree.rb_root));
> > > - mmu_notifier_unregister_no_release(&per_mm->mn, per_mm->mm);
> > > + hmm_mirror_unregister(&per_mm->mirror);
> > > put_pid(per_mm->tgid);
> > > - mmu_notifier_call_srcu(&per_mm->rcu, free_per_mm);
> > > +
> > > + kfree(per_mm);
> >
> > Notice that mmu_notifier only uses SRCU to fence in-progress ops
> > callbacks, so I think hmm internally has the bug that this ODP
> > approach prevents.
> >
> > hmm should follow the same pattern ODP has and 'kfree_srcu' the hmm
> > struct, use container_of in the mmu_notifier callbacks, and use the
> > otherwise vestigal kref_get_unless_zero() to bail:
> >
> > From 0cb536dc0150ba964a1d655151d7b7a84d0f915a Mon Sep 17 00:00:00 2001
> > From: Jason Gunthorpe <jgg@mellanox.com>
> > Date: Wed, 22 May 2019 16:52:52 -0300
> > Subject: [PATCH] hmm: Fix use after free with struct hmm in the mmu notifiers
> >
> > mmu_notifier_unregister_no_release() is not a fence and the mmu_notifier
> > system will continue to reference hmm->mn until the srcu grace period
> > expires.
> >
> > CPU0 CPU1
> > __mmu_notifier_invalidate_range_start()
> > srcu_read_lock
> > hlist_for_each ()
> > // mn == hmm->mn
> > hmm_mirror_unregister()
> > hmm_put()
> > hmm_free()
> > mmu_notifier_unregister_no_release()
> > hlist_del_init_rcu(hmm-mn->list)
> > mn->ops->invalidate_range_start(mn, range);
> > mm_get_hmm()
> > mm->hmm = NULL;
> > kfree(hmm)
> > mutex_lock(&hmm->lock);
> >
> > Use SRCU to kfree the hmm memory so that the notifiers can rely on hmm
> > existing. Get the now-safe hmm struct through container_of and directly
> > check kref_get_unless_zero to lock it against free.
>
> It is already badly handled with BUG_ON()
You can't crash the kernel because userspace forced a race, and no it
isn't handled today because there is no RCU locking in mm_get_hmm nor
is there a kfree_rcu for the struct hmm to make the
kref_get_unless_zero work without use-after-free.
> i just need to convert those to return and to use
> mmu_notifier_call_srcu() to free hmm struct.
Isn't that what this patch does?
> The way race is avoided is because mm->hmm will either be NULL or
> point to another hmm struct before an existing hmm is free.
There is no locking on mm->hmm so it is useless to prevent races.
> Also if range_start/range_end use kref_get_unless_zero() but right
> now this is BUG_ON if it turn out to be NULL, it should just return
> on NULL.
Still needs rcu.
Also the container_of is necessary to avoid some race where you could
be doing:
CPU0 CPU1 CPU2
hlist_for_each ()
mmu_notifier_unregister_no_release(hmm1)
spin_lock(&mm->page_table_lock);
mm->hmm = NULL
spin_unlock(&mm->page_table_lock);
hmm2 = hmm_get_or_create()
mn == hmm1->mn
mn->ops->invalidate_range_start(mn, range)
mm_get_mm() == hmm2
hist_for_each con't
mn == hmm2->mn
mn->ops->invalidate_range_start(mn, range)
mm_get_mm() == hmm2
Now we called the same notifier twice on hmm2. Ooops.
There is no reason to risk this confusion just to avoid container_of.
So we agree this patch is necessary? Can you test it an ack it please?
Jason
^ permalink raw reply
* Re: [PATCH v4 0/1] Use HMM for ODP v4
From: Jerome Glisse @ 2019-05-22 22:06 UTC (permalink / raw)
To: Ralph Campbell
Cc: Jason Gunthorpe, linux-mm, linux-kernel, linux-rdma,
Leon Romanovsky, Doug Ledford, Artemy Kovalyov, Moni Shoua,
Mike Marciniszyn, Kaike Wan, Dennis Dalessandro
In-Reply-To: <05e7f491-b8a4-4214-ab75-9ecf1128aaa6@nvidia.com>
On Wed, May 22, 2019 at 02:12:31PM -0700, Ralph Campbell wrote:
>
> On 5/22/19 1:12 PM, Jason Gunthorpe wrote:
> > On Wed, May 22, 2019 at 01:48:52PM -0400, Jerome Glisse wrote:
> >
> > > static void put_per_mm(struct ib_umem_odp *umem_odp)
> > > {
> > > struct ib_ucontext_per_mm *per_mm = umem_odp->per_mm;
> > > @@ -325,9 +283,10 @@ static void put_per_mm(struct ib_umem_odp *umem_odp)
> > > up_write(&per_mm->umem_rwsem);
> > > WARN_ON(!RB_EMPTY_ROOT(&per_mm->umem_tree.rb_root));
> > > - mmu_notifier_unregister_no_release(&per_mm->mn, per_mm->mm);
> > > + hmm_mirror_unregister(&per_mm->mirror);
> > > put_pid(per_mm->tgid);
> > > - mmu_notifier_call_srcu(&per_mm->rcu, free_per_mm);
> > > +
> > > + kfree(per_mm);
> >
> > Notice that mmu_notifier only uses SRCU to fence in-progress ops
> > callbacks, so I think hmm internally has the bug that this ODP
> > approach prevents.
> >
> > hmm should follow the same pattern ODP has and 'kfree_srcu' the hmm
> > struct, use container_of in the mmu_notifier callbacks, and use the
> > otherwise vestigal kref_get_unless_zero() to bail:
>
> You might also want to look at my patch where
> I try to fix some of these same issues (5/5).
>
> https://marc.info/?l=linux-mm&m=155718572908765&w=2
I need to review the patchset but i do not want to invert referencing
ie having mm hold reference on hmm. Will review tommorrow. I wanted to
do that today but did not had time.
^ permalink raw reply
* Re: [PATCH v4 0/1] Use HMM for ODP v4
From: Jerome Glisse @ 2019-05-22 22:04 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: linux-mm, linux-kernel, linux-rdma, Leon Romanovsky, Doug Ledford,
Artemy Kovalyov, Moni Shoua, Mike Marciniszyn, Kaike Wan,
Dennis Dalessandro
In-Reply-To: <20190522201247.GH6054@ziepe.ca>
On Wed, May 22, 2019 at 05:12:47PM -0300, Jason Gunthorpe wrote:
> On Wed, May 22, 2019 at 01:48:52PM -0400, Jerome Glisse wrote:
>
> > static void put_per_mm(struct ib_umem_odp *umem_odp)
> > {
> > struct ib_ucontext_per_mm *per_mm = umem_odp->per_mm;
> > @@ -325,9 +283,10 @@ static void put_per_mm(struct ib_umem_odp *umem_odp)
> > up_write(&per_mm->umem_rwsem);
> >
> > WARN_ON(!RB_EMPTY_ROOT(&per_mm->umem_tree.rb_root));
> > - mmu_notifier_unregister_no_release(&per_mm->mn, per_mm->mm);
> > + hmm_mirror_unregister(&per_mm->mirror);
> > put_pid(per_mm->tgid);
> > - mmu_notifier_call_srcu(&per_mm->rcu, free_per_mm);
> > +
> > + kfree(per_mm);
>
> Notice that mmu_notifier only uses SRCU to fence in-progress ops
> callbacks, so I think hmm internally has the bug that this ODP
> approach prevents.
>
> hmm should follow the same pattern ODP has and 'kfree_srcu' the hmm
> struct, use container_of in the mmu_notifier callbacks, and use the
> otherwise vestigal kref_get_unless_zero() to bail:
>
> From 0cb536dc0150ba964a1d655151d7b7a84d0f915a Mon Sep 17 00:00:00 2001
> From: Jason Gunthorpe <jgg@mellanox.com>
> Date: Wed, 22 May 2019 16:52:52 -0300
> Subject: [PATCH] hmm: Fix use after free with struct hmm in the mmu notifiers
>
> mmu_notifier_unregister_no_release() is not a fence and the mmu_notifier
> system will continue to reference hmm->mn until the srcu grace period
> expires.
>
> CPU0 CPU1
> __mmu_notifier_invalidate_range_start()
> srcu_read_lock
> hlist_for_each ()
> // mn == hmm->mn
> hmm_mirror_unregister()
> hmm_put()
> hmm_free()
> mmu_notifier_unregister_no_release()
> hlist_del_init_rcu(hmm-mn->list)
> mn->ops->invalidate_range_start(mn, range);
> mm_get_hmm()
> mm->hmm = NULL;
> kfree(hmm)
> mutex_lock(&hmm->lock);
>
> Use SRCU to kfree the hmm memory so that the notifiers can rely on hmm
> existing. Get the now-safe hmm struct through container_of and directly
> check kref_get_unless_zero to lock it against free.
It is already badly handled with BUG_ON(), i just need to convert
those to return and to use mmu_notifier_call_srcu() to free hmm
struct.
The way race is avoided is because mm->hmm will either be NULL or
point to another hmm struct before an existing hmm is free. Also
if range_start/range_end use kref_get_unless_zero() but right now
this is BUG_ON if it turn out to be NULL, it should just return
on NULL.
>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> ---
> include/linux/hmm.h | 1 +
> mm/hmm.c | 25 +++++++++++++++++++------
> 2 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> index 51ec27a8466816..8b91c90d3b88cb 100644
> --- a/include/linux/hmm.h
> +++ b/include/linux/hmm.h
> @@ -102,6 +102,7 @@ struct hmm {
> struct mmu_notifier mmu_notifier;
> struct rw_semaphore mirrors_sem;
> wait_queue_head_t wq;
> + struct rcu_head rcu;
> long notifiers;
> bool dead;
> };
> diff --git a/mm/hmm.c b/mm/hmm.c
> index 816c2356f2449f..824e7e160d8167 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -113,6 +113,11 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
> return NULL;
> }
>
> +static void hmm_fee_rcu(struct rcu_head *rcu)
> +{
> + kfree(container_of(rcu, struct hmm, rcu));
> +}
> +
> static void hmm_free(struct kref *kref)
> {
> struct hmm *hmm = container_of(kref, struct hmm, kref);
> @@ -125,7 +130,7 @@ static void hmm_free(struct kref *kref)
> mm->hmm = NULL;
> spin_unlock(&mm->page_table_lock);
>
> - kfree(hmm);
> + mmu_notifier_call_srcu(&hmm->rcu, hmm_fee_rcu);
> }
>
> static inline void hmm_put(struct hmm *hmm)
> @@ -153,10 +158,14 @@ void hmm_mm_destroy(struct mm_struct *mm)
>
> static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
> {
> - struct hmm *hmm = mm_get_hmm(mm);
> + struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
> struct hmm_mirror *mirror;
> struct hmm_range *range;
>
> + /* hmm is in progress to free */
> + if (!kref_get_unless_zero(&hmm->kref))
> + return;
> +
> /* Report this HMM as dying. */
> hmm->dead = true;
>
> @@ -194,13 +203,15 @@ static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
> static int hmm_invalidate_range_start(struct mmu_notifier *mn,
> const struct mmu_notifier_range *nrange)
> {
> - struct hmm *hmm = mm_get_hmm(nrange->mm);
> + struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
> struct hmm_mirror *mirror;
> struct hmm_update update;
> struct hmm_range *range;
> int ret = 0;
>
> - VM_BUG_ON(!hmm);
> + /* hmm is in progress to free */
> + if (!kref_get_unless_zero(&hmm->kref))
> + return 0;
>
> update.start = nrange->start;
> update.end = nrange->end;
> @@ -248,9 +259,11 @@ static int hmm_invalidate_range_start(struct mmu_notifier *mn,
> static void hmm_invalidate_range_end(struct mmu_notifier *mn,
> const struct mmu_notifier_range *nrange)
> {
> - struct hmm *hmm = mm_get_hmm(nrange->mm);
> + struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
>
> - VM_BUG_ON(!hmm);
> + /* hmm is in progress to free */
> + if (!kref_get_unless_zero(&hmm->kref))
> + return;
>
> mutex_lock(&hmm->lock);
> hmm->notifiers--;
> --
> 2.21.0
>
^ permalink raw reply
* Re: [PATCH v4 0/1] Use HMM for ODP v4
From: Jerome Glisse @ 2019-05-22 21:49 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: linux-kernel, linux-rdma, Leon Romanovsky, Doug Ledford,
Artemy Kovalyov, Moni Shoua, Mike Marciniszyn, Kaike Wan,
Dennis Dalessandro, linux-mm
In-Reply-To: <20190522192219.GF6054@ziepe.ca>
On Wed, May 22, 2019 at 04:22:19PM -0300, Jason Gunthorpe wrote:
> On Wed, May 22, 2019 at 01:48:52PM -0400, Jerome Glisse wrote:
>
> > > > +long ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp,
> > > > + struct hmm_range *range)
> > > > {
> > > > + struct device *device = umem_odp->umem.context->device->dma_device;
> > > > + struct ib_ucontext_per_mm *per_mm = umem_odp->per_mm;
> > > > struct ib_umem *umem = &umem_odp->umem;
> > > > - struct task_struct *owning_process = NULL;
> > > > - struct mm_struct *owning_mm = umem_odp->umem.owning_mm;
> > > > - struct page **local_page_list = NULL;
> > > > - u64 page_mask, off;
> > > > - int j, k, ret = 0, start_idx, npages = 0, page_shift;
> > > > - unsigned int flags = 0;
> > > > - phys_addr_t p = 0;
> > > > -
> > > > - if (access_mask == 0)
> > > > + struct mm_struct *mm = per_mm->mm;
> > > > + unsigned long idx, npages;
> > > > + long ret;
> > > > +
> > > > + if (mm == NULL)
> > > > + return -ENOENT;
> > > > +
> > > > + /* Only drivers with invalidate support can use this function. */
> > > > + if (!umem->context->invalidate_range)
> > > > return -EINVAL;
> > > >
> > > > - if (user_virt < ib_umem_start(umem) ||
> > > > - user_virt + bcnt > ib_umem_end(umem))
> > > > - return -EFAULT;
> > > > + /* Sanity checks. */
> > > > + if (range->default_flags == 0)
> > > > + return -EINVAL;
> > > >
> > > > - local_page_list = (struct page **)__get_free_page(GFP_KERNEL);
> > > > - if (!local_page_list)
> > > > - return -ENOMEM;
> > > > + if (range->start < ib_umem_start(umem) ||
> > > > + range->end > ib_umem_end(umem))
> > > > + return -EINVAL;
> > > >
> > > > - page_shift = umem->page_shift;
> > > > - page_mask = ~(BIT(page_shift) - 1);
> > > > - off = user_virt & (~page_mask);
> > > > - user_virt = user_virt & page_mask;
> > > > - bcnt += off; /* Charge for the first page offset as well. */
> > > > + idx = (range->start - ib_umem_start(umem)) >> umem->page_shift;
> > >
> > > Is this math OK? What is supposed to happen if the range->start is not
> > > page aligned to the internal page size?
> >
> > range->start is align on 1 << page_shift boundary within pagefault_mr
> > thus the above math is ok. We can add a BUG_ON() and comments if you
> > want.
>
> OK
>
> > > > + range->pfns = &umem_odp->pfns[idx];
> > > > + range->pfn_shift = ODP_FLAGS_BITS;
> > > > + range->values = odp_hmm_values;
> > > > + range->flags = odp_hmm_flags;
> > > >
> > > > /*
> > > > - * owning_process is allowed to be NULL, this means somehow the mm is
> > > > - * existing beyond the lifetime of the originating process.. Presumably
> > > > - * mmget_not_zero will fail in this case.
> > > > + * If mm is dying just bail out early without trying to take mmap_sem.
> > > > + * Note that this might race with mm destruction but that is fine the
> > > > + * is properly refcounted so are all HMM structure.
> > > > */
> > > > - owning_process = get_pid_task(umem_odp->per_mm->tgid, PIDTYPE_PID);
> > > > - if (!owning_process || !mmget_not_zero(owning_mm)) {
> > >
> > > But we are not in a HMM context here, and per_mm is not a HMM
> > > structure.
> > >
> > > So why is mm suddenly guarenteed valid? It was a bug report that
> > > triggered the race the mmget_not_zero is fixing, so I need a better
> > > explanation why it is now safe. From what I see the hmm_range_fault
> > > is doing stuff like find_vma without an active mmget??
> >
> > So the mm struct can not go away as long as we hold a reference on
> > the hmm struct and we hold a reference on it through both hmm_mirror
> > and hmm_range struct. So struct mm can not go away and thus it is
> > safe to try to take its mmap_sem.
>
> This was always true here, though, so long as the umem_odp exists the
> the mm has a grab on it. But a grab is not a get..
>
> The point here was the old code needed an mmget() in order to do
> get_user_pages_remote()
>
> If hmm does not need an external mmget() then fine, we delete this
> stuff and rely on hmm.
>
> But I don't think that is true as we have:
>
> CPU 0 CPU1
> mmput()
> __mmput()
> exit_mmap()
> down_read(&mm->mmap_sem);
> hmm_range_dma_map(range, device,..
> ret = hmm_range_fault(range, block);
> if (hmm->mm == NULL || hmm->dead)
> mmu_notifier_release()
> hmm->dead = true
> vma = find_vma(hmm->mm, start);
> .. rb traversal .. while (vma) remove_vma()
>
> *goes boom*
>
> I think this is violating the basic constraint of the mm by acting on
> a mm's VMA's without holding a mmget() to prevent concurrent
> destruction.
>
> In other words, mmput() destruction does not respect the mmap_sem - so
> holding the mmap sem alone is not enough locking.
>
> The unlucked hmm->dead simply can't save this. Frankly every time I
> look a struct with 'dead' in it, I find races like this.
>
> Thus we should put the mmget_notzero back in.
So for some reason i thought exit_mmap() was setting the mm_rb
to empty node and flushing vmacache so that find_vma() would
fail. Might have been in some patch that never went upstream.
Note that right before find_vma() there is also range->valid
check which will also intercept mm release.
Anyway the easy fix is to get ref on mm user in range_register.
>
> I saw some other funky looking stuff in hmm as well..
>
> > Hence it is safe to take mmap_sem and it is safe to call in hmm, if
> > mm have been kill it will return EFAULT and this will propagate to
> > RDMA.
>
> > As per_mm i removed the per_mm->mm = NULL from release so that it is
> > always safe to use that field even in face of racing mm "killing".
>
> Yes, that certainly wasn't good.
>
> > > > - * An array of the pages included in the on-demand paging umem.
> > > > - * Indices of pages that are currently not mapped into the device will
> > > > - * contain NULL.
> > > > + * An array of the pages included in the on-demand paging umem. Indices
> > > > + * of pages that are currently not mapped into the device will contain
> > > > + * 0.
> > > > */
> > > > - struct page **page_list;
> > > > + uint64_t *pfns;
> > >
> > > Are these actually pfns, or are they mangled with some shift? (what is range->pfn_shift?)
> >
> > They are not pfns they have flags (hence range->pfn_shift) at the
> > bottoms i just do not have a better name for this.
>
> I think you need to have a better name then
Suggestion ? i have no idea for a better name, it has pfn value
in it.
Cheers,
Jérôme
^ permalink raw reply
* Re: [PATCH v15 05/17] arms64: untag user pointers passed to memory syscalls
From: Evgenii Stepanov @ 2019-05-22 21:16 UTC (permalink / raw)
To: Catalin Marinas
Cc: Andrey Konovalov, Linux ARM, Linux Memory Management List, LKML,
amd-gfx, dri-devel, linux-rdma, linux-media, kvm,
open list:KERNEL SELFTEST FRAMEWORK, Vincenzo Frascino,
Will Deacon, Mark Rutland, Andrew Morton, Greg Kroah-Hartman,
Kees Cook, Yishai Hadas, Felix Kuehling, Alexander Deucher
In-Reply-To: <20190522114910.emlckebwzv2qz42i@mbp>
On Wed, May 22, 2019 at 4:49 AM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Mon, May 06, 2019 at 06:30:51PM +0200, Andrey Konovalov wrote:
> > This patch is a part of a series that extends arm64 kernel ABI to allow to
> > pass tagged user pointers (with the top byte set to something else other
> > than 0x00) as syscall arguments.
> >
> > This patch allows tagged pointers to be passed to the following memory
> > syscalls: brk, get_mempolicy, madvise, mbind, mincore, mlock, mlock2,
> > mmap, mmap_pgoff, mprotect, mremap, msync, munlock, munmap,
> > remap_file_pages, shmat and shmdt.
> >
> > This is done by untagging pointers passed to these syscalls in the
> > prologues of their handlers.
>
> I'll go through them one by one to see if we can tighten the expected
> ABI while having the MTE in mind.
>
> > diff --git a/arch/arm64/kernel/sys.c b/arch/arm64/kernel/sys.c
> > index b44065fb1616..933bb9f3d6ec 100644
> > --- a/arch/arm64/kernel/sys.c
> > +++ b/arch/arm64/kernel/sys.c
> > @@ -35,10 +35,33 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len,
> > {
> > if (offset_in_page(off) != 0)
> > return -EINVAL;
> > -
> > + addr = untagged_addr(addr);
> > return ksys_mmap_pgoff(addr, len, prot, flags, fd, off >> PAGE_SHIFT);
> > }
>
> If user passes a tagged pointer to mmap() and the address is honoured
> (or MAP_FIXED is given), what is the expected return pointer? Does it
> need to be tagged with the value from the hint?
For HWASan the most convenient would be to use the tag from the hint.
But since in the TBI (not MTE) mode the kernel has no idea what
meaning userspace assigns to pointer tags, perhaps it should not try
to guess, and should return raw (zero-tagged) address instead.
> With MTE, we may want to use this as a request for the default colour of
> the mapped pages (still under discussion).
I like this - and in that case it would make sense to return the
pointer that can be immediately dereferenced without crashing the
process, i.e. with the matching tag.
> > +SYSCALL_DEFINE6(arm64_mmap_pgoff, unsigned long, addr, unsigned long, len,
> > + unsigned long, prot, unsigned long, flags,
> > + unsigned long, fd, unsigned long, pgoff)
> > +{
> > + addr = untagged_addr(addr);
> > + return ksys_mmap_pgoff(addr, len, prot, flags, fd, pgoff);
> > +}
>
> We don't have __NR_mmap_pgoff on arm64.
>
> > +SYSCALL_DEFINE5(arm64_mremap, unsigned long, addr, unsigned long, old_len,
> > + unsigned long, new_len, unsigned long, flags,
> > + unsigned long, new_addr)
> > +{
> > + addr = untagged_addr(addr);
> > + new_addr = untagged_addr(new_addr);
> > + return ksys_mremap(addr, old_len, new_len, flags, new_addr);
> > +}
>
> Similar comment as for mmap(), do we want the tag from new_addr to be
> preserved? In addition, should we check that the two tags are identical
> or mremap() should become a way to repaint a memory region?
>
> > +SYSCALL_DEFINE2(arm64_munmap, unsigned long, addr, size_t, len)
> > +{
> > + addr = untagged_addr(addr);
> > + return ksys_munmap(addr, len);
> > +}
>
> This looks fine.
>
> > +SYSCALL_DEFINE1(arm64_brk, unsigned long, brk)
> > +{
> > + brk = untagged_addr(brk);
> > + return ksys_brk(brk);
> > +}
>
> I wonder whether brk() should simply not accept tags, and should not
> return them (similar to the prctl(PR_SET_MM) discussion). We could
> document this in the ABI requirements.
>
> > +SYSCALL_DEFINE5(arm64_get_mempolicy, int __user *, policy,
> > + unsigned long __user *, nmask, unsigned long, maxnode,
> > + unsigned long, addr, unsigned long, flags)
> > +{
> > + addr = untagged_addr(addr);
> > + return ksys_get_mempolicy(policy, nmask, maxnode, addr, flags);
> > +}
> > +
> > +SYSCALL_DEFINE3(arm64_madvise, unsigned long, start,
> > + size_t, len_in, int, behavior)
> > +{
> > + start = untagged_addr(start);
> > + return ksys_madvise(start, len_in, behavior);
> > +}
> > +
> > +SYSCALL_DEFINE6(arm64_mbind, unsigned long, start, unsigned long, len,
> > + unsigned long, mode, const unsigned long __user *, nmask,
> > + unsigned long, maxnode, unsigned int, flags)
> > +{
> > + start = untagged_addr(start);
> > + return ksys_mbind(start, len, mode, nmask, maxnode, flags);
> > +}
> > +
> > +SYSCALL_DEFINE2(arm64_mlock, unsigned long, start, size_t, len)
> > +{
> > + start = untagged_addr(start);
> > + return ksys_mlock(start, len, VM_LOCKED);
> > +}
> > +
> > +SYSCALL_DEFINE2(arm64_mlock2, unsigned long, start, size_t, len)
> > +{
> > + start = untagged_addr(start);
> > + return ksys_mlock(start, len, VM_LOCKED);
> > +}
> > +
> > +SYSCALL_DEFINE2(arm64_munlock, unsigned long, start, size_t, len)
> > +{
> > + start = untagged_addr(start);
> > + return ksys_munlock(start, len);
> > +}
> > +
> > +SYSCALL_DEFINE3(arm64_mprotect, unsigned long, start, size_t, len,
> > + unsigned long, prot)
> > +{
> > + start = untagged_addr(start);
> > + return ksys_mprotect_pkey(start, len, prot, -1);
> > +}
> > +
> > +SYSCALL_DEFINE3(arm64_msync, unsigned long, start, size_t, len, int, flags)
> > +{
> > + start = untagged_addr(start);
> > + return ksys_msync(start, len, flags);
> > +}
> > +
> > +SYSCALL_DEFINE3(arm64_mincore, unsigned long, start, size_t, len,
> > + unsigned char __user *, vec)
> > +{
> > + start = untagged_addr(start);
> > + return ksys_mincore(start, len, vec);
> > +}
>
> These look fine.
>
> > +SYSCALL_DEFINE5(arm64_remap_file_pages, unsigned long, start,
> > + unsigned long, size, unsigned long, prot,
> > + unsigned long, pgoff, unsigned long, flags)
> > +{
> > + start = untagged_addr(start);
> > + return ksys_remap_file_pages(start, size, prot, pgoff, flags);
> > +}
>
> While this has been deprecated for some time, I presume user space still
> invokes it?
>
> > +SYSCALL_DEFINE3(arm64_shmat, int, shmid, char __user *, shmaddr, int, shmflg)
> > +{
> > + shmaddr = untagged_addr(shmaddr);
> > + return ksys_shmat(shmid, shmaddr, shmflg);
> > +}
> > +
> > +SYSCALL_DEFINE1(arm64_shmdt, char __user *, shmaddr)
> > +{
> > + shmaddr = untagged_addr(shmaddr);
> > + return ksys_shmdt(shmaddr);
> > +}
>
> Do we actually want to allow shared tagged memory? Who's going to tag
> it? If not, we can document it as not supported.
>
> --
> Catalin
^ permalink raw reply
* Re: [PATCH v4 0/1] Use HMM for ODP v4
From: Ralph Campbell @ 2019-05-22 21:12 UTC (permalink / raw)
To: Jason Gunthorpe, Jerome Glisse, linux-mm
Cc: linux-kernel, linux-rdma, Leon Romanovsky, Doug Ledford,
Artemy Kovalyov, Moni Shoua, Mike Marciniszyn, Kaike Wan,
Dennis Dalessandro
In-Reply-To: <20190522201247.GH6054@ziepe.ca>
On 5/22/19 1:12 PM, Jason Gunthorpe wrote:
> On Wed, May 22, 2019 at 01:48:52PM -0400, Jerome Glisse wrote:
>
>> static void put_per_mm(struct ib_umem_odp *umem_odp)
>> {
>> struct ib_ucontext_per_mm *per_mm = umem_odp->per_mm;
>> @@ -325,9 +283,10 @@ static void put_per_mm(struct ib_umem_odp *umem_odp)
>> up_write(&per_mm->umem_rwsem);
>>
>> WARN_ON(!RB_EMPTY_ROOT(&per_mm->umem_tree.rb_root));
>> - mmu_notifier_unregister_no_release(&per_mm->mn, per_mm->mm);
>> + hmm_mirror_unregister(&per_mm->mirror);
>> put_pid(per_mm->tgid);
>> - mmu_notifier_call_srcu(&per_mm->rcu, free_per_mm);
>> +
>> + kfree(per_mm);
>
> Notice that mmu_notifier only uses SRCU to fence in-progress ops
> callbacks, so I think hmm internally has the bug that this ODP
> approach prevents.
>
> hmm should follow the same pattern ODP has and 'kfree_srcu' the hmm
> struct, use container_of in the mmu_notifier callbacks, and use the
> otherwise vestigal kref_get_unless_zero() to bail:
You might also want to look at my patch where
I try to fix some of these same issues (5/5).
https://marc.info/?l=linux-mm&m=155718572908765&w=2
> From 0cb536dc0150ba964a1d655151d7b7a84d0f915a Mon Sep 17 00:00:00 2001
> From: Jason Gunthorpe <jgg@mellanox.com>
> Date: Wed, 22 May 2019 16:52:52 -0300
> Subject: [PATCH] hmm: Fix use after free with struct hmm in the mmu notifiers
>
> mmu_notifier_unregister_no_release() is not a fence and the mmu_notifier
> system will continue to reference hmm->mn until the srcu grace period
> expires.
>
> CPU0 CPU1
> __mmu_notifier_invalidate_range_start()
> srcu_read_lock
> hlist_for_each ()
> // mn == hmm->mn
> hmm_mirror_unregister()
> hmm_put()
> hmm_free()
> mmu_notifier_unregister_no_release()
> hlist_del_init_rcu(hmm-mn->list)
> mn->ops->invalidate_range_start(mn, range);
> mm_get_hmm()
> mm->hmm = NULL;
> kfree(hmm)
> mutex_lock(&hmm->lock);
>
> Use SRCU to kfree the hmm memory so that the notifiers can rely on hmm
> existing. Get the now-safe hmm struct through container_of and directly
> check kref_get_unless_zero to lock it against free.
>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> ---
> include/linux/hmm.h | 1 +
> mm/hmm.c | 25 +++++++++++++++++++------
> 2 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> index 51ec27a8466816..8b91c90d3b88cb 100644
> --- a/include/linux/hmm.h
> +++ b/include/linux/hmm.h
> @@ -102,6 +102,7 @@ struct hmm {
> struct mmu_notifier mmu_notifier;
> struct rw_semaphore mirrors_sem;
> wait_queue_head_t wq;
> + struct rcu_head rcu;
> long notifiers;
> bool dead;
> };
> diff --git a/mm/hmm.c b/mm/hmm.c
> index 816c2356f2449f..824e7e160d8167 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -113,6 +113,11 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
> return NULL;
> }
>
> +static void hmm_fee_rcu(struct rcu_head *rcu)
> +{
> + kfree(container_of(rcu, struct hmm, rcu));
> +}
> +
> static void hmm_free(struct kref *kref)
> {
> struct hmm *hmm = container_of(kref, struct hmm, kref);
> @@ -125,7 +130,7 @@ static void hmm_free(struct kref *kref)
> mm->hmm = NULL;
> spin_unlock(&mm->page_table_lock);
>
> - kfree(hmm);
> + mmu_notifier_call_srcu(&hmm->rcu, hmm_fee_rcu);
> }
>
> static inline void hmm_put(struct hmm *hmm)
> @@ -153,10 +158,14 @@ void hmm_mm_destroy(struct mm_struct *mm)
>
> static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
> {
> - struct hmm *hmm = mm_get_hmm(mm);
> + struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
> struct hmm_mirror *mirror;
> struct hmm_range *range;
>
> + /* hmm is in progress to free */
> + if (!kref_get_unless_zero(&hmm->kref))
> + return;
> +
> /* Report this HMM as dying. */
> hmm->dead = true;
>
> @@ -194,13 +203,15 @@ static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
> static int hmm_invalidate_range_start(struct mmu_notifier *mn,
> const struct mmu_notifier_range *nrange)
> {
> - struct hmm *hmm = mm_get_hmm(nrange->mm);
> + struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
> struct hmm_mirror *mirror;
> struct hmm_update update;
> struct hmm_range *range;
> int ret = 0;
>
> - VM_BUG_ON(!hmm);
> + /* hmm is in progress to free */
> + if (!kref_get_unless_zero(&hmm->kref))
> + return 0;
>
> update.start = nrange->start;
> update.end = nrange->end;
> @@ -248,9 +259,11 @@ static int hmm_invalidate_range_start(struct mmu_notifier *mn,
> static void hmm_invalidate_range_end(struct mmu_notifier *mn,
> const struct mmu_notifier_range *nrange)
> {
> - struct hmm *hmm = mm_get_hmm(nrange->mm);
> + struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
>
> - VM_BUG_ON(!hmm);
> + /* hmm is in progress to free */
> + if (!kref_get_unless_zero(&hmm->kref))
> + return;
>
> mutex_lock(&hmm->lock);
> hmm->notifiers--;
>
^ permalink raw reply
* Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
From: Kees Cook @ 2019-05-22 20:47 UTC (permalink / raw)
To: Catalin Marinas
Cc: enh, Evgenii Stepanov, Andrey Konovalov, Khalid Aziz, Linux ARM,
Linux Memory Management List, LKML, amd-gfx, dri-devel,
linux-rdma, linux-media, kvm, open list:KERNEL SELFTEST FRAMEWORK,
Vincenzo Frascino, Will Deacon, Mark Rutland, Andrew Morton,
Greg Kroah-Hartman, Yishai Hadas
In-Reply-To: <20190522163527.rnnc6t4tll7tk5zw@mbp>
On Wed, May 22, 2019 at 05:35:27PM +0100, Catalin Marinas wrote:
> The two hard requirements I have for supporting any new hardware feature
> in Linux are (1) a single kernel image binary continues to run on old
> hardware while making use of the new feature if available and (2) old
> user space continues to run on new hardware while new user space can
> take advantage of the new feature.
Agreed! And I think the series meets these requirements, yes?
> For MTE, we just can't enable it by default since there are applications
> who use the top byte of a pointer and expect it to be ignored rather
> than failing with a mismatched tag. Just think of a hwasan compiled
> binary where TBI is expected to work and you try to run it with MTE
> turned on.
Ah! Okay, here's the use-case I wasn't thinking of: the concern is TBI
conflicting with MTE. And anything that starts using TBI suddenly can't
run in the future because it's being interpreted as MTE bits? (Is that
the ABI concern? I feel like we got into the weeds about ioctl()s and
one-off bugs...)
So there needs to be some way to let the kernel know which of three
things it should be doing:
1- leaving userspace addresses as-is (present)
2- wiping the top bits before using (this series)
3- wiping the top bits for most things, but retaining them for MTE as
needed (the future)
I expect MTE to be the "default" in the future. Once a system's libc has
grown support for it, everything will be trying to use MTE. TBI will be
the special case (but TBI is effectively a prerequisite).
AFAICT, the only difference I see between 2 and 3 will be the tag handling
in usercopy (all other places will continue to ignore the top bits). Is
that accurate?
Is "1" a per-process state we want to keep? (I assume not, but rather it
is available via no TBI/MTE CONFIG or a boot-time option, if at all?)
To choose between "2" and "3", it seems we need a per-process flag to
opt into TBI (and out of MTE). For userspace, how would a future binary
choose TBI over MTE? If it's a library issue, we can't use an ELF bit,
since the choice may be "late" after ELF load (this implies the need
for a prctl().) If it's binary-only ("built with HWKASan") then an ELF
bit seems sufficient. And without the marking, I'd expect the kernel to
enforce MTE when there are high bits.
> I would also expect the C library or dynamic loader to check for the
> presence of a HWCAP_MTE bit before starting to tag memory allocations,
> otherwise it would get SIGILL on the first MTE instruction it tries to
> execute.
I've got the same question as Elliot: aren't MTE instructions just NOP
to older CPUs? I.e. if the CPU (or kernel) don't support it, it just
gets entirely ignored: checking is only needed to satisfy curiosity
or behavioral expectations.
To me, the conflict seems to be using TBI in the face of expecting MTE to
be the default state of the future. (But the internal changes needed
for TBI -- this series -- is a prereq for MTE.)
--
Kees Cook
^ permalink raw reply
* Re: [PATCH] RDMA/mlx5: Use DIV_ROUND_UP_ULL macro to allow 32 bit to build
From: Steven Rostedt @ 2019-05-22 20:19 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: LKML, Leon Romanovsky, Doug Ledford, linux-rdma, Linus Torvalds
In-Reply-To: <20190522201412.GI6054@ziepe.ca>
On Wed, 22 May 2019 17:14:12 -0300
Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > As long as it is correct and doesn't break my builds. I really prefer
> > if these kinds of things don't make it into Linus's tree to begin with.
> > I'm surprised the zero-day bot didn't catch this. Because this is
> > something that it normally does.
>
> Yes, I was also surprised and I asked them.. They said they needed to
> update ARM compilers to see this..
Really? This triggered on x86 not ARM for me.
-- Steve
^ permalink raw reply
* Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
From: enh @ 2019-05-22 20:15 UTC (permalink / raw)
To: Kees Cook
Cc: Catalin Marinas, Evgenii Stepanov, Andrey Konovalov, Khalid Aziz,
Linux ARM, Linux Memory Management List, LKML, amd-gfx, dri-devel,
linux-rdma, linux-media, kvm, open list:KERNEL SELFTEST FRAMEWORK,
Vincenzo Frascino, Will Deacon, Mark Rutland, Andrew Morton,
Greg Kroah-Hartman, Yishai Hadas
In-Reply-To: <201905221157.A9BAB1F296@keescook>
On Wed, May 22, 2019 at 12:21 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, May 22, 2019 at 08:30:21AM -0700, enh wrote:
> > On Wed, May 22, 2019 at 3:11 AM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > On Tue, May 21, 2019 at 05:04:39PM -0700, Kees Cook wrote:
> > > > I just want to make sure I fully understand your concern about this
> > > > being an ABI break, and I work best with examples. The closest situation
> > > > I can see would be:
> > > >
> > > > - some program has no idea about MTE
> > >
> > > Apart from some libraries like libc (and maybe those that handle
> > > specific device ioctls), I think most programs should have no idea about
> > > MTE. I wouldn't expect programmers to have to change their app just
> > > because we have a new feature that colours heap allocations.
>
> Right -- things should Just Work from the application perspective.
>
> > obviously i'm biased as a libc maintainer, but...
> >
> > i don't think it helps to move this to libc --- now you just have an
> > extra dependency where to have a guaranteed working system you need to
> > update your kernel and libc together. (or at least update your libc to
> > understand new ioctls etc _before_ you can update your kernel.)
>
> I think (hope?) we've all agreed that we shouldn't pass this off to
> userspace. At the very least, it reduces the utility of MTE, and at worst
> it complicates userspace when this is clearly a kernel/architecture issue.
>
> >
> > > > - malloc() starts returning MTE-tagged addresses
> > > > - program doesn't break from that change
> > > > - program uses some syscall that is missing untagged_addr() and fails
> > > > - kernel has now broken userspace that used to work
> > >
> > > That's one aspect though probably more of a case of plugging in a new
> > > device (graphics card, network etc.) and the ioctl to the new device
> > > doesn't work.
>
> I think MTE will likely be rather like NX/PXN and SMAP/PAN: there will
> be glitches, and we can disable stuff either via CONFIG or (as is more
> common now) via a kernel commandline with untagged_addr() containing a
> static branch, etc. But I actually don't think we need to go this route
> (see below...)
>
> > > The other is that, assuming we reach a point where the kernel entirely
> > > supports this relaxed ABI, can we guarantee that it won't break in the
> > > future. Let's say some subsequent kernel change (some refactoring)
> > > misses out an untagged_addr(). This renders a previously TBI/MTE-capable
> > > syscall unusable. Can we rely only on testing?
> > >
> > > > The trouble I see with this is that it is largely theoretical and
> > > > requires part of userspace to collude to start using a new CPU feature
> > > > that tickles a bug in the kernel. As I understand the golden rule,
> > > > this is a bug in the kernel (a missed ioctl() or such) to be fixed,
> > > > not a global breaking of some userspace behavior.
> > >
> > > Yes, we should follow the rule that it's a kernel bug but it doesn't
> > > help the user that a newly installed kernel causes user space to no
> > > longer reach a prompt. Hence the proposal of an opt-in via personality
> > > (for MTE we would need an explicit opt-in by the user anyway since the
> > > top byte is no longer ignored but checked against the allocation tag).
> >
> > but realistically would this actually get used in this way? or would
> > any given system either be MTE or non-MTE. in which case a kernel
> > configuration option would seem to make more sense. (because either
> > way, the hypothetical user basically needs to recompile the kernel to
> > get back on their feet. or all of userspace.)
>
> Right: the point is to design things so that we do our best to not break
> userspace that is using the new feature (which I think this series has
> done well). But supporting MTE/TBI is just like supporting PAN: if someone
> refactors a driver and swaps a copy_from_user() to a memcpy(), it's going
> to break under PAN. There will be the same long tail of these bugs like
> any other, but my sense is that they are small and rare. But I agree:
> they're going to be pretty weird bugs to track down. The final result,
> however, will be excellent annotation in the kernel for where userspace
> addresses get used and people make assumptions about them.
>
> The sooner we get the series landed and gain QEMU support (or real
> hardware), the faster we can hammer out these missed corner-cases.
> What's the timeline for either of those things, BTW?
>
> > > > I feel like I'm missing something about this being seen as an ABI
> > > > break. The kernel already fails on userspace addresses that have high
> > > > bits set -- are there things that _depend_ on this failure to operate?
> > >
> > > It's about providing a relaxed ABI which allows non-zero top byte and
> > > breaking it later inadvertently without having something better in place
> > > to analyse the kernel changes.
>
> It sounds like the question is how to switch a process in or out of this
> ABI (but I don't think that's the real issue: I think it's just a matter
> of whether or not a process uses tags at all). Doing it at the prctl()
> level doesn't make sense to me, except maybe to detect MTE support or
> something. ("Should I tag allocations?") And that state is controlled
> by the kernel: the kernel does it or it doesn't.
>
> If a process wants to not tag, that's also up to the allocator where
> it can decide not to ask the kernel, and just not tag. Nothing breaks in
> userspace if a process is NOT tagging and untagged_addr() exists or is
> missing. This, I think, is the core way this doesn't trip over the
> golden rule: an old system image will run fine (because it's not
> tagging). A *new* system may encounter bugs with tagging because it's a
> new feature: this is The Way Of Things. But we don't break old userspace
> because old userspace isn't using tags.
>
> So the agreement appears to be between the kernel and the allocator.
> Kernel says "I support this" or not. Telling the allocator to not tag if
> something breaks sounds like an entirely userspace decision, yes?
sgtm, and the AT_FLAGS suggestion sounds fine for our needs in that regard.
> --
> Kees Cook
^ permalink raw reply
* Re: [PATCH] RDMA/mlx5: Use DIV_ROUND_UP_ULL macro to allow 32 bit to build
From: Jason Gunthorpe @ 2019-05-22 20:14 UTC (permalink / raw)
To: Steven Rostedt
Cc: LKML, Leon Romanovsky, Doug Ledford, linux-rdma, Linus Torvalds
In-Reply-To: <20190522154305.615d1d76@gandalf.local.home>
On Wed, May 22, 2019 at 03:43:05PM -0400, Steven Rostedt wrote:
> On Wed, 22 May 2019 16:28:21 -0300
> Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> > On Wed, May 22, 2019 at 02:54:50PM -0400, Steven Rostedt wrote:
> > >
> > > From: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > >
> > > When testing 32 bit x86, my build failed with:
> > >
> > > ERROR: "__udivdi3" [drivers/infiniband/hw/mlx5/mlx5_ib.ko] undefined!
> > >
> > > It appears that a few non-ULL roundup() calls were made, which uses a
> > > normal division against a 64 bit number. This is fine for x86_64, but
> > > on 32 bit x86, it causes the compiler to look for a helper function
> > > __udivdi3, which we do not have in the kernel, and thus fails to build.
> > >
> > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> >
> > Do you like this version better?
> >
> > https://patchwork.kernel.org/patch/10950913/
> >
>
> Honestly, I don't care ;-)
>
> As long as it is correct and doesn't break my builds. I really prefer
> if these kinds of things don't make it into Linus's tree to begin with.
> I'm surprised the zero-day bot didn't catch this. Because this is
> something that it normally does.
Yes, I was also surprised and I asked them.. They said they needed to
update ARM compilers to see this..
Jason
^ permalink raw reply
* Re: [PATCH v4 0/1] Use HMM for ODP v4
From: Jason Gunthorpe @ 2019-05-22 20:12 UTC (permalink / raw)
To: Jerome Glisse, linux-mm
Cc: linux-kernel, linux-rdma, Leon Romanovsky, Doug Ledford,
Artemy Kovalyov, Moni Shoua, Mike Marciniszyn, Kaike Wan,
Dennis Dalessandro
In-Reply-To: <20190522174852.GA23038@redhat.com>
On Wed, May 22, 2019 at 01:48:52PM -0400, Jerome Glisse wrote:
> static void put_per_mm(struct ib_umem_odp *umem_odp)
> {
> struct ib_ucontext_per_mm *per_mm = umem_odp->per_mm;
> @@ -325,9 +283,10 @@ static void put_per_mm(struct ib_umem_odp *umem_odp)
> up_write(&per_mm->umem_rwsem);
>
> WARN_ON(!RB_EMPTY_ROOT(&per_mm->umem_tree.rb_root));
> - mmu_notifier_unregister_no_release(&per_mm->mn, per_mm->mm);
> + hmm_mirror_unregister(&per_mm->mirror);
> put_pid(per_mm->tgid);
> - mmu_notifier_call_srcu(&per_mm->rcu, free_per_mm);
> +
> + kfree(per_mm);
Notice that mmu_notifier only uses SRCU to fence in-progress ops
callbacks, so I think hmm internally has the bug that this ODP
approach prevents.
hmm should follow the same pattern ODP has and 'kfree_srcu' the hmm
struct, use container_of in the mmu_notifier callbacks, and use the
otherwise vestigal kref_get_unless_zero() to bail:
>From 0cb536dc0150ba964a1d655151d7b7a84d0f915a Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <jgg@mellanox.com>
Date: Wed, 22 May 2019 16:52:52 -0300
Subject: [PATCH] hmm: Fix use after free with struct hmm in the mmu notifiers
mmu_notifier_unregister_no_release() is not a fence and the mmu_notifier
system will continue to reference hmm->mn until the srcu grace period
expires.
CPU0 CPU1
__mmu_notifier_invalidate_range_start()
srcu_read_lock
hlist_for_each ()
// mn == hmm->mn
hmm_mirror_unregister()
hmm_put()
hmm_free()
mmu_notifier_unregister_no_release()
hlist_del_init_rcu(hmm-mn->list)
mn->ops->invalidate_range_start(mn, range);
mm_get_hmm()
mm->hmm = NULL;
kfree(hmm)
mutex_lock(&hmm->lock);
Use SRCU to kfree the hmm memory so that the notifiers can rely on hmm
existing. Get the now-safe hmm struct through container_of and directly
check kref_get_unless_zero to lock it against free.
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
---
include/linux/hmm.h | 1 +
mm/hmm.c | 25 +++++++++++++++++++------
2 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 51ec27a8466816..8b91c90d3b88cb 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -102,6 +102,7 @@ struct hmm {
struct mmu_notifier mmu_notifier;
struct rw_semaphore mirrors_sem;
wait_queue_head_t wq;
+ struct rcu_head rcu;
long notifiers;
bool dead;
};
diff --git a/mm/hmm.c b/mm/hmm.c
index 816c2356f2449f..824e7e160d8167 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -113,6 +113,11 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
return NULL;
}
+static void hmm_fee_rcu(struct rcu_head *rcu)
+{
+ kfree(container_of(rcu, struct hmm, rcu));
+}
+
static void hmm_free(struct kref *kref)
{
struct hmm *hmm = container_of(kref, struct hmm, kref);
@@ -125,7 +130,7 @@ static void hmm_free(struct kref *kref)
mm->hmm = NULL;
spin_unlock(&mm->page_table_lock);
- kfree(hmm);
+ mmu_notifier_call_srcu(&hmm->rcu, hmm_fee_rcu);
}
static inline void hmm_put(struct hmm *hmm)
@@ -153,10 +158,14 @@ void hmm_mm_destroy(struct mm_struct *mm)
static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
{
- struct hmm *hmm = mm_get_hmm(mm);
+ struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
struct hmm_mirror *mirror;
struct hmm_range *range;
+ /* hmm is in progress to free */
+ if (!kref_get_unless_zero(&hmm->kref))
+ return;
+
/* Report this HMM as dying. */
hmm->dead = true;
@@ -194,13 +203,15 @@ static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
static int hmm_invalidate_range_start(struct mmu_notifier *mn,
const struct mmu_notifier_range *nrange)
{
- struct hmm *hmm = mm_get_hmm(nrange->mm);
+ struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
struct hmm_mirror *mirror;
struct hmm_update update;
struct hmm_range *range;
int ret = 0;
- VM_BUG_ON(!hmm);
+ /* hmm is in progress to free */
+ if (!kref_get_unless_zero(&hmm->kref))
+ return 0;
update.start = nrange->start;
update.end = nrange->end;
@@ -248,9 +259,11 @@ static int hmm_invalidate_range_start(struct mmu_notifier *mn,
static void hmm_invalidate_range_end(struct mmu_notifier *mn,
const struct mmu_notifier_range *nrange)
{
- struct hmm *hmm = mm_get_hmm(nrange->mm);
+ struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
- VM_BUG_ON(!hmm);
+ /* hmm is in progress to free */
+ if (!kref_get_unless_zero(&hmm->kref))
+ return;
mutex_lock(&hmm->lock);
hmm->notifiers--;
--
2.21.0
^ permalink raw reply related
* Re: [PATCH] RDMA/mlx5: Use DIV_ROUND_UP_ULL macro to allow 32 bit to build
From: Steven Rostedt @ 2019-05-22 19:43 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: LKML, Leon Romanovsky, Doug Ledford, linux-rdma, Linus Torvalds
In-Reply-To: <20190522192821.GG6054@ziepe.ca>
On Wed, 22 May 2019 16:28:21 -0300
Jason Gunthorpe <jgg@ziepe.ca> wrote:
> On Wed, May 22, 2019 at 02:54:50PM -0400, Steven Rostedt wrote:
> >
> > From: Steven Rostedt (VMware) <rostedt@goodmis.org>
> >
> > When testing 32 bit x86, my build failed with:
> >
> > ERROR: "__udivdi3" [drivers/infiniband/hw/mlx5/mlx5_ib.ko] undefined!
> >
> > It appears that a few non-ULL roundup() calls were made, which uses a
> > normal division against a 64 bit number. This is fine for x86_64, but
> > on 32 bit x86, it causes the compiler to look for a helper function
> > __udivdi3, which we do not have in the kernel, and thus fails to build.
> >
> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > ---
>
> Do you like this version better?
>
> https://patchwork.kernel.org/patch/10950913/
>
Honestly, I don't care ;-)
As long as it is correct and doesn't break my builds. I really prefer
if these kinds of things don't make it into Linus's tree to begin with.
I'm surprised the zero-day bot didn't catch this. Because this is
something that it normally does.
-- Steve
^ permalink raw reply
* [PATCH AUTOSEL 4.4 22/92] RDMA/cma: Consider scope_id while binding to ipv6 ll address
From: Sasha Levin @ 2019-05-22 19:30 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Parav Pandit, Daniel Jurgens, Leon Romanovsky, Jason Gunthorpe,
Sasha Levin, linux-rdma
In-Reply-To: <20190522193127.27079-1-sashal@kernel.org>
From: Parav Pandit <parav@mellanox.com>
[ Upstream commit 5d7ed2f27bbd482fd29e6b2e204b1a1ee8a0b268 ]
When two netdev have same link local addresses (such as vlan and non
vlan), two rdma cm listen id should be able to bind to following different
addresses.
listener-1: addr=lla, scope_id=A, port=X
listener-2: addr=lla, scope_id=B, port=X
However while comparing the addresses only addr and port are considered,
due to which 2nd listener fails to listen.
In below example of two listeners, 2nd listener is failing with address in
use error.
$ rping -sv -a fe80::268a:7ff:feb3:d113%ens2f1 -p 4545&
$ rping -sv -a fe80::268a:7ff:feb3:d113%ens2f1.200 -p 4545
rdma_bind_addr: Address already in use
To overcome this, consider the scope_ids as well which forms the accurate
IPv6 link local address.
Signed-off-by: Parav Pandit <parav@mellanox.com>
Reviewed-by: Daniel Jurgens <danielj@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/infiniband/core/cma.c | 25 +++++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 1454290078def..76e7eca35a110 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -902,18 +902,31 @@ static inline int cma_any_addr(struct sockaddr *addr)
return cma_zero_addr(addr) || cma_loopback_addr(addr);
}
-static int cma_addr_cmp(struct sockaddr *src, struct sockaddr *dst)
+static int cma_addr_cmp(const struct sockaddr *src, const struct sockaddr *dst)
{
if (src->sa_family != dst->sa_family)
return -1;
switch (src->sa_family) {
case AF_INET:
- return ((struct sockaddr_in *) src)->sin_addr.s_addr !=
- ((struct sockaddr_in *) dst)->sin_addr.s_addr;
- case AF_INET6:
- return ipv6_addr_cmp(&((struct sockaddr_in6 *) src)->sin6_addr,
- &((struct sockaddr_in6 *) dst)->sin6_addr);
+ return ((struct sockaddr_in *)src)->sin_addr.s_addr !=
+ ((struct sockaddr_in *)dst)->sin_addr.s_addr;
+ case AF_INET6: {
+ struct sockaddr_in6 *src_addr6 = (struct sockaddr_in6 *)src;
+ struct sockaddr_in6 *dst_addr6 = (struct sockaddr_in6 *)dst;
+ bool link_local;
+
+ if (ipv6_addr_cmp(&src_addr6->sin6_addr,
+ &dst_addr6->sin6_addr))
+ return 1;
+ link_local = ipv6_addr_type(&dst_addr6->sin6_addr) &
+ IPV6_ADDR_LINKLOCAL;
+ /* Link local must match their scope_ids */
+ return link_local ? (src_addr6->sin6_scope_id !=
+ dst_addr6->sin6_scope_id) :
+ 0;
+ }
+
default:
return ib_addr_cmp(&((struct sockaddr_ib *) src)->sib_addr,
&((struct sockaddr_ib *) dst)->sib_addr);
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.9 032/114] RDMA/cma: Consider scope_id while binding to ipv6 ll address
From: Sasha Levin @ 2019-05-22 19:28 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Parav Pandit, Daniel Jurgens, Leon Romanovsky, Jason Gunthorpe,
Sasha Levin, linux-rdma
In-Reply-To: <20190522193017.26567-1-sashal@kernel.org>
From: Parav Pandit <parav@mellanox.com>
[ Upstream commit 5d7ed2f27bbd482fd29e6b2e204b1a1ee8a0b268 ]
When two netdev have same link local addresses (such as vlan and non
vlan), two rdma cm listen id should be able to bind to following different
addresses.
listener-1: addr=lla, scope_id=A, port=X
listener-2: addr=lla, scope_id=B, port=X
However while comparing the addresses only addr and port are considered,
due to which 2nd listener fails to listen.
In below example of two listeners, 2nd listener is failing with address in
use error.
$ rping -sv -a fe80::268a:7ff:feb3:d113%ens2f1 -p 4545&
$ rping -sv -a fe80::268a:7ff:feb3:d113%ens2f1.200 -p 4545
rdma_bind_addr: Address already in use
To overcome this, consider the scope_ids as well which forms the accurate
IPv6 link local address.
Signed-off-by: Parav Pandit <parav@mellanox.com>
Reviewed-by: Daniel Jurgens <danielj@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/infiniband/core/cma.c | 25 +++++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 85d4ef319c905..0c7d63e0f0fde 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -1021,18 +1021,31 @@ static inline int cma_any_addr(struct sockaddr *addr)
return cma_zero_addr(addr) || cma_loopback_addr(addr);
}
-static int cma_addr_cmp(struct sockaddr *src, struct sockaddr *dst)
+static int cma_addr_cmp(const struct sockaddr *src, const struct sockaddr *dst)
{
if (src->sa_family != dst->sa_family)
return -1;
switch (src->sa_family) {
case AF_INET:
- return ((struct sockaddr_in *) src)->sin_addr.s_addr !=
- ((struct sockaddr_in *) dst)->sin_addr.s_addr;
- case AF_INET6:
- return ipv6_addr_cmp(&((struct sockaddr_in6 *) src)->sin6_addr,
- &((struct sockaddr_in6 *) dst)->sin6_addr);
+ return ((struct sockaddr_in *)src)->sin_addr.s_addr !=
+ ((struct sockaddr_in *)dst)->sin_addr.s_addr;
+ case AF_INET6: {
+ struct sockaddr_in6 *src_addr6 = (struct sockaddr_in6 *)src;
+ struct sockaddr_in6 *dst_addr6 = (struct sockaddr_in6 *)dst;
+ bool link_local;
+
+ if (ipv6_addr_cmp(&src_addr6->sin6_addr,
+ &dst_addr6->sin6_addr))
+ return 1;
+ link_local = ipv6_addr_type(&dst_addr6->sin6_addr) &
+ IPV6_ADDR_LINKLOCAL;
+ /* Link local must match their scope_ids */
+ return link_local ? (src_addr6->sin6_scope_id !=
+ dst_addr6->sin6_scope_id) :
+ 0;
+ }
+
default:
return ib_addr_cmp(&((struct sockaddr_ib *) src)->sib_addr,
&((struct sockaddr_ib *) dst)->sib_addr);
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] RDMA/mlx5: Use DIV_ROUND_UP_ULL macro to allow 32 bit to build
From: Jason Gunthorpe @ 2019-05-22 19:28 UTC (permalink / raw)
To: Steven Rostedt
Cc: LKML, Leon Romanovsky, Doug Ledford, linux-rdma, Linus Torvalds
In-Reply-To: <20190522145450.25ff483d@gandalf.local.home>
On Wed, May 22, 2019 at 02:54:50PM -0400, Steven Rostedt wrote:
>
> From: Steven Rostedt (VMware) <rostedt@goodmis.org>
>
> When testing 32 bit x86, my build failed with:
>
> ERROR: "__udivdi3" [drivers/infiniband/hw/mlx5/mlx5_ib.ko] undefined!
>
> It appears that a few non-ULL roundup() calls were made, which uses a
> normal division against a 64 bit number. This is fine for x86_64, but
> on 32 bit x86, it causes the compiler to look for a helper function
> __udivdi3, which we do not have in the kernel, and thus fails to build.
>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
Do you like this version better?
https://patchwork.kernel.org/patch/10950913/
Jason
^ permalink raw reply
* [PATCH AUTOSEL 4.14 047/167] RDMA/cma: Consider scope_id while binding to ipv6 ll address
From: Sasha Levin @ 2019-05-22 19:26 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Parav Pandit, Daniel Jurgens, Leon Romanovsky, Jason Gunthorpe,
Sasha Levin, linux-rdma
In-Reply-To: <20190522192842.25858-1-sashal@kernel.org>
From: Parav Pandit <parav@mellanox.com>
[ Upstream commit 5d7ed2f27bbd482fd29e6b2e204b1a1ee8a0b268 ]
When two netdev have same link local addresses (such as vlan and non
vlan), two rdma cm listen id should be able to bind to following different
addresses.
listener-1: addr=lla, scope_id=A, port=X
listener-2: addr=lla, scope_id=B, port=X
However while comparing the addresses only addr and port are considered,
due to which 2nd listener fails to listen.
In below example of two listeners, 2nd listener is failing with address in
use error.
$ rping -sv -a fe80::268a:7ff:feb3:d113%ens2f1 -p 4545&
$ rping -sv -a fe80::268a:7ff:feb3:d113%ens2f1.200 -p 4545
rdma_bind_addr: Address already in use
To overcome this, consider the scope_ids as well which forms the accurate
IPv6 link local address.
Signed-off-by: Parav Pandit <parav@mellanox.com>
Reviewed-by: Daniel Jurgens <danielj@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/infiniband/core/cma.c | 25 +++++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 7c5eca312aa88..620e2ec92bb5a 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -1076,18 +1076,31 @@ static inline int cma_any_addr(struct sockaddr *addr)
return cma_zero_addr(addr) || cma_loopback_addr(addr);
}
-static int cma_addr_cmp(struct sockaddr *src, struct sockaddr *dst)
+static int cma_addr_cmp(const struct sockaddr *src, const struct sockaddr *dst)
{
if (src->sa_family != dst->sa_family)
return -1;
switch (src->sa_family) {
case AF_INET:
- return ((struct sockaddr_in *) src)->sin_addr.s_addr !=
- ((struct sockaddr_in *) dst)->sin_addr.s_addr;
- case AF_INET6:
- return ipv6_addr_cmp(&((struct sockaddr_in6 *) src)->sin6_addr,
- &((struct sockaddr_in6 *) dst)->sin6_addr);
+ return ((struct sockaddr_in *)src)->sin_addr.s_addr !=
+ ((struct sockaddr_in *)dst)->sin_addr.s_addr;
+ case AF_INET6: {
+ struct sockaddr_in6 *src_addr6 = (struct sockaddr_in6 *)src;
+ struct sockaddr_in6 *dst_addr6 = (struct sockaddr_in6 *)dst;
+ bool link_local;
+
+ if (ipv6_addr_cmp(&src_addr6->sin6_addr,
+ &dst_addr6->sin6_addr))
+ return 1;
+ link_local = ipv6_addr_type(&dst_addr6->sin6_addr) &
+ IPV6_ADDR_LINKLOCAL;
+ /* Link local must match their scope_ids */
+ return link_local ? (src_addr6->sin6_scope_id !=
+ dst_addr6->sin6_scope_id) :
+ 0;
+ }
+
default:
return ib_addr_cmp(&((struct sockaddr_ib *) src)->sib_addr,
&((struct sockaddr_ib *) dst)->sib_addr);
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.14 004/167] IB/hfi1: Fix WQ_MEM_RECLAIM warning
From: Sasha Levin @ 2019-05-22 19:25 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Mike Marciniszyn, Michael J . Ruhl, Dennis Dalessandro,
Jason Gunthorpe, Sasha Levin, linux-rdma
In-Reply-To: <20190522192842.25858-1-sashal@kernel.org>
From: Mike Marciniszyn <mike.marciniszyn@intel.com>
[ Upstream commit 4c4b1996b5db688e2dcb8242b0a3bf7b1e845e42 ]
The work_item cancels that occur when a QP is destroyed can elicit the
following trace:
workqueue: WQ_MEM_RECLAIM ipoib_wq:ipoib_cm_tx_reap [ib_ipoib] is flushing !WQ_MEM_RECLAIM hfi0_0:_hfi1_do_send [hfi1]
WARNING: CPU: 7 PID: 1403 at kernel/workqueue.c:2486 check_flush_dependency+0xb1/0x100
Call Trace:
__flush_work.isra.29+0x8c/0x1a0
? __switch_to_asm+0x40/0x70
__cancel_work_timer+0x103/0x190
? schedule+0x32/0x80
iowait_cancel_work+0x15/0x30 [hfi1]
rvt_reset_qp+0x1f8/0x3e0 [rdmavt]
rvt_destroy_qp+0x65/0x1f0 [rdmavt]
? _cond_resched+0x15/0x30
ib_destroy_qp+0xe9/0x230 [ib_core]
ipoib_cm_tx_reap+0x21c/0x560 [ib_ipoib]
process_one_work+0x171/0x370
worker_thread+0x49/0x3f0
kthread+0xf8/0x130
? max_active_store+0x80/0x80
? kthread_bind+0x10/0x10
ret_from_fork+0x35/0x40
Since QP destruction frees memory, hfi1_wq should have the WQ_MEM_RECLAIM.
The hfi1_wq does not allocate memory with GFP_KERNEL or otherwise become
entangled with memory reclaim, so this flag is appropriate.
Fixes: 0a226edd203f ("staging/rdma/hfi1: Use parallel workqueue for SDMA engines")
Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
Signed-off-by: Mike Marciniszyn <mike.marciniszyn@intel.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/infiniband/hw/hfi1/init.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/infiniband/hw/hfi1/init.c b/drivers/infiniband/hw/hfi1/init.c
index b7481701542e9..27e7de4c4a34b 100644
--- a/drivers/infiniband/hw/hfi1/init.c
+++ b/drivers/infiniband/hw/hfi1/init.c
@@ -769,7 +769,8 @@ static int create_workqueues(struct hfi1_devdata *dd)
ppd->hfi1_wq =
alloc_workqueue(
"hfi%d_%d",
- WQ_SYSFS | WQ_HIGHPRI | WQ_CPU_INTENSIVE,
+ WQ_SYSFS | WQ_HIGHPRI | WQ_CPU_INTENSIVE |
+ WQ_MEM_RECLAIM,
HFI1_MAX_ACTIVE_WORKQUEUE_ENTRIES,
dd->unit, pidx);
if (!ppd->hfi1_wq)
--
2.20.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox