* [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
* [PATCH AUTOSEL 4.19 071/244] RDMA/cma: Consider scope_id while binding to ipv6 ll address
From: Sasha Levin @ 2019-05-22 19:23 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Parav Pandit, Daniel Jurgens, Leon Romanovsky, Jason Gunthorpe,
Sasha Levin, linux-rdma
In-Reply-To: <20190522192630.24917-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 6f5be78024762..39dc7be56884a 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -1078,18 +1078,31 @@ static inline bool cma_any_addr(const 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.19 004/244] IB/hfi1: Fix WQ_MEM_RECLAIM warning
From: Sasha Levin @ 2019-05-22 19:22 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Mike Marciniszyn, Michael J . Ruhl, Dennis Dalessandro,
Jason Gunthorpe, Sasha Levin, linux-rdma
In-Reply-To: <20190522192630.24917-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 da786eb18558d..368f4f08b6866 100644
--- a/drivers/infiniband/hw/hfi1/init.c
+++ b/drivers/infiniband/hw/hfi1/init.c
@@ -798,7 +798,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
* Re: [PATCH v4 0/1] Use HMM for ODP v4
From: Jason Gunthorpe @ 2019-05-22 19:22 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: <20190522174852.GA23038@redhat.com>
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.
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
Jason
^ permalink raw reply
* Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
From: Kees Cook @ 2019-05-22 19:21 UTC (permalink / raw)
To: enh
Cc: Mark Rutland, kvm, Szabolcs Nagy, Catalin Marinas, Will Deacon,
dri-devel, Linux Memory Management List, Khalid Aziz,
open list:KERNEL SELFTEST FRAMEWORK, Vincenzo Frascino,
Jacob Bramley, Leon Romanovsky, linux-rdma, amd-gfx,
Dmitry Vyukov, Dave Martin, Evgenii Stepanov, linux-media,
Kevin Brodsky, Ruben Ayrapetyan, Andrey Konovalov,
Ramana Radhakrishnan, Alex
In-Reply-To: <CAJgzZoosKBwqXRyA6fb8QQSZXFqfHqe9qO9je5TogHhzuoGXJQ@mail.gmail.com>
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?
--
Kees Cook
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* [PATCH AUTOSEL 5.0 088/317] RDMA/cma: Consider scope_id while binding to ipv6 ll address
From: Sasha Levin @ 2019-05-22 19:19 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Parav Pandit, Daniel Jurgens, Leon Romanovsky, Jason Gunthorpe,
Sasha Levin, linux-rdma
In-Reply-To: <20190522192338.23715-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 81bded0d37d1e..cb482f3389504 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -1170,18 +1170,31 @@ static inline bool cma_any_addr(const 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 5.0 006/317] IB/hfi1: Fix WQ_MEM_RECLAIM warning
From: Sasha Levin @ 2019-05-22 19:18 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Mike Marciniszyn, Michael J . Ruhl, Dennis Dalessandro,
Jason Gunthorpe, Sasha Levin, linux-rdma
In-Reply-To: <20190522192338.23715-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 c532ceb0bb9af..b66c4fe8151a1 100644
--- a/drivers/infiniband/hw/hfi1/init.c
+++ b/drivers/infiniband/hw/hfi1/init.c
@@ -797,7 +797,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
* [PATCH AUTOSEL 5.1 040/375] net/mlx5: E-Switch, Use atomic rep state to serialize state change
From: Sasha Levin @ 2019-05-22 19:15 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Bodong Wang, Parav Pandit, Vu Pham, Saeed Mahameed, Sasha Levin,
netdev, linux-rdma
In-Reply-To: <20190522192115.22666-1-sashal@kernel.org>
From: Bodong Wang <bodong@mellanox.com>
[ Upstream commit 6f4e02193c9a9ea54dd3151cf97489fa787cd0e6 ]
When the state of rep was introduced, it was also designed to prevent
duplicate unloading of the same rep. Considering the following two
flows when an eswitch manager is at switchdev mode with n VF reps loaded.
+--------------------------------------+--------------------------------+
| cpu-0 | cpu-1 |
| -------- | -------- |
| mlx5_ib_remove | mlx5_eswitch_disable_sriov |
| mlx5_ib_unregister_vport_reps | esw_offloads_cleanup |
| mlx5_eswitch_unregister_vport_reps | esw_offloads_unload_all_reps |
| __unload_reps_all_vport | __unload_reps_all_vport |
+--------------------------------------+--------------------------------+
These two flows will try to unload the same rep. Per original design,
once one flow unloads the rep, the state moves to REGISTERED. The 2nd
flow will no longer needs to do the unload and bails out. However, as
read and write of the state is not atomic, when 1st flow is doing the
unload, the state is still LOADED, 2nd flow is able to do the same
unload action. Kernel crash will happen.
To solve this, driver should do atomic test-and-set for the state. So
that only one flow can change the rep state from LOADED to REGISTERED,
and proceed to do the actual unloading.
Since the state is changing to atomic type, all other read/write should
be atomic action as well.
Fixes: f121e0ea9586 (net/mlx5: E-Switch, Add state to eswitch vport representors)
Signed-off-by: Bodong Wang <bodong@mellanox.com>
Reviewed-by: Parav Pandit <parav@mellanox.com>
Reviewed-by: Vu Pham <vuhuong@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
.../mellanox/mlx5/core/eswitch_offloads.c | 36 +++++++++----------
include/linux/mlx5/eswitch.h | 2 +-
2 files changed, 18 insertions(+), 20 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index 9b2d78ee22b88..d2d8da133082c 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -363,7 +363,7 @@ static int esw_set_global_vlan_pop(struct mlx5_eswitch *esw, u8 val)
esw_debug(esw->dev, "%s applying global %s policy\n", __func__, val ? "pop" : "none");
for (vf_vport = 1; vf_vport < esw->enabled_vports; vf_vport++) {
rep = &esw->offloads.vport_reps[vf_vport];
- if (rep->rep_if[REP_ETH].state != REP_LOADED)
+ if (atomic_read(&rep->rep_if[REP_ETH].state) != REP_LOADED)
continue;
err = __mlx5_eswitch_set_vport_vlan(esw, rep->vport, 0, 0, val);
@@ -1306,7 +1306,8 @@ int esw_offloads_init_reps(struct mlx5_eswitch *esw)
ether_addr_copy(rep->hw_id, hw_id);
for (rep_type = 0; rep_type < NUM_REP_TYPES; rep_type++)
- rep->rep_if[rep_type].state = REP_UNREGISTERED;
+ atomic_set(&rep->rep_if[rep_type].state,
+ REP_UNREGISTERED);
}
return 0;
@@ -1315,11 +1316,9 @@ int esw_offloads_init_reps(struct mlx5_eswitch *esw)
static void __esw_offloads_unload_rep(struct mlx5_eswitch *esw,
struct mlx5_eswitch_rep *rep, u8 rep_type)
{
- if (rep->rep_if[rep_type].state != REP_LOADED)
- return;
-
- rep->rep_if[rep_type].unload(rep);
- rep->rep_if[rep_type].state = REP_REGISTERED;
+ if (atomic_cmpxchg(&rep->rep_if[rep_type].state,
+ REP_LOADED, REP_REGISTERED) == REP_LOADED)
+ rep->rep_if[rep_type].unload(rep);
}
static void __unload_reps_special_vport(struct mlx5_eswitch *esw, u8 rep_type)
@@ -1380,16 +1379,15 @@ static int __esw_offloads_load_rep(struct mlx5_eswitch *esw,
{
int err = 0;
- if (rep->rep_if[rep_type].state != REP_REGISTERED)
- return 0;
-
- err = rep->rep_if[rep_type].load(esw->dev, rep);
- if (err)
- return err;
-
- rep->rep_if[rep_type].state = REP_LOADED;
+ if (atomic_cmpxchg(&rep->rep_if[rep_type].state,
+ REP_REGISTERED, REP_LOADED) == REP_REGISTERED) {
+ err = rep->rep_if[rep_type].load(esw->dev, rep);
+ if (err)
+ atomic_set(&rep->rep_if[rep_type].state,
+ REP_REGISTERED);
+ }
- return 0;
+ return err;
}
static int __load_reps_special_vport(struct mlx5_eswitch *esw, u8 rep_type)
@@ -2076,7 +2074,7 @@ void mlx5_eswitch_register_vport_reps(struct mlx5_eswitch *esw,
rep_if->get_proto_dev = __rep_if->get_proto_dev;
rep_if->priv = __rep_if->priv;
- rep_if->state = REP_REGISTERED;
+ atomic_set(&rep_if->state, REP_REGISTERED);
}
}
EXPORT_SYMBOL(mlx5_eswitch_register_vport_reps);
@@ -2091,7 +2089,7 @@ void mlx5_eswitch_unregister_vport_reps(struct mlx5_eswitch *esw, u8 rep_type)
__unload_reps_all_vport(esw, max_vf, rep_type);
mlx5_esw_for_all_reps(esw, i, rep)
- rep->rep_if[rep_type].state = REP_UNREGISTERED;
+ atomic_set(&rep->rep_if[rep_type].state, REP_UNREGISTERED);
}
EXPORT_SYMBOL(mlx5_eswitch_unregister_vport_reps);
@@ -2111,7 +2109,7 @@ void *mlx5_eswitch_get_proto_dev(struct mlx5_eswitch *esw,
rep = mlx5_eswitch_get_rep(esw, vport);
- if (rep->rep_if[rep_type].state == REP_LOADED &&
+ if (atomic_read(&rep->rep_if[rep_type].state) == REP_LOADED &&
rep->rep_if[rep_type].get_proto_dev)
return rep->rep_if[rep_type].get_proto_dev(rep);
return NULL;
diff --git a/include/linux/mlx5/eswitch.h b/include/linux/mlx5/eswitch.h
index 96d8435421de8..0ca77dd1429c0 100644
--- a/include/linux/mlx5/eswitch.h
+++ b/include/linux/mlx5/eswitch.h
@@ -35,7 +35,7 @@ struct mlx5_eswitch_rep_if {
void (*unload)(struct mlx5_eswitch_rep *rep);
void *(*get_proto_dev)(struct mlx5_eswitch_rep *rep);
void *priv;
- u8 state;
+ atomic_t state;
};
struct mlx5_eswitch_rep {
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 5.1 007/375] IB/hfi1: Fix WQ_MEM_RECLAIM warning
From: Sasha Levin @ 2019-05-22 19:15 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Mike Marciniszyn, Michael J . Ruhl, Dennis Dalessandro,
Jason Gunthorpe, Sasha Levin, linux-rdma
In-Reply-To: <20190522192115.22666-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 faaaac8fbc553..3af5eb10a5ffb 100644
--- a/drivers/infiniband/hw/hfi1/init.c
+++ b/drivers/infiniband/hw/hfi1/init.c
@@ -805,7 +805,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
* [PATCH] RDMA/mlx5: Use DIV_ROUND_UP_ULL macro to allow 32 bit to build
From: Steven Rostedt @ 2019-05-22 18:54 UTC (permalink / raw)
To: LKML
Cc: Leon Romanovsky, Doug Ledford, Jason Gunthorpe, linux-rdma,
Linus Torvalds
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/cmd.c b/drivers/infiniband/hw/mlx5/cmd.c
index e3ec79b8f7f5..f080df9934e8 100644
--- a/drivers/infiniband/hw/mlx5/cmd.c
+++ b/drivers/infiniband/hw/mlx5/cmd.c
@@ -190,7 +190,7 @@ int mlx5_cmd_alloc_sw_icm(struct mlx5_dm *dm, int type, u64 length,
u16 uid, phys_addr_t *addr, u32 *obj_id)
{
struct mlx5_core_dev *dev = dm->dev;
- u32 num_blocks = DIV_ROUND_UP(length, MLX5_SW_ICM_BLOCK_SIZE(dev));
+ u32 num_blocks = DIV_ROUND_UP_ULL(length, MLX5_SW_ICM_BLOCK_SIZE(dev));
u32 out[MLX5_ST_SZ_DW(general_obj_out_cmd_hdr)] = {};
u32 in[MLX5_ST_SZ_DW(create_sw_icm_in)] = {};
unsigned long *block_map;
@@ -266,7 +266,7 @@ int mlx5_cmd_dealloc_sw_icm(struct mlx5_dm *dm, int type, u64 length,
u16 uid, phys_addr_t addr, u32 obj_id)
{
struct mlx5_core_dev *dev = dm->dev;
- u32 num_blocks = DIV_ROUND_UP(length, MLX5_SW_ICM_BLOCK_SIZE(dev));
+ u32 num_blocks = DIV_ROUND_UP_ULL(length, MLX5_SW_ICM_BLOCK_SIZE(dev));
u32 out[MLX5_ST_SZ_DW(general_obj_out_cmd_hdr)] = {};
u32 in[MLX5_ST_SZ_DW(general_obj_in_cmd_hdr)] = {};
unsigned long *block_map;
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;
^ permalink raw reply related
* Re: [PATCH v4 0/1] Use HMM for ODP v4
From: Jason Gunthorpe @ 2019-05-22 18:32 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:
> From 0b429b2ffbec348e283693cb97d7ffce760d89da Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Glisse?= <jglisse@redhat.com>
> Date: Sat, 8 Dec 2018 15:47:55 -0500
> Subject: [PATCH] RDMA/odp: convert to use HMM for ODP v5
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Convert ODP to use HMM so that we can build on common infrastructure
> for different class of devices that want to mirror a process address
> space into a device. There is no functional changes.
>
> Changes since v4:
> - Rebase on top of rdma-next
> Changes since v3:
> - Rebase on top of 5.2-rc1
> Changes since v2:
> - Update to match changes to HMM API
> Changes since v1:
> - improved comments
> - simplified page alignment computation
>
> Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
> Cc: Jason Gunthorpe <jgg@mellanox.com>
> Cc: Leon Romanovsky <leonro@mellanox.com>
> Cc: Doug Ledford <dledford@redhat.com>
> Cc: Artemy Kovalyov <artemyko@mellanox.com>
> Cc: Moni Shoua <monis@mellanox.com>
> Cc: Mike Marciniszyn <mike.marciniszyn@intel.com>
> Cc: Kaike Wan <kaike.wan@intel.com>
> Cc: Dennis Dalessandro <dennis.dalessandro@intel.com>
> drivers/infiniband/core/umem_odp.c | 506 +++++++++--------------------
> drivers/infiniband/hw/mlx5/mem.c | 20 +-
> drivers/infiniband/hw/mlx5/mr.c | 2 +-
> drivers/infiniband/hw/mlx5/odp.c | 104 +++---
> include/rdma/ib_umem_odp.h | 47 +--
> 5 files changed, 233 insertions(+), 446 deletions(-)
The kconfig stuff is missing, and it doesn't compile in various cases
I tried.
The kconfig stuff for hmm is also really obnoxious, you can't just
enabe HMM_MIRROR, you have to track down all the little required
elements to get it to turn on..
Once I did get it to compile, I also get warnings:
mm/hmm.c: In function ‘hmm_vma_walk_pud’:
mm/hmm.c:782:28: warning: unused variable ‘pfn’ [-Wunused-variable]
unsigned long i, npages, pfn;
^~~
mm/hmm.c: In function ‘hmm_range_snapshot’:
mm/hmm.c:1027:19: warning: unused variable ‘h’ [-Wunused-variable]
struct hstate *h = hstate_vma(vma);
Because this kernel doesn't have CONFIG_HUGETLB_PAGE
Please fold this into your patch if it has to be resent.. I think it
fixes the compilation problems.
diff --git a/drivers/infiniband/Kconfig b/drivers/infiniband/Kconfig
index cbfbea49f126cd..e3eefd0917985a 100644
--- a/drivers/infiniband/Kconfig
+++ b/drivers/infiniband/Kconfig
@@ -63,7 +63,7 @@ config INFINIBAND_USER_MEM
config INFINIBAND_ON_DEMAND_PAGING
bool "InfiniBand on-demand paging support"
depends on INFINIBAND_USER_MEM
- select MMU_NOTIFIER
+ depends on HMM_MIRROR
default y
---help---
On demand paging support for the InfiniBand subsystem.
diff --git a/include/rdma/ib_umem_odp.h b/include/rdma/ib_umem_odp.h
index e1476e9ebb7906..f760103c07349a 100644
--- a/include/rdma/ib_umem_odp.h
+++ b/include/rdma/ib_umem_odp.h
@@ -115,6 +115,16 @@ static inline size_t ib_umem_odp_num_pages(struct ib_umem_odp *umem_odp)
#define ODP_DMA_ADDR_MASK (~(ODP_READ_ALLOWED_BIT | ODP_WRITE_ALLOWED_BIT))
+#define ODP_READ_BIT (1<<0ULL)
+#define ODP_WRITE_BIT (1<<1ULL)
+/*
+ * The device bit is not use by ODP but is there to full-fill HMM API which
+ * also support device with device memory (like GPU). So from ODP/RDMA POV
+ * this can be ignored.
+ */
+#define ODP_DEVICE_BIT (1<<2ULL)
+#define ODP_FLAGS_BITS 3
+
#ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING
struct ib_ucontext_per_mm {
@@ -138,16 +148,6 @@ struct ib_umem_odp *ib_alloc_odp_umem(struct ib_umem_odp *root_umem,
unsigned long addr, size_t size);
void ib_umem_odp_release(struct ib_umem_odp *umem_odp);
-#define ODP_READ_BIT (1<<0ULL)
-#define ODP_WRITE_BIT (1<<1ULL)
-/*
- * The device bit is not use by ODP but is there to full-fill HMM API which
- * also support device with device memory (like GPU). So from ODP/RDMA POV
- * this can be ignored.
- */
-#define ODP_DEVICE_BIT (1<<2ULL)
-#define ODP_FLAGS_BITS 3
-
long ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp,
struct hmm_range *range);
^ permalink raw reply related
* Re: [PATCH v4 0/1] Use HMM for ODP v4
From: Jerome Glisse @ 2019-05-22 17:48 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
In-Reply-To: <20190522005225.GA30819@ziepe.ca>
[-- Attachment #1: Type: text/plain, Size: 9793 bytes --]
On Tue, May 21, 2019 at 09:52:25PM -0300, Jason Gunthorpe wrote:
> On Tue, May 21, 2019 at 04:53:22PM -0400, Jerome Glisse wrote:
> > On Mon, May 06, 2019 at 04:56:57PM -0300, Jason Gunthorpe wrote:
> > > On Thu, Apr 11, 2019 at 02:13:13PM -0400, jglisse@redhat.com wrote:
> > > > From: Jérôme Glisse <jglisse@redhat.com>
> > > >
> > > > Just fixed Kconfig and build when ODP was not enabled, other than that
> > > > this is the same as v3. Here is previous cover letter:
> > > >
> > > > Git tree with all prerequisite:
> > > > https://cgit.freedesktop.org/~glisse/linux/log/?h=rdma-odp-hmm-v4
> > > >
> > > > This patchset convert RDMA ODP to use HMM underneath this is motivated
> > > > by stronger code sharing for same feature (share virtual memory SVM or
> > > > Share Virtual Address SVA) and also stronger integration with mm code to
> > > > achieve that. It depends on HMM patchset posted for inclusion in 5.2 [2]
> > > > and [3].
> > > >
> > > > It has been tested with pingpong test with -o and others flags to test
> > > > different size/features associated with ODP.
> > > >
> > > > Moreover they are some features of HMM in the works like peer to peer
> > > > support, fast CPU page table snapshot, fast IOMMU mapping update ...
> > > > It will be easier for RDMA devices with ODP to leverage those if they
> > > > use HMM underneath.
> > > >
> > > > Quick summary of what HMM is:
> > > > HMM is a toolbox for device driver to implement software support for
> > > > Share Virtual Memory (SVM). Not only it provides helpers to mirror a
> > > > process address space on a device (hmm_mirror). It also provides
> > > > helper to allow to use device memory to back regular valid virtual
> > > > address of a process (any valid mmap that is not an mmap of a device
> > > > or a DAX mapping). They are two kinds of device memory. Private memory
> > > > that is not accessible to CPU because it does not have all the expected
> > > > properties (this is for all PCIE devices) or public memory which can
> > > > also be access by CPU without restriction (with OpenCAPI or CCIX or
> > > > similar cache-coherent and atomic inter-connect).
> > > >
> > > > Device driver can use each of HMM tools separatly. You do not have to
> > > > use all the tools it provides.
> > > >
> > > > For RDMA device i do not expect a need to use the device memory support
> > > > of HMM. This device memory support is geared toward accelerator like GPU.
> > > >
> > > >
> > > > You can find a branch [1] with all the prerequisite in. This patch is on
> > > > top of rdma-next with the HMM patchset [2] and mmu notifier patchset [3]
> > > > applied on top of it.
> > > >
> > > > [1] https://cgit.freedesktop.org/~glisse/linux/log/?h=rdma-odp-hmm-v4
> > > > [2] https://lkml.org/lkml/2019/4/3/1032
> > > > [3] https://lkml.org/lkml/2019/3/26/900
> > >
> > > Jerome, please let me know if these dependent series are merged during
> > > the first week of the merge window.
> > >
> > > This patch has been tested and could go along next week if the
> > > dependencies are met.
> > >
> >
> > 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
>
> > +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.
>
> > + 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.
Now if we race with a destruction (ie all process that referenced the
mm struct are dead/dying) hmm struct will be marked as dead this is
tested by hmm_mirror_mm_is_alive() and also by hmm_range_snapshot()
(which is call by hmm_range_dma_map()). In other word if mm->mm_users
is zero then hmm will be mark as dead.
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".
>
> > @@ -603,11 +603,29 @@ static int pagefault_mr(struct mlx5_ib_dev *dev, struct mlx5_ib_mr *mr,
> >
> > next_mr:
> > size = min_t(size_t, bcnt, ib_umem_end(&odp->umem) - io_virt);
> > -
> > page_shift = mr->umem->page_shift;
> > page_mask = ~(BIT(page_shift) - 1);
> > + /*
> > + * We need to align io_virt on page size so off is the extra bytes we
> > + * will be faulting and fault_size is the page aligned size we are
> > + * faulting.
> > + */
> > + io_virt = io_virt & page_mask;
> > + off = (io_virt & (~page_mask));
> > + fault_size = ALIGN(size + off, 1UL << page_shift);
> > +
> > + if (io_virt < ib_umem_start(&odp->umem))
> > + return -EINVAL;
> > +
> > start_idx = (io_virt - (mr->mmkey.iova & page_mask)) >> page_shift;
> > - access_mask = ODP_READ_ALLOWED_BIT;
> > +
> > + if (odp_mr->per_mm == NULL || odp_mr->per_mm->mm == NULL)
> > + return -ENOENT;
>
> How can this happen? Where is the locking?
>
> per_mm is supposed to outlive any odp_mr's the refer to it, and the mm
> is supposed to remain grab'd as long as the per_mm exists..
This can not happen removed the test.
>
> > diff --git a/include/rdma/ib_umem_odp.h b/include/rdma/ib_umem_odp.h
> > index eeec4e53c448..70b2df8e5a6c 100644
> > +++ b/include/rdma/ib_umem_odp.h
> > @@ -36,6 +36,7 @@
> > #include <rdma/ib_umem.h>
> > #include <rdma/ib_verbs.h>
> > #include <linux/interval_tree.h>
> > +#include <linux/hmm.h>
> >
> > struct umem_odp_node {
> > u64 __subtree_last;
> > @@ -47,11 +48,11 @@ struct ib_umem_odp {
> > struct ib_ucontext_per_mm *per_mm;
> >
> > /*
> > - * 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.
Cheers,
Jérôme
[-- Attachment #2: 0001-RDMA-odp-convert-to-use-HMM-for-ODP-v5.patch --]
[-- Type: text/plain, Size: 34058 bytes --]
>From 0b429b2ffbec348e283693cb97d7ffce760d89da Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Glisse?= <jglisse@redhat.com>
Date: Sat, 8 Dec 2018 15:47:55 -0500
Subject: [PATCH] RDMA/odp: convert to use HMM for ODP v5
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Convert ODP to use HMM so that we can build on common infrastructure
for different class of devices that want to mirror a process address
space into a device. There is no functional changes.
Changes since v4:
- Rebase on top of rdma-next
Changes since v3:
- Rebase on top of 5.2-rc1
Changes since v2:
- Update to match changes to HMM API
Changes since v1:
- improved comments
- simplified page alignment computation
Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
Cc: Jason Gunthorpe <jgg@mellanox.com>
Cc: Leon Romanovsky <leonro@mellanox.com>
Cc: Doug Ledford <dledford@redhat.com>
Cc: Artemy Kovalyov <artemyko@mellanox.com>
Cc: Moni Shoua <monis@mellanox.com>
Cc: Mike Marciniszyn <mike.marciniszyn@intel.com>
Cc: Kaike Wan <kaike.wan@intel.com>
Cc: Dennis Dalessandro <dennis.dalessandro@intel.com>
---
drivers/infiniband/core/umem_odp.c | 506 +++++++++--------------------
drivers/infiniband/hw/mlx5/mem.c | 20 +-
drivers/infiniband/hw/mlx5/mr.c | 2 +-
drivers/infiniband/hw/mlx5/odp.c | 104 +++---
include/rdma/ib_umem_odp.h | 47 +--
5 files changed, 233 insertions(+), 446 deletions(-)
diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c
index c3b3c523401f..7aca59ac8551 100644
--- a/drivers/infiniband/core/umem_odp.c
+++ b/drivers/infiniband/core/umem_odp.c
@@ -46,6 +46,20 @@
#include <rdma/ib_umem.h>
#include <rdma/ib_umem_odp.h>
+
+static uint64_t odp_hmm_flags[HMM_PFN_FLAG_MAX] = {
+ ODP_READ_BIT, /* HMM_PFN_VALID */
+ ODP_WRITE_BIT, /* HMM_PFN_WRITE */
+ ODP_DEVICE_BIT, /* HMM_PFN_DEVICE_PRIVATE */
+};
+
+static uint64_t odp_hmm_values[HMM_PFN_VALUE_MAX] = {
+ -1UL, /* HMM_PFN_ERROR */
+ 0UL, /* HMM_PFN_NONE */
+ -2UL, /* HMM_PFN_SPECIAL */
+};
+
+
/*
* The ib_umem list keeps track of memory regions for which the HW
* device request to receive notification when the related memory
@@ -78,55 +92,24 @@ static u64 node_last(struct umem_odp_node *n)
INTERVAL_TREE_DEFINE(struct umem_odp_node, rb, u64, __subtree_last,
node_start, node_last, static, rbt_ib_umem)
-static void ib_umem_notifier_start_account(struct ib_umem_odp *umem_odp)
-{
- mutex_lock(&umem_odp->umem_mutex);
- if (umem_odp->notifiers_count++ == 0)
- /*
- * Initialize the completion object for waiting on
- * notifiers. Since notifier_count is zero, no one should be
- * waiting right now.
- */
- reinit_completion(&umem_odp->notifier_completion);
- mutex_unlock(&umem_odp->umem_mutex);
-}
-
-static void ib_umem_notifier_end_account(struct ib_umem_odp *umem_odp)
-{
- mutex_lock(&umem_odp->umem_mutex);
- /*
- * This sequence increase will notify the QP page fault that the page
- * that is going to be mapped in the spte could have been freed.
- */
- ++umem_odp->notifiers_seq;
- if (--umem_odp->notifiers_count == 0)
- complete_all(&umem_odp->notifier_completion);
- mutex_unlock(&umem_odp->umem_mutex);
-}
-
static int ib_umem_notifier_release_trampoline(struct ib_umem_odp *umem_odp,
u64 start, u64 end, void *cookie)
{
- /*
- * Increase the number of notifiers running, to
- * prevent any further fault handling on this MR.
- */
- ib_umem_notifier_start_account(umem_odp);
umem_odp->dying = 1;
/* Make sure that the fact the umem is dying is out before we release
* all pending page faults. */
smp_wmb();
- complete_all(&umem_odp->notifier_completion);
- umem_odp->umem.context->invalidate_range(
- umem_odp, ib_umem_start(umem_odp), ib_umem_end(umem_odp));
+ umem_odp->umem.context->invalidate_range(umem_odp,
+ ib_umem_start(umem_odp),
+ ib_umem_end(umem_odp));
return 0;
}
-static void ib_umem_notifier_release(struct mmu_notifier *mn,
- struct mm_struct *mm)
+static void ib_umem_notifier_release(struct hmm_mirror *mirror)
{
- struct ib_ucontext_per_mm *per_mm =
- container_of(mn, struct ib_ucontext_per_mm, mn);
+ struct ib_ucontext_per_mm *per_mm;
+
+ per_mm = container_of(mirror, struct ib_ucontext_per_mm, mirror);
down_read(&per_mm->umem_rwsem);
if (per_mm->active)
@@ -136,21 +119,22 @@ static void ib_umem_notifier_release(struct mmu_notifier *mn,
up_read(&per_mm->umem_rwsem);
}
-static int invalidate_range_start_trampoline(struct ib_umem_odp *item,
- u64 start, u64 end, void *cookie)
+static int invalidate_range_trampoline(struct ib_umem_odp *item,
+ u64 start, u64 end, void *cookie)
{
- ib_umem_notifier_start_account(item);
item->umem.context->invalidate_range(item, start, end);
return 0;
}
-static int ib_umem_notifier_invalidate_range_start(struct mmu_notifier *mn,
- const struct mmu_notifier_range *range)
+static int ib_sync_cpu_device_pagetables(struct hmm_mirror *mirror,
+ const struct hmm_update *range)
{
- struct ib_ucontext_per_mm *per_mm =
- container_of(mn, struct ib_ucontext_per_mm, mn);
+ struct ib_ucontext_per_mm *per_mm;
+ int ret;
+
+ per_mm = container_of(mirror, struct ib_ucontext_per_mm, mirror);
- if (mmu_notifier_range_blockable(range))
+ if (range->blockable)
down_read(&per_mm->umem_rwsem);
else if (!down_read_trylock(&per_mm->umem_rwsem))
return -EAGAIN;
@@ -165,39 +149,17 @@ static int ib_umem_notifier_invalidate_range_start(struct mmu_notifier *mn,
return 0;
}
- return rbt_ib_umem_for_each_in_range(&per_mm->umem_tree, range->start,
+ ret = rbt_ib_umem_for_each_in_range(&per_mm->umem_tree, range->start,
range->end,
- invalidate_range_start_trampoline,
- mmu_notifier_range_blockable(range),
- NULL);
-}
-
-static int invalidate_range_end_trampoline(struct ib_umem_odp *item, u64 start,
- u64 end, void *cookie)
-{
- ib_umem_notifier_end_account(item);
- return 0;
-}
-
-static void ib_umem_notifier_invalidate_range_end(struct mmu_notifier *mn,
- const struct mmu_notifier_range *range)
-{
- struct ib_ucontext_per_mm *per_mm =
- container_of(mn, struct ib_ucontext_per_mm, mn);
-
- if (unlikely(!per_mm->active))
- return;
-
- rbt_ib_umem_for_each_in_range(&per_mm->umem_tree, range->start,
- range->end,
- invalidate_range_end_trampoline, true, NULL);
+ invalidate_range_trampoline,
+ range->blockable, NULL);
up_read(&per_mm->umem_rwsem);
+ return ret;
}
-static const struct mmu_notifier_ops ib_umem_notifiers = {
+static const struct hmm_mirror_ops ib_umem_notifiers = {
.release = ib_umem_notifier_release,
- .invalidate_range_start = ib_umem_notifier_invalidate_range_start,
- .invalidate_range_end = ib_umem_notifier_invalidate_range_end,
+ .sync_cpu_device_pagetables = ib_sync_cpu_device_pagetables,
};
static void add_umem_to_per_mm(struct ib_umem_odp *umem_odp)
@@ -219,7 +181,6 @@ static void remove_umem_from_per_mm(struct ib_umem_odp *umem_odp)
if (likely(ib_umem_start(umem_odp) != ib_umem_end(umem_odp)))
rbt_ib_umem_remove(&umem_odp->interval_tree,
&per_mm->umem_tree);
- complete_all(&umem_odp->notifier_completion);
up_write(&per_mm->umem_rwsem);
}
@@ -246,11 +207,13 @@ static struct ib_ucontext_per_mm *alloc_per_mm(struct ib_ucontext *ctx,
WARN_ON(mm != current->mm);
- per_mm->mn.ops = &ib_umem_notifiers;
- ret = mmu_notifier_register(&per_mm->mn, per_mm->mm);
+ per_mm->mirror.ops = &ib_umem_notifiers;
+ down_write(&mm->mmap_sem);
+ ret = hmm_mirror_register(&per_mm->mirror, per_mm->mm);
+ up_write(&mm->mmap_sem);
if (ret) {
dev_err(&ctx->device->dev,
- "Failed to register mmu_notifier %d\n", ret);
+ "Failed to register HMM mirror %d\n", ret);
goto out_pid;
}
@@ -292,11 +255,6 @@ static int get_per_mm(struct ib_umem_odp *umem_odp)
return 0;
}
-static void free_per_mm(struct rcu_head *rcu)
-{
- kfree(container_of(rcu, struct ib_ucontext_per_mm, rcu));
-}
-
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);
}
struct ib_umem_odp *ib_alloc_odp_umem(struct ib_umem_odp *root,
@@ -355,11 +314,9 @@ struct ib_umem_odp *ib_alloc_odp_umem(struct ib_umem_odp *root,
mmgrab(umem->owning_mm);
mutex_init(&odp_data->umem_mutex);
- init_completion(&odp_data->notifier_completion);
- odp_data->page_list =
- vzalloc(array_size(pages, sizeof(*odp_data->page_list)));
- if (!odp_data->page_list) {
+ odp_data->pfns = vzalloc(array_size(pages, sizeof(*odp_data->pfns)));
+ if (!odp_data->pfns) {
ret = -ENOMEM;
goto out_odp_data;
}
@@ -368,7 +325,7 @@ struct ib_umem_odp *ib_alloc_odp_umem(struct ib_umem_odp *root,
vzalloc(array_size(pages, sizeof(*odp_data->dma_list)));
if (!odp_data->dma_list) {
ret = -ENOMEM;
- goto out_page_list;
+ goto out_pfns;
}
/*
@@ -382,8 +339,8 @@ struct ib_umem_odp *ib_alloc_odp_umem(struct ib_umem_odp *root,
return odp_data;
-out_page_list:
- vfree(odp_data->page_list);
+out_pfns:
+ vfree(odp_data->pfns);
out_odp_data:
mmdrop(umem->owning_mm);
kfree(odp_data);
@@ -419,13 +376,11 @@ int ib_umem_odp_get(struct ib_umem_odp *umem_odp, int access)
mutex_init(&umem_odp->umem_mutex);
- init_completion(&umem_odp->notifier_completion);
-
if (ib_umem_odp_num_pages(umem_odp)) {
- umem_odp->page_list =
- vzalloc(array_size(sizeof(*umem_odp->page_list),
+ umem_odp->pfns =
+ vzalloc(array_size(sizeof(*umem_odp->pfns),
ib_umem_odp_num_pages(umem_odp)));
- if (!umem_odp->page_list)
+ if (!umem_odp->pfns)
return -ENOMEM;
umem_odp->dma_list =
@@ -433,7 +388,7 @@ int ib_umem_odp_get(struct ib_umem_odp *umem_odp, int access)
ib_umem_odp_num_pages(umem_odp)));
if (!umem_odp->dma_list) {
ret_val = -ENOMEM;
- goto out_page_list;
+ goto out_pfns;
}
}
@@ -446,8 +401,8 @@ int ib_umem_odp_get(struct ib_umem_odp *umem_odp, int access)
out_dma_list:
vfree(umem_odp->dma_list);
-out_page_list:
- vfree(umem_odp->page_list);
+out_pfns:
+ vfree(umem_odp->pfns);
return ret_val;
}
@@ -465,287 +420,126 @@ void ib_umem_odp_release(struct ib_umem_odp *umem_odp)
remove_umem_from_per_mm(umem_odp);
put_per_mm(umem_odp);
vfree(umem_odp->dma_list);
- vfree(umem_odp->page_list);
-}
-
-/*
- * Map for DMA and insert a single page into the on-demand paging page tables.
- *
- * @umem: the umem to insert the page to.
- * @page_index: index in the umem to add the page to.
- * @page: the page struct to map and add.
- * @access_mask: access permissions needed for this page.
- * @current_seq: sequence number for synchronization with invalidations.
- * the sequence number is taken from
- * umem_odp->notifiers_seq.
- *
- * The function returns -EFAULT if the DMA mapping operation fails. It returns
- * -EAGAIN if a concurrent invalidation prevents us from updating the page.
- *
- * The page is released via put_page even if the operation failed. For
- * on-demand pinning, the page is released whenever it isn't stored in the
- * umem.
- */
-static int ib_umem_odp_map_dma_single_page(
- struct ib_umem_odp *umem_odp,
- int page_index,
- struct page *page,
- u64 access_mask,
- unsigned long current_seq)
-{
- struct ib_ucontext *context = umem_odp->umem.context;
- struct ib_device *dev = context->device;
- dma_addr_t dma_addr;
- int remove_existing_mapping = 0;
- int ret = 0;
-
- /*
- * Note: we avoid writing if seq is different from the initial seq, to
- * handle case of a racing notifier. This check also allows us to bail
- * early if we have a notifier running in parallel with us.
- */
- if (ib_umem_mmu_notifier_retry(umem_odp, current_seq)) {
- ret = -EAGAIN;
- goto out;
- }
- if (!(umem_odp->dma_list[page_index])) {
- dma_addr =
- ib_dma_map_page(dev, page, 0, BIT(umem_odp->page_shift),
- DMA_BIDIRECTIONAL);
- if (ib_dma_mapping_error(dev, dma_addr)) {
- ret = -EFAULT;
- goto out;
- }
- umem_odp->dma_list[page_index] = dma_addr | access_mask;
- umem_odp->page_list[page_index] = page;
- umem_odp->npages++;
- } else if (umem_odp->page_list[page_index] == page) {
- umem_odp->dma_list[page_index] |= access_mask;
- } else {
- pr_err("error: got different pages in IB device and from get_user_pages. IB device page: %p, gup page: %p\n",
- umem_odp->page_list[page_index], page);
- /* Better remove the mapping now, to prevent any further
- * damage. */
- remove_existing_mapping = 1;
- }
-
-out:
- put_page(page);
-
- if (remove_existing_mapping) {
- ib_umem_notifier_start_account(umem_odp);
- context->invalidate_range(
- umem_odp,
- ib_umem_start(umem_odp) +
- (page_index << umem_odp->page_shift),
- ib_umem_start(umem_odp) +
- ((page_index + 1) << umem_odp->page_shift));
- ib_umem_notifier_end_account(umem_odp);
- ret = -EAGAIN;
- }
-
- return ret;
+ vfree(umem_odp->pfns);
}
/**
* ib_umem_odp_map_dma_pages - Pin and DMA map userspace memory in an ODP MR.
- *
- * Pins the range of pages passed in the argument, and maps them to
- * DMA addresses. The DMA addresses of the mapped pages is updated in
- * umem_odp->dma_list.
- *
- * Returns the number of pages mapped in success, negative error code
- * for failure.
- * An -EAGAIN error code is returned when a concurrent mmu notifier prevents
- * the function from completing its task.
- * An -ENOENT error code indicates that userspace process is being terminated
- * and mm was already destroyed.
* @umem_odp: the umem to map and pin
- * @user_virt: the address from which we need to map.
- * @bcnt: the minimal number of bytes to pin and map. The mapping might be
- * bigger due to alignment, and may also be smaller in case of an error
- * pinning or mapping a page. The actual pages mapped is returned in
- * the return value.
- * @access_mask: bit mask of the requested access permissions for the given
- * range.
- * @current_seq: the MMU notifiers sequance value for synchronization with
- * invalidations. the sequance number is read from
- * umem_odp->notifiers_seq before calling this function
+ * @range: range of virtual address to be mapped to the device
+ * Returns: -EINVAL some invalid arguments, -EAGAIN need to try again, -ENOENT
+ * if process is being terminated, number of pages mapped otherwise.
+ *
+ * Map to device a range of virtual address passed in the argument. The DMA
+ * addresses are in umem_odp->dma_list and the corresponding page informations
+ * in umem_odp->pfns.
*/
-int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt,
- u64 bcnt, u64 access_mask,
- unsigned long current_seq)
+long ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp,
+ struct hmm_range *range)
{
- 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;
- unsigned int flags = 0, page_shift;
- phys_addr_t p = 0;
-
- if (access_mask == 0)
+ struct device *device = umem_odp->umem.context->device->dma_device;
+ struct ib_ucontext_per_mm *per_mm = umem_odp->per_mm;
+ 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_odp->umem.context->invalidate_range)
return -EINVAL;
- if (user_virt < ib_umem_start(umem_odp) ||
- user_virt + bcnt > ib_umem_end(umem_odp))
- 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_odp) ||
+ range->end > ib_umem_end(umem_odp))
+ return -EINVAL;
- page_shift = umem_odp->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_odp)) >> umem_odp->page_shift;
+ 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.
+ *
+ * This means that per_mm->mm is always a valid pointer as hmm struct
+ * hold a reference on mm struct and the hmm_mirror struct hold a
+ * reference on the hmm struct as do the hmm_range struct. Thus at this
+ * point the mm struct can not be freed from under us and it is safe
+ * to dereference it and try to take the mmap_sem.
+ *
+ * If all process owning the mm struct are dead or dying then HMM will
+ * catch that and it is safe to call within HMM which will return with
+ * -EFAULT.
*/
- owning_process = get_pid_task(umem_odp->per_mm->tgid, PIDTYPE_PID);
- if (!owning_process || !mmget_not_zero(owning_mm)) {
- ret = -EINVAL;
- goto out_put_task;
- }
-
- if (access_mask & ODP_WRITE_ALLOWED_BIT)
- flags |= FOLL_WRITE;
-
- start_idx = (user_virt - ib_umem_start(umem_odp)) >> page_shift;
- k = start_idx;
-
- while (bcnt > 0) {
- const size_t gup_num_pages = min_t(size_t,
- (bcnt + BIT(page_shift) - 1) >> page_shift,
- PAGE_SIZE / sizeof(struct page *));
-
- down_read(&owning_mm->mmap_sem);
- /*
- * Note: this might result in redundent page getting. We can
- * avoid this by checking dma_list to be 0 before calling
- * get_user_pages. However, this make the code much more
- * complex (and doesn't gain us much performance in most use
- * cases).
- */
- npages = get_user_pages_remote(owning_process, owning_mm,
- user_virt, gup_num_pages,
- flags, local_page_list, NULL, NULL);
- up_read(&owning_mm->mmap_sem);
-
- if (npages < 0) {
- if (npages != -EAGAIN)
- pr_warn("fail to get %zu user pages with error %d\n", gup_num_pages, npages);
- else
- pr_debug("fail to get %zu user pages with error %d\n", gup_num_pages, npages);
- break;
- }
+ if (!hmm_mirror_mm_is_alive(&per_mm->mirror))
+ return -EINVAL;
+ down_read(&mm->mmap_sem);
+ mutex_lock(&umem_odp->umem_mutex);
+ ret = hmm_range_dma_map(range, device,
+ &umem_odp->dma_list[idx], true);
+ mutex_unlock(&umem_odp->umem_mutex);
+ npages = ret;
- bcnt -= min_t(size_t, npages << PAGE_SHIFT, bcnt);
- mutex_lock(&umem_odp->umem_mutex);
- for (j = 0; j < npages; j++, user_virt += PAGE_SIZE) {
- if (user_virt & ~page_mask) {
- p += PAGE_SIZE;
- if (page_to_phys(local_page_list[j]) != p) {
- ret = -EFAULT;
- break;
- }
- put_page(local_page_list[j]);
- continue;
- }
-
- ret = ib_umem_odp_map_dma_single_page(
- umem_odp, k, local_page_list[j],
- access_mask, current_seq);
- if (ret < 0) {
- if (ret != -EAGAIN)
- pr_warn("ib_umem_odp_map_dma_single_page failed with error %d\n", ret);
- else
- pr_debug("ib_umem_odp_map_dma_single_page failed with error %d\n", ret);
- break;
- }
-
- p = page_to_phys(local_page_list[j]);
- k++;
- }
- mutex_unlock(&umem_odp->umem_mutex);
-
- if (ret < 0) {
- /*
- * Release pages, remembering that the first page
- * to hit an error was already released by
- * ib_umem_odp_map_dma_single_page().
- */
- if (npages - (j + 1) > 0)
- release_pages(&local_page_list[j+1],
- npages - (j + 1));
- break;
- }
- }
+ /*
+ * The mmap_sem have been drop if hmm_vma_fault_and_dma_map() returned
+ * with -EAGAIN. In which case we need to retry as -EBUSY but we also
+ * need to take the mmap_sem again.
+ */
+ if (ret != -EAGAIN)
+ up_read(&mm->mmap_sem);
- if (ret >= 0) {
- if (npages < 0 && k == start_idx)
- ret = npages;
- else
- ret = k - start_idx;
+ if (ret <= 0) {
+ /* Convert -EBUSY to -EAGAIN and 0 to -EAGAIN */
+ ret = ret == -EBUSY ? -EAGAIN : ret;
+ return ret ? ret : -EAGAIN;
}
- mmput(owning_mm);
-out_put_task:
- if (owning_process)
- put_task_struct(owning_process);
- free_page((unsigned long)local_page_list);
- return ret;
+ umem_odp->npages += npages;
+ return npages;
}
EXPORT_SYMBOL(ib_umem_odp_map_dma_pages);
-void ib_umem_odp_unmap_dma_pages(struct ib_umem_odp *umem_odp, u64 virt,
- u64 bound)
+void ib_umem_odp_unmap_dma_pages(struct ib_umem_odp *umem_odp,
+ u64 virt, u64 bound)
{
- int idx;
- u64 addr;
- struct ib_device *dev = umem_odp->umem.context->device;
+ struct device *device = umem_odp->umem.context->device->dma_device;
+ unsigned long idx, page_mask;
+ struct hmm_range range;
+ long ret;
+
+ if (!umem_odp->npages)
+ return;
- virt = max_t(u64, virt, ib_umem_start(umem_odp));
+ bound = ALIGN(bound, 1UL << umem_odp->page_shift);
+ page_mask = ~(BIT(umem_odp->page_shift) - 1);
+ virt &= page_mask;
+
+ virt = max_t(u64, virt, ib_umem_start(umem_odp));
bound = min_t(u64, bound, ib_umem_end(umem_odp));
- /* Note that during the run of this function, the
- * notifiers_count of the MR is > 0, preventing any racing
- * faults from completion. We might be racing with other
- * invalidations, so we must make sure we free each page only
- * once. */
+
+ idx = ((unsigned long)virt - ib_umem_start(umem_odp)) >> PAGE_SHIFT;
+
+ range.page_shift = umem_odp->page_shift;
+ range.pfns = &umem_odp->pfns[idx];
+ range.pfn_shift = ODP_FLAGS_BITS;
+ range.values = odp_hmm_values;
+ range.flags = odp_hmm_flags;
+ range.start = virt;
+ range.end = bound;
+
mutex_lock(&umem_odp->umem_mutex);
- for (addr = virt; addr < bound; addr += BIT(umem_odp->page_shift)) {
- idx = (addr - ib_umem_start(umem_odp)) >> umem_odp->page_shift;
- if (umem_odp->page_list[idx]) {
- struct page *page = umem_odp->page_list[idx];
- dma_addr_t dma = umem_odp->dma_list[idx];
- dma_addr_t dma_addr = dma & ODP_DMA_ADDR_MASK;
-
- WARN_ON(!dma_addr);
-
- ib_dma_unmap_page(dev, dma_addr, PAGE_SIZE,
- DMA_BIDIRECTIONAL);
- if (dma & ODP_WRITE_ALLOWED_BIT) {
- struct page *head_page = compound_head(page);
- /*
- * set_page_dirty prefers being called with
- * the page lock. However, MMU notifiers are
- * called sometimes with and sometimes without
- * the lock. We rely on the umem_mutex instead
- * to prevent other mmu notifiers from
- * continuing and allowing the page mapping to
- * be removed.
- */
- set_page_dirty(head_page);
- }
- umem_odp->page_list[idx] = NULL;
- umem_odp->dma_list[idx] = 0;
- umem_odp->npages--;
- }
- }
+ ret = hmm_range_dma_unmap(&range, NULL, device,
+ &umem_odp->dma_list[idx], true);
+ if (ret > 0)
+ umem_odp->npages -= ret;
mutex_unlock(&umem_odp->umem_mutex);
}
EXPORT_SYMBOL(ib_umem_odp_unmap_dma_pages);
diff --git a/drivers/infiniband/hw/mlx5/mem.c b/drivers/infiniband/hw/mlx5/mem.c
index fe1a76d8531c..f2d92f0eb3f4 100644
--- a/drivers/infiniband/hw/mlx5/mem.c
+++ b/drivers/infiniband/hw/mlx5/mem.c
@@ -112,16 +112,16 @@ void mlx5_ib_cont_pages(struct ib_umem *umem, u64 addr,
*count = i;
}
-static u64 umem_dma_to_mtt(dma_addr_t umem_dma)
+static u64 umem_dma_to_mtt(struct ib_umem_odp *odp, size_t idx)
{
- u64 mtt_entry = umem_dma & ODP_DMA_ADDR_MASK;
+ u64 mtt_entry = odp->dma_list[idx];
- if (umem_dma & ODP_READ_ALLOWED_BIT)
+ if (odp->pfns[idx] & ODP_READ_BIT)
mtt_entry |= MLX5_IB_MTT_READ;
- if (umem_dma & ODP_WRITE_ALLOWED_BIT)
+ if (odp->pfns[idx] & ODP_WRITE_BIT)
mtt_entry |= MLX5_IB_MTT_WRITE;
- return mtt_entry;
+ return cpu_to_be64(mtt_entry);
}
/*
@@ -151,15 +151,13 @@ void __mlx5_ib_populate_pas(struct mlx5_ib_dev *dev, struct ib_umem *umem,
int entry;
if (umem->is_odp) {
+ struct ib_umem_odp *odp = to_ib_umem_odp(umem);
+
WARN_ON(shift != 0);
WARN_ON(access_flags != (MLX5_IB_MTT_READ | MLX5_IB_MTT_WRITE));
- for (i = 0; i < num_pages; ++i) {
- dma_addr_t pa =
- to_ib_umem_odp(umem)->dma_list[offset + i];
-
- pas[i] = cpu_to_be64(umem_dma_to_mtt(pa));
- }
+ for (i = 0; i < num_pages; ++i)
+ pas[i] = umem_dma_to_mtt(odp, offset + i);
return;
}
diff --git a/drivers/infiniband/hw/mlx5/mr.c b/drivers/infiniband/hw/mlx5/mr.c
index 4d033796dcfc..a8e7ac685ce1 100644
--- a/drivers/infiniband/hw/mlx5/mr.c
+++ b/drivers/infiniband/hw/mlx5/mr.c
@@ -1605,7 +1605,7 @@ static void dereg_mr(struct mlx5_ib_dev *dev, struct mlx5_ib_mr *mr)
/* Wait for all running page-fault handlers to finish. */
synchronize_srcu(&dev->mr_srcu);
/* Destroy all page mappings */
- if (umem_odp->page_list)
+ if (umem_odp->pfns)
mlx5_ib_invalidate_range(umem_odp,
ib_umem_start(umem_odp),
ib_umem_end(umem_odp));
diff --git a/drivers/infiniband/hw/mlx5/odp.c b/drivers/infiniband/hw/mlx5/odp.c
index d0c6f9cc97ef..45438620c0fa 100644
--- a/drivers/infiniband/hw/mlx5/odp.c
+++ b/drivers/infiniband/hw/mlx5/odp.c
@@ -255,8 +255,7 @@ void mlx5_ib_invalidate_range(struct ib_umem_odp *umem_odp, unsigned long start,
* estimate the cost of another UMR vs. the cost of bigger
* UMR.
*/
- if (umem_odp->dma_list[idx] &
- (ODP_READ_ALLOWED_BIT | ODP_WRITE_ALLOWED_BIT)) {
+ if (umem_odp->pfns[idx] & ODP_READ_BIT) {
if (!in_block) {
blk_start_idx = idx;
in_block = 1;
@@ -577,17 +576,18 @@ static int pagefault_mr(struct mlx5_ib_dev *dev, struct mlx5_ib_mr *mr,
u64 io_virt, size_t bcnt, u32 *bytes_mapped,
u32 flags)
{
- int npages = 0, current_seq, page_shift, ret, np;
- bool implicit = false;
struct ib_umem_odp *odp_mr = to_ib_umem_odp(mr->umem);
bool downgrade = flags & MLX5_PF_FLAGS_DOWNGRADE;
bool prefetch = flags & MLX5_PF_FLAGS_PREFETCH;
- u64 access_mask;
+ unsigned long npages = 0, page_shift, np, off;
u64 start_idx, page_mask;
struct ib_umem_odp *odp;
- size_t size;
+ struct hmm_range range;
+ bool implicit = false;
+ size_t size, fault_size;
+ long ret;
- if (!odp_mr->page_list) {
+ if (!odp_mr->pfns) {
odp = implicit_mr_get_data(mr, io_virt, bcnt);
if (IS_ERR(odp))
@@ -600,11 +600,26 @@ static int pagefault_mr(struct mlx5_ib_dev *dev, struct mlx5_ib_mr *mr,
next_mr:
size = min_t(size_t, bcnt, ib_umem_end(odp) - io_virt);
-
page_shift = odp->page_shift;
page_mask = ~(BIT(page_shift) - 1);
+ /*
+ * We need to align io_virt on page size so off is the extra bytes we
+ * will be faulting and fault_size is the page aligned size we are
+ * faulting.
+ */
+ io_virt = io_virt & page_mask;
+ off = (io_virt & (~page_mask));
+ fault_size = ALIGN(size + off, 1UL << page_shift);
+
+ if (io_virt < ib_umem_start(odp))
+ return -EINVAL;
+
start_idx = (io_virt - (mr->mmkey.iova & page_mask)) >> page_shift;
- access_mask = ODP_READ_ALLOWED_BIT;
+
+ ret = hmm_range_register(&range, odp_mr->per_mm->mm,
+ io_virt, io_virt + fault_size, page_shift);
+ if (ret)
+ return ret;
if (prefetch && !downgrade && !mr->umem->writable) {
/* prefetch with write-access must
@@ -614,58 +629,55 @@ static int pagefault_mr(struct mlx5_ib_dev *dev, struct mlx5_ib_mr *mr,
goto out;
}
+ range.default_flags = ODP_READ_BIT;
if (mr->umem->writable && !downgrade)
- access_mask |= ODP_WRITE_ALLOWED_BIT;
-
- current_seq = READ_ONCE(odp->notifiers_seq);
- /*
- * Ensure the sequence number is valid for some time before we call
- * gup.
- */
- smp_rmb();
-
- ret = ib_umem_odp_map_dma_pages(to_ib_umem_odp(mr->umem), io_virt, size,
- access_mask, current_seq);
+ range.default_flags |= ODP_WRITE_BIT;
+ ret = ib_umem_odp_map_dma_pages(to_ib_umem_odp(mr->umem), &range);
if (ret < 0)
- goto out;
+ goto again;
np = ret;
mutex_lock(&odp->umem_mutex);
- if (!ib_umem_mmu_notifier_retry(to_ib_umem_odp(mr->umem),
- current_seq)) {
+ if (hmm_range_valid(&range)) {
/*
* No need to check whether the MTTs really belong to
- * this MR, since ib_umem_odp_map_dma_pages already
+ * this MR, since ib_umem_odp_map_dma_pages() already
* checks this.
*/
ret = mlx5_ib_update_xlt(mr, start_idx, np,
page_shift, MLX5_IB_UPD_XLT_ATOMIC);
- } else {
+ } else
ret = -EAGAIN;
- }
mutex_unlock(&odp->umem_mutex);
if (ret < 0) {
- if (ret != -EAGAIN)
+ if (ret != -EAGAIN) {
mlx5_ib_err(dev, "Failed to update mkey page tables\n");
- goto out;
+ goto out;
+ }
+ goto again;
}
if (bytes_mapped) {
- u32 new_mappings = (np << page_shift) -
- (io_virt - round_down(io_virt, 1 << page_shift));
+ long new_mappings = (np << page_shift) - off;
+ new_mappings = new_mappings < 0 ? 0 : new_mappings;
*bytes_mapped += min_t(u32, new_mappings, size);
}
npages += np << (page_shift - PAGE_SHIFT);
+ hmm_range_unregister(&range);
bcnt -= size;
- if (unlikely(bcnt)) {
+ if (unlikely(bcnt > 0)) {
struct ib_umem_odp *next;
- io_virt += size;
+ /*
+ * Next virtual address is after the number of bytes we faulted
+ * in this step.
+ */
+ io_virt += fault_size;
next = odp_next(odp);
if (unlikely(!next || next->umem.address != io_virt)) {
mlx5_ib_dbg(dev, "next implicit leaf removed at 0x%llx. got %p\n",
@@ -679,24 +691,18 @@ static int pagefault_mr(struct mlx5_ib_dev *dev, struct mlx5_ib_mr *mr,
return npages;
-out:
- if (ret == -EAGAIN) {
- if (implicit || !odp->dying) {
- unsigned long timeout =
- msecs_to_jiffies(MMU_NOTIFIER_TIMEOUT);
-
- if (!wait_for_completion_timeout(
- &odp->notifier_completion,
- timeout)) {
- mlx5_ib_warn(dev, "timeout waiting for mmu notifier. seq %d against %d. notifiers_count=%d\n",
- current_seq, odp->notifiers_seq, odp->notifiers_count);
- }
- } else {
- /* The MR is being killed, kill the QP as well. */
- ret = -EFAULT;
- }
- }
+again:
+ if (ret != -EAGAIN)
+ goto out;
+
+ /* Check if the MR is being killed, kill the QP as well. */
+ if (!implicit || odp->dying)
+ ret = -EFAULT;
+ else if (!hmm_range_wait_until_valid(&range, MMU_NOTIFIER_TIMEOUT))
+ mlx5_ib_warn(dev, "timeout waiting for mmu notifier.\n");
+out:
+ hmm_range_unregister(&range);
return ret;
}
diff --git a/include/rdma/ib_umem_odp.h b/include/rdma/ib_umem_odp.h
index 479db5c98ff6..e1476e9ebb79 100644
--- a/include/rdma/ib_umem_odp.h
+++ b/include/rdma/ib_umem_odp.h
@@ -36,6 +36,7 @@
#include <rdma/ib_umem.h>
#include <rdma/ib_verbs.h>
#include <linux/interval_tree.h>
+#include <linux/hmm.h>
struct umem_odp_node {
u64 __subtree_last;
@@ -47,11 +48,11 @@ struct ib_umem_odp {
struct ib_ucontext_per_mm *per_mm;
/*
- * 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;
/*
* An array of the same size as page_list, with DMA addresses mapped
* for pages the pages in page_list. The lower two bits designate
@@ -67,14 +68,11 @@ struct ib_umem_odp {
struct mutex umem_mutex;
void *private; /* for the HW driver to use. */
- int notifiers_seq;
- int notifiers_count;
int npages;
/* Tree tracking */
struct umem_odp_node interval_tree;
- struct completion notifier_completion;
int dying;
unsigned int page_shift;
struct work_struct work;
@@ -129,11 +127,10 @@ struct ib_ucontext_per_mm {
/* Protects umem_tree */
struct rw_semaphore umem_rwsem;
- struct mmu_notifier mn;
+ struct hmm_mirror mirror;
unsigned int odp_mrs_count;
struct list_head ucontext_list;
- struct rcu_head rcu;
};
int ib_umem_odp_get(struct ib_umem_odp *umem_odp, int access);
@@ -141,9 +138,18 @@ struct ib_umem_odp *ib_alloc_odp_umem(struct ib_umem_odp *root_umem,
unsigned long addr, size_t size);
void ib_umem_odp_release(struct ib_umem_odp *umem_odp);
-int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 start_offset,
- u64 bcnt, u64 access_mask,
- unsigned long current_seq);
+#define ODP_READ_BIT (1<<0ULL)
+#define ODP_WRITE_BIT (1<<1ULL)
+/*
+ * The device bit is not use by ODP but is there to full-fill HMM API which
+ * also support device with device memory (like GPU). So from ODP/RDMA POV
+ * this can be ignored.
+ */
+#define ODP_DEVICE_BIT (1<<2ULL)
+#define ODP_FLAGS_BITS 3
+
+long ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp,
+ struct hmm_range *range);
void ib_umem_odp_unmap_dma_pages(struct ib_umem_odp *umem_odp, u64 start_offset,
u64 bound);
@@ -166,23 +172,6 @@ int rbt_ib_umem_for_each_in_range(struct rb_root_cached *root,
struct ib_umem_odp *rbt_ib_umem_lookup(struct rb_root_cached *root,
u64 addr, u64 length);
-static inline int ib_umem_mmu_notifier_retry(struct ib_umem_odp *umem_odp,
- unsigned long mmu_seq)
-{
- /*
- * This code is strongly based on the KVM code from
- * mmu_notifier_retry. Should be called with
- * the relevant locks taken (umem_odp->umem_mutex
- * and the ucontext umem_mutex semaphore locked for read).
- */
-
- if (unlikely(umem_odp->notifiers_count))
- return 1;
- if (umem_odp->notifiers_seq != mmu_seq)
- return 1;
- return 0;
-}
-
#else /* CONFIG_INFINIBAND_ON_DEMAND_PAGING */
static inline int ib_umem_odp_get(struct ib_umem_odp *umem_odp, int access)
--
2.20.1
^ permalink raw reply related
* Re: [PATCH rdma-next v2 00/17] Statistics counter support
From: Jason Gunthorpe @ 2019-05-22 17:31 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Majd Dibbiny,
Mark Zhang, Saeed Mahameed, linux-netdev
In-Reply-To: <20190429083453.16654-1-leon@kernel.org>
On Mon, Apr 29, 2019 at 11:34:36AM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
>
> Changelog:
> v1 -> v2:
> * Rebased to latest rdma-next
> v0 -> v1:
> * Changed wording of counter comment
> * Removed unneeded assignments
> * Added extra patch to present global counters
>
> * I didn't change QP type from int to be enum ib_qp_type,
> because it caused to cyclic dependency between ib_verbs.h and
> rdma_counter.h.
>
>
> Hi,
>
> This series from Mark provides dynamic statistics infrastructure.
> He uses netlink interface to configure and retrieve those counters.
>
> This infrastructure allows to users monitor various objects by binding
> to them counters. As the beginning, we used QP object as target for
> those counters, but future patches will include ODP MR information too.
>
> Two binding modes are supported:
> - Auto: This allows a user to build automatic set of objects to a counter
> according to common criteria. For example in a per-type scheme, where in
> one process all QPs with same QP type are bound automatically to a single
> counter.
> - Manual: This allows a user to manually bind objects on a counter.
>
> Those two modes are mutual-exclusive with separation between processes,
> objects created by different processes cannot be bound to a same counter.
>
> For objects which don't support counter binding, we will return
> pre-allocated counters.
>
> $ rdma statistic qp set link mlx5_2/1 auto type on
> $ rdma statistic qp set link mlx5_2/1 auto off
> $ rdma statistic qp bind link mlx5_2/1 lqpn 178
> $ rdma statistic qp unbind link mlx5_2/1 cntn 4 lqpn 178
> $ rdma statistic show
> $ rdma statistic qp mode
Can you please include the command outputs?
Jason
^ permalink raw reply
* Re: [PATCH rdma-next v2 17/17] RDMA/nldev: Allow get default counter statistics through RDMA netlink
From: Jason Gunthorpe @ 2019-05-22 17:30 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Majd Dibbiny,
Mark Zhang, Saeed Mahameed, linux-netdev
In-Reply-To: <20190429083453.16654-18-leon@kernel.org>
On Mon, Apr 29, 2019 at 11:34:53AM +0300, Leon Romanovsky wrote:
> From: Mark Zhang <markz@mellanox.com>
>
> This patch adds the ability to return the hwstats of per-port default
> counters (which can also be queried through sysfs nodes).
>
> Signed-off-by: Mark Zhang <markz@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> drivers/infiniband/core/nldev.c | 101 +++++++++++++++++++++++++++++++-
> 1 file changed, 99 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c
> index 53c1d2d82a06..cb2dd38f49f1 100644
> +++ b/drivers/infiniband/core/nldev.c
> @@ -1709,6 +1709,98 @@ static int nldev_stat_del_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
> return ret;
> }
>
> +static int nldev_res_get_default_counter_doit(struct sk_buff *skb,
> + struct nlmsghdr *nlh,
> + struct netlink_ext_ack *extack,
> + struct nlattr *tb[])
> +{
> + struct rdma_hw_stats *stats;
> + struct nlattr *table_attr;
> + struct ib_device *device;
> + int ret, num_cnts, i;
> + struct sk_buff *msg;
> + u32 index, port;
> + u64 v;
> +
> + if (!tb[RDMA_NLDEV_ATTR_DEV_INDEX] || !tb[RDMA_NLDEV_ATTR_PORT_INDEX])
> + return -EINVAL;
> +
> + index = nla_get_u32(tb[RDMA_NLDEV_ATTR_DEV_INDEX]);
> + device = ib_device_get_by_index(sock_net(skb->sk), index);
> + if (!device)
> + return -EINVAL;
> +
> + if (!device->ops.alloc_hw_stats || !device->ops.get_hw_stats) {
> + ret = -EINVAL;
> + goto err;
> + }
> +
> + port = nla_get_u32(tb[RDMA_NLDEV_ATTR_PORT_INDEX]);
> + if (!rdma_is_port_valid(device, port)) {
> + ret = -EINVAL;
> + goto err;
> + }
> +
> + msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> + if (!msg) {
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + nlh = nlmsg_put(msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
> + RDMA_NL_GET_TYPE(RDMA_NL_NLDEV,
> + RDMA_NLDEV_CMD_STAT_GET),
> + 0, 0);
> +
> + if (fill_nldev_handle(msg, device) ||
> + nla_put_u32(msg, RDMA_NLDEV_ATTR_PORT_INDEX, port)) {
> + ret = -EMSGSIZE;
> + goto err_msg;
> + }
> +
> + stats = device->ops.alloc_hw_stats(device, port);
> + if (!stats) {
> + ret = -ENOMEM;
> + goto err_msg;
> + }
Why do we need yet another one of these to be allocated?
> + num_cnts = device->ops.get_hw_stats(device, stats, port, 0);
Is '0' right here?
Jason
^ permalink raw reply
* Re: [PATCH rdma-next v2 13/17] RDMA/core: Get sum value of all counters when perform a sysfs stat read
From: Jason Gunthorpe @ 2019-05-22 17:26 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Majd Dibbiny,
Mark Zhang, Saeed Mahameed, linux-netdev
In-Reply-To: <20190429083453.16654-14-leon@kernel.org>
On Mon, Apr 29, 2019 at 11:34:49AM +0300, Leon Romanovsky wrote:
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index c56ffc61ab1e..8ae4906a60e7 100644
> +++ b/drivers/infiniband/core/device.c
> @@ -1255,7 +1255,11 @@ int ib_register_device(struct ib_device *device, const char *name)
> goto dev_cleanup;
> }
>
> - rdma_counter_init(device);
> + ret = rdma_counter_init(device);
> + if (ret) {
> + dev_warn(&device->dev, "Couldn't initialize counter\n");
> + goto sysfs_cleanup;
> + }
Don't put this things randomly, if there is some reason it should be
after sysfs it needs a comment, otherwise if it is just allocating
memory it belongs earlier, and the unwind should be done in release.
I also think it is very strange/wrong that both sysfs and counters are
allocating the same alloc_hw_stats object
Why can't they share?
Jason
^ permalink raw reply
* Re: [PATCH rdma-next v2 11/17] RDMA/netlink: Implement counter dumpit calback
From: Jason Gunthorpe @ 2019-05-22 17:22 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Majd Dibbiny,
Mark Zhang, Saeed Mahameed, linux-netdev
In-Reply-To: <20190429083453.16654-12-leon@kernel.org>
On Mon, Apr 29, 2019 at 11:34:47AM +0300, Leon Romanovsky wrote:
> From: Mark Zhang <markz@mellanox.com>
>
> This patch adds the ability to return all available counters
> together with their properties and hwstats.
>
> Signed-off-by: Mark Zhang <markz@mellanox.com>
> Reviewed-by: Majd Dibbiny <majd@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> drivers/infiniband/core/counters.c | 28 +++++
> drivers/infiniband/core/device.c | 2 +
> drivers/infiniband/core/nldev.c | 173 +++++++++++++++++++++++++++++
> include/rdma/ib_verbs.h | 10 ++
> include/rdma/rdma_counter.h | 3 +
> include/uapi/rdma/rdma_netlink.h | 10 +-
> 6 files changed, 225 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/core/counters.c b/drivers/infiniband/core/counters.c
> index 665e0d43c21b..36cd9eca1e46 100644
> +++ b/drivers/infiniband/core/counters.c
> @@ -62,6 +62,9 @@ static struct rdma_counter *rdma_counter_alloc(struct ib_device *dev, u8 port,
> {
> struct rdma_counter *counter;
>
> + if (!dev->ops.counter_alloc_stats)
> + return NULL;
> +
> counter = kzalloc(sizeof(*counter), GFP_KERNEL);
> if (!counter)
> return NULL;
> @@ -69,16 +72,25 @@ static struct rdma_counter *rdma_counter_alloc(struct ib_device *dev, u8 port,
> counter->device = dev;
> counter->port = port;
> counter->res.type = RDMA_RESTRACK_COUNTER;
> + counter->stats = dev->ops.counter_alloc_stats(counter);
> + if (!counter->stats)
> + goto err_stats;
> +
> counter->mode.mode = mode;
> atomic_set(&counter->usecnt, 0);
> mutex_init(&counter->lock);
>
> return counter;
> +
> +err_stats:
> + kfree(counter);
> + return NULL;
> }
>
> static void rdma_counter_dealloc(struct rdma_counter *counter)
> {
> rdma_restrack_del(&counter->res);
> + kfree(counter->stats);
> kfree(counter);
> }
>
> @@ -279,6 +291,22 @@ int rdma_counter_unbind_qp(struct ib_qp *qp, bool force)
> return 0;
> }
>
> +int rdma_counter_query_stats(struct rdma_counter *counter)
> +{
> + int ret;
> +
> + struct ib_device *dev = counter->device;
> +
Extra blank line
Something about festive trees
Jason
^ permalink raw reply
* Re: [PATCH rdma-next v2 11/17] RDMA/netlink: Implement counter dumpit calback
From: Jason Gunthorpe @ 2019-05-22 17:21 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Majd Dibbiny,
Mark Zhang, Saeed Mahameed, linux-netdev
In-Reply-To: <20190429083453.16654-12-leon@kernel.org>
On Mon, Apr 29, 2019 at 11:34:47AM +0300, Leon Romanovsky wrote:
> From: Mark Zhang <markz@mellanox.com>
>
> This patch adds the ability to return all available counters
> together with their properties and hwstats.
>
> Signed-off-by: Mark Zhang <markz@mellanox.com>
> Reviewed-by: Majd Dibbiny <majd@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> drivers/infiniband/core/counters.c | 28 +++++
> drivers/infiniband/core/device.c | 2 +
> drivers/infiniband/core/nldev.c | 173 +++++++++++++++++++++++++++++
> include/rdma/ib_verbs.h | 10 ++
> include/rdma/rdma_counter.h | 3 +
> include/uapi/rdma/rdma_netlink.h | 10 +-
> 6 files changed, 225 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/core/counters.c b/drivers/infiniband/core/counters.c
> index 665e0d43c21b..36cd9eca1e46 100644
> +++ b/drivers/infiniband/core/counters.c
> @@ -62,6 +62,9 @@ static struct rdma_counter *rdma_counter_alloc(struct ib_device *dev, u8 port,
> {
> struct rdma_counter *counter;
>
> + if (!dev->ops.counter_alloc_stats)
> + return NULL;
> +
Seems weird to add this now, why was it Ok to have counters prior to
this patch?
Jason
^ permalink raw reply
* Re: [PATCH rdma-next v2 06/17] RDMA/counter: Add "auto" configuration mode support
From: Jason Gunthorpe @ 2019-05-22 17:15 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Majd Dibbiny,
Mark Zhang, Saeed Mahameed, linux-netdev
In-Reply-To: <20190429083453.16654-7-leon@kernel.org>
On Mon, Apr 29, 2019 at 11:34:42AM +0300, Leon Romanovsky wrote:
> +/**
> + * rdma_counter_unbind_qp - Unbind a qp from a counter
> + * @force:
> + * true - Decrease the counter ref-count anyway (e.g., qp destroy)
> + */
> +int rdma_counter_unbind_qp(struct ib_qp *qp, bool force)
> +{
> + struct rdma_counter *counter = qp->counter;
> + int ret;
> +
> + if (!counter)
> + return -EINVAL;
> +
> + ret = __rdma_counter_unbind_qp(qp, force);
> + if (ret && !force)
> + return ret;
> +
> + rdma_restrack_put(&counter->res);
> + if (atomic_dec_and_test(&counter->usecnt))
> + rdma_counter_dealloc(counter);
An atomic that does kfree when it reaches zero should be implemented
with a kref.
Jason
^ permalink raw reply
* Re: [PATCH rdma-next v2 06/17] RDMA/counter: Add "auto" configuration mode support
From: Jason Gunthorpe @ 2019-05-22 17:11 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Majd Dibbiny,
Mark Zhang, Saeed Mahameed, linux-netdev
In-Reply-To: <20190429083453.16654-7-leon@kernel.org>
On Mon, Apr 29, 2019 at 11:34:42AM +0300, Leon Romanovsky wrote:
> void rdma_counter_init(struct ib_device *dev)
> {
> struct rdma_port_counter *port_counter;
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index 9204b4251fc8..dfaa57de871f 100644
> +++ b/drivers/infiniband/core/device.c
> @@ -2349,6 +2349,8 @@ void ib_set_device_ops(struct ib_device *dev, const struct ib_device_ops *ops)
> SET_DEVICE_OP(dev_ops, set_vf_guid);
> SET_DEVICE_OP(dev_ops, set_vf_link_state);
> SET_DEVICE_OP(dev_ops, unmap_fmr);
> + SET_DEVICE_OP(dev_ops, counter_bind_qp);
> + SET_DEVICE_OP(dev_ops, counter_unbind_qp);
Keep sorted
Jason
^ permalink raw reply
* Re: [PATCH rdma-next v2 13/17] RDMA/core: Get sum value of all counters when perform a sysfs stat read
From: Jason Gunthorpe @ 2019-05-22 17:10 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Majd Dibbiny,
Mark Zhang, Saeed Mahameed, linux-netdev
In-Reply-To: <20190429083453.16654-14-leon@kernel.org>
On Mon, Apr 29, 2019 at 11:34:49AM +0300, Leon Romanovsky wrote:
> From: Mark Zhang <markz@mellanox.com>
>
> Since a QP can only be bound to one counter, then if it is bound to a
> separate counter, for backward compatibility purpose, the statistic
> value must be:
> * stat of default counter
> + stat of all running allocated counters
> + stat of all deallocated counters (history stats)
>
> Signed-off-by: Mark Zhang <markz@mellanox.com>
> Reviewed-by: Majd Dibbiny <majd@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> drivers/infiniband/core/counters.c | 99 +++++++++++++++++++++++++++++-
> drivers/infiniband/core/device.c | 8 ++-
> drivers/infiniband/core/sysfs.c | 10 ++-
> include/rdma/rdma_counter.h | 5 +-
> 4 files changed, 113 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/infiniband/core/counters.c b/drivers/infiniband/core/counters.c
> index 36cd9eca1e46..f598b1cdb241 100644
> +++ b/drivers/infiniband/core/counters.c
> @@ -146,6 +146,20 @@ static int __rdma_counter_bind_qp(struct rdma_counter *counter,
> return ret;
> }
>
> +static void counter_history_stat_update(const struct rdma_counter *counter)
> +{
> + struct ib_device *dev = counter->device;
> + struct rdma_port_counter *port_counter;
> + int i;
> +
> + port_counter = &dev->port_data[counter->port].port_counter;
> + if (!port_counter->hstats)
> + return;
> +
> + for (i = 0; i < counter->stats->num_counters; i++)
> + port_counter->hstats->value[i] += counter->stats->value[i];
> +}
> +
> static int __rdma_counter_unbind_qp(struct ib_qp *qp, bool force)
> {
> struct rdma_counter *counter = qp->counter;
> @@ -285,8 +299,10 @@ int rdma_counter_unbind_qp(struct ib_qp *qp, bool force)
> return ret;
>
> rdma_restrack_put(&counter->res);
> - if (atomic_dec_and_test(&counter->usecnt))
> + if (atomic_dec_and_test(&counter->usecnt)) {
> + counter_history_stat_update(counter);
> rdma_counter_dealloc(counter);
> + }
>
> return 0;
> }
> @@ -307,21 +323,98 @@ int rdma_counter_query_stats(struct rdma_counter *counter)
> return ret;
> }
>
> -void rdma_counter_init(struct ib_device *dev)
> +static u64 get_running_counters_hwstat_sum(struct ib_device *dev,
> + u8 port, u32 index)
> +{
> + struct rdma_restrack_entry *res;
> + struct rdma_restrack_root *rt;
> + struct rdma_counter *counter;
> + unsigned long id = 0;
> + u64 sum = 0;
> +
> + rt = &dev->res[RDMA_RESTRACK_COUNTER];
> + xa_lock(&rt->xa);
> + xa_for_each(&rt->xa, id, res) {
> + if (!rdma_restrack_get(res))
> + continue;
Why do we need to get refcounts if we are holding the xa_lock?
> +
> + counter = container_of(res, struct rdma_counter, res);
> + if ((counter->device != dev) || (counter->port != port))
> + goto next;
> +
> + if (rdma_counter_query_stats(counter))
> + goto next;
And rdma_counter_query_stats does
+ mutex_lock(&counter->lock);
So this was never tested as it will insta-crash with lockdep.
Presumably this is why it is using xa_for_each and restrack_get - but
it needs to drop the lock after successful get.
This sort of comment applies to nearly evey place in this series that
uses xa_for_each.
This needs to be tested with lockdep.
Jason
^ permalink raw reply
* Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
From: enh @ 2019-05-22 16:58 UTC (permalink / raw)
To: Catalin Marinas
Cc: Kees Cook, 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 9:35 AM Catalin Marinas <catalin.marinas@arm.com> 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.
> >
> > 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.)
>
> That's not what I meant (or I misunderstood you). If we have a relaxed
> ABI in the kernel and a libc that returns tagged pointers on malloc() I
> wouldn't expect the programmer to do anything different in the
> application code like explicit untagging. Basically the program would
> continue to run unmodified irrespective of whether you use an old libc
> without tagged pointers or a new one which tags heap allocations.
>
> What I do expect is that the libc checks for the presence of the relaxed
> ABI, currently proposed as an AT_FLAGS bit (for MTE we'd have a
> HWCAP_MTE), and only tag the malloc() pointers if the kernel supports
> the relaxed ABI. As you said, you shouldn't expect that the C library
> and kernel are upgraded together, so they should be able to work in any
> new/old version combination.
yes, that part makes sense. i do think we'd use the AT_FLAGS bit, for
exactly this.
i was questioning the argument about the ioctl issues, and saying that
from my perspective, untagging bugs are not really any different than
any other kind of kernel bug.
> > > > 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.)
>
> 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.
>
> The distro user space usually has a hard requirement that it continues
> to run on (certain) old hardware. We can't enforce this in the kernel
> but we offer the option to user space developers of checking feature
> availability through HWCAP bits.
>
> The Android story may be different as you have more control about which
> kernel configurations are deployed on specific SoCs. I'm looking more
> from a Linux distro angle where you just get an off-the-shelf OS image
> and install it on your hardware, either taking advantage of new features
> or just not using them if the software was not updated. Or, if updated
> software is installed on old hardware, it would just run.
>
> 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.
>
> 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.
(a bit off-topic, but i thought the MTE instructions were encoded in
the no-op space, to avoid this?)
> > i'm not sure i see this new way for a kernel update to break my system
> > and need to be fixed forward/rolled back as any different from any of
> > the existing ways in which this can happen :-) as an end-user i have
> > to rely on whoever's sending me software updates to test adequately
> > enough that they find the problems. as an end user, there isn't any
> > difference between "my phone rebooted when i tried to take a photo
> > because of a kernel/driver leak", say, and "my phone rebooted when i
> > tried to take a photo because of missing untagging of a pointer passed
> > via ioctl".
> >
> > i suspect you and i have very different people in mind when we say "user" :-)
>
> Indeed, I think we have different users in mind. I didn't mean the end
> user who doesn't really care which C library version it's running on
> their phone but rather advanced users (not necessarily kernel
> developers) that prefer to build their own kernels with every release.
> We could extend this to kernel developers who don't have time to track
> down why a new kernel triggers lots of SIGSEGVs during boot.
i still don't see how this isn't just a regular testing/CI issue, the
same as any other kind of kernel bug. it's already the case that i can
get a bad kernel...
> --
> Catalin
^ permalink raw reply
* Re: [PATCH rdma-next v2 05/17] RDMA/counter: Add set/clear per-port auto mode support
From: Jason Gunthorpe @ 2019-05-22 16:56 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Majd Dibbiny,
Mark Zhang, Saeed Mahameed, linux-netdev
In-Reply-To: <20190429083453.16654-6-leon@kernel.org>
On Mon, Apr 29, 2019 at 11:34:41AM +0300, Leon Romanovsky wrote:
> From: Mark Zhang <markz@mellanox.com>
>
> Add an API to support set/clear per-port auto mode.
>
> Signed-off-by: Mark Zhang <markz@mellanox.com>
> Reviewed-by: Majd Dibbiny <majd@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> drivers/infiniband/core/Makefile | 2 +-
> drivers/infiniband/core/counters.c | 77 ++++++++++++++++++++++++++++++
> drivers/infiniband/core/device.c | 4 ++
> include/rdma/ib_verbs.h | 2 +
> include/rdma/rdma_counter.h | 24 ++++++++++
> include/uapi/rdma/rdma_netlink.h | 26 ++++++++++
> 6 files changed, 134 insertions(+), 1 deletion(-)
> create mode 100644 drivers/infiniband/core/counters.c
>
> diff --git a/drivers/infiniband/core/Makefile b/drivers/infiniband/core/Makefile
> index 313f2349b518..cddf748c15c9 100644
> +++ b/drivers/infiniband/core/Makefile
> @@ -12,7 +12,7 @@ ib_core-y := packer.o ud_header.o verbs.o cq.o rw.o sysfs.o \
> device.o fmr_pool.o cache.o netlink.o \
> roce_gid_mgmt.o mr_pool.o addr.o sa_query.o \
> multicast.o mad.o smi.o agent.o mad_rmpp.o \
> - nldev.o restrack.o
> + nldev.o restrack.o counters.o
>
> ib_core-$(CONFIG_SECURITY_INFINIBAND) += security.o
> ib_core-$(CONFIG_CGROUP_RDMA) += cgroup.o
> diff --git a/drivers/infiniband/core/counters.c b/drivers/infiniband/core/counters.c
> new file mode 100644
> index 000000000000..bda8d945a758
> +++ b/drivers/infiniband/core/counters.c
> @@ -0,0 +1,77 @@
> +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
> +/*
> + * Copyright (c) 2019 Mellanox Technologies. All rights reserved.
> + */
> +#include <rdma/ib_verbs.h>
> +#include <rdma/rdma_counter.h>
> +
> +#include "core_priv.h"
> +#include "restrack.h"
> +
> +#define ALL_AUTO_MODE_MASKS (RDMA_COUNTER_MASK_QP_TYPE)
> +
> +static int __counter_set_mode(struct rdma_counter_mode *curr,
> + enum rdma_nl_counter_mode new_mode,
> + enum rdma_nl_counter_mask new_mask)
> +{
> + if ((new_mode == RDMA_COUNTER_MODE_AUTO) &&
> + ((new_mask & (~ALL_AUTO_MODE_MASKS)) ||
> + (curr->mode != RDMA_COUNTER_MODE_NONE)))
> + return -EINVAL;
> +
> + curr->mode = new_mode;
> + curr->mask = new_mask;
> + return 0;
> +}
> +
> +/**
> + * rdma_counter_set_auto_mode() - Turn on/off per-port auto mode
> + *
> + * When @on is true, the @mask must be set
> + */
> +int rdma_counter_set_auto_mode(struct ib_device *dev, u8 port,
> + bool on, enum rdma_nl_counter_mask mask)
> +{
> + struct rdma_port_counter *port_counter;
> + int ret;
> +
> + if (!rdma_is_port_valid(dev, port))
> + return -EINVAL;
> +
> + port_counter = &dev->port_data[port].port_counter;
> + mutex_lock(&port_counter->lock);
> + if (on) {
> + ret = __counter_set_mode(&port_counter->mode,
> + RDMA_COUNTER_MODE_AUTO, mask);
> + } else {
> + if (port_counter->mode.mode != RDMA_COUNTER_MODE_AUTO) {
> + ret = -EINVAL;
> + goto out;
> + }
> + ret = __counter_set_mode(&port_counter->mode,
> + RDMA_COUNTER_MODE_NONE, 0);
> + }
> +
> +out:
> + mutex_unlock(&port_counter->lock);
> + return ret;
> +}
> +
> +void rdma_counter_init(struct ib_device *dev)
> +{
> + struct rdma_port_counter *port_counter;
> + u32 port;
> +
> + if (!dev->ops.alloc_hw_stats)
> + return;
> +
> + rdma_for_each_port(dev, port) {
> + port_counter = &dev->port_data[port].port_counter;
> + port_counter->mode.mode = RDMA_COUNTER_MODE_NONE;
> + mutex_init(&port_counter->lock);
> + }
> +}
> +
> +void rdma_counter_cleanup(struct ib_device *dev)
> +{
> +}
Please don't add empty functions
> @@ -1304,6 +1307,7 @@ static void __ib_unregister_device(struct ib_device *ib_dev)
> goto out;
>
> disable_device(ib_dev);
> + rdma_counter_cleanup(ib_dev);
This is the wrong place to call this, the patch that actually adds a
body is just doing kfree's so it is properly called
'rdma_counter_release' and it belongs in ib_device_release()
And it shouldn't test hw_stats, and it shouldn't have a 'fail' stanza
for allocation either.
Jason
^ permalink raw reply
* Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
From: Catalin Marinas @ 2019-05-22 16:35 UTC (permalink / raw)
To: enh
Cc: Kees Cook, 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: <CAJgzZoosKBwqXRyA6fb8QQSZXFqfHqe9qO9je5TogHhzuoGXJQ@mail.gmail.com>
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.
>
> 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.)
That's not what I meant (or I misunderstood you). If we have a relaxed
ABI in the kernel and a libc that returns tagged pointers on malloc() I
wouldn't expect the programmer to do anything different in the
application code like explicit untagging. Basically the program would
continue to run unmodified irrespective of whether you use an old libc
without tagged pointers or a new one which tags heap allocations.
What I do expect is that the libc checks for the presence of the relaxed
ABI, currently proposed as an AT_FLAGS bit (for MTE we'd have a
HWCAP_MTE), and only tag the malloc() pointers if the kernel supports
the relaxed ABI. As you said, you shouldn't expect that the C library
and kernel are upgraded together, so they should be able to work in any
new/old version combination.
> > > 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.)
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.
The distro user space usually has a hard requirement that it continues
to run on (certain) old hardware. We can't enforce this in the kernel
but we offer the option to user space developers of checking feature
availability through HWCAP bits.
The Android story may be different as you have more control about which
kernel configurations are deployed on specific SoCs. I'm looking more
from a Linux distro angle where you just get an off-the-shelf OS image
and install it on your hardware, either taking advantage of new features
or just not using them if the software was not updated. Or, if updated
software is installed on old hardware, it would just run.
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.
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'm not sure i see this new way for a kernel update to break my system
> and need to be fixed forward/rolled back as any different from any of
> the existing ways in which this can happen :-) as an end-user i have
> to rely on whoever's sending me software updates to test adequately
> enough that they find the problems. as an end user, there isn't any
> difference between "my phone rebooted when i tried to take a photo
> because of a kernel/driver leak", say, and "my phone rebooted when i
> tried to take a photo because of missing untagging of a pointer passed
> via ioctl".
>
> i suspect you and i have very different people in mind when we say "user" :-)
Indeed, I think we have different users in mind. I didn't mean the end
user who doesn't really care which C library version it's running on
their phone but rather advanced users (not necessarily kernel
developers) that prefer to build their own kernels with every release.
We could extend this to kernel developers who don't have time to track
down why a new kernel triggers lots of SIGSEGVs during boot.
--
Catalin
^ permalink raw reply
* Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
From: enh @ 2019-05-22 15:30 UTC (permalink / raw)
To: Catalin Marinas
Cc: Kees Cook, 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: <20190522101110.m2stmpaj7seezveq@mbp>
On Wed, May 22, 2019 at 3:11 AM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> Hi Kees,
>
> Thanks for joining the thread ;).
>
> On Tue, May 21, 2019 at 05:04:39PM -0700, Kees Cook wrote:
> > On Tue, May 21, 2019 at 07:29:33PM +0100, Catalin Marinas wrote:
> > > On Mon, May 20, 2019 at 04:53:07PM -0700, Evgenii Stepanov wrote:
> > > > On Fri, May 17, 2019 at 7:49 AM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > > > IMO (RFC for now), I see two ways forward:
> > > > > [...]
> > > > > 2. Similar shim to the above libc wrapper but inside the kernel
> > > > > (arch/arm64 only; most pointer arguments could be covered with an
> > > > > __SC_CAST similar to the s390 one). There are two differences from
> > > > > what we've discussed in the past:
> > > > >
> > > > > a) this is an opt-in by the user which would have to explicitly call
> > > > > prctl(). If it returns -ENOTSUPP etc., the user won't be allowed
> > > > > to pass tagged pointers to the kernel. This would probably be the
> > > > > responsibility of the C lib to make sure it doesn't tag heap
> > > > > allocations. If the user did not opt-in, the syscalls are routed
> > > > > through the normal path (no untagging address shim).
> > > > >
> > > > > b) ioctl() and other blacklisted syscalls (prctl) will not accept
> > > > > tagged pointers (to be documented in Vicenzo's ABI patches).
> > > >
> > > > The way I see it, a patch that breaks handling of tagged pointers is
> > > > not that different from, say, a patch that adds a wild pointer
> > > > dereference. Both are bugs; the difference is that (a) the former
> > > > breaks a relatively uncommon target and (b) it's arguably an easier
> > > > mistake to make. If MTE adoption goes well, (a) will not be the case
> > > > for long.
> > >
> > > It's also the fact such patch would go unnoticed for a long time until
> > > someone exercises that code path. And when they do, the user would be
> > > pretty much in the dark trying to figure what what went wrong, why a
> > > SIGSEGV or -EFAULT happened. What's worse, we can't even say we fixed
> > > all the places where it matters in the current kernel codebase (ignoring
> > > future patches).
> >
> > So, looking forward a bit, this isn't going to be an ARM-specific issue
> > for long.
>
> I do hope so.
>
> > In fact, I think we shouldn't have arm-specific syscall wrappers
> > in this series: I think untagged_addr() should likely be added at the
> > top-level and have it be a no-op for other architectures.
>
> That's what the current patchset does, so we have this as a starting
> point. Kostya raised another potential issue with the syscall wrappers:
> with MTE the kernel will be forced to enable the match-all (wildcard)
> pointers for user space accesses since copy_from_user() would only get a
> 0 tag. So it has wider implications than just uaccess routines not
> checking the colour.
>
> > So given this becoming a kernel-wide multi-architecture issue (under
> > the assumption that x86, RISC-V, and others will gain similar TBI or
> > MTE things), we should solve it in a way that we can re-use.
>
> Can we do any better to aid the untagged_addr() placement (e.g. better
> type annotations, better static analysis)? We have to distinguish
> between user pointers that may be dereferenced by the kernel (I think
> almost fully covered with this patchset) and user addresses represented
> as ulong that may:
>
> a) be converted to a user pointer and dereferenced; I think that's the
> case for many overloaded ulong/u64 arguments
>
> b) used for address space management, rbtree look-ups etc. where the tag
> is no longer relevant and it even gets in the way
>
> We tried last year to identify void __user * casts to unsigned long
> using sparse on the assumption that pointers can be tagged while ulong
> is about address space management and needs to lose such tag. I think we
> could have pushed this further. For example, get_user_pages() takes an
> unsigned long but it is perfectly capable of untagging the address
> itself. Shall we change its first argument to void __user * (together
> with all its callers)?
>
> find_vma(), OTOH, could untag the address but it doesn't help since
> vm_start/end don't have such information (that's more about the content
> or type that the user decided) and the callers check against it.
>
> Are there any other places where this matters? These patches tracked
> down find_vma() as some heuristics but we may need better static
> analysis to identify other cases.
>
> > We need something that is going to work everywhere. And it needs to be
> > supported by the kernel for the simple reason that the kernel needs to
> > do MTE checks during copy_from_user(): having that information stripped
> > means we lose any userspace-assigned MTE protections if they get handled
> > by the kernel, which is a total non-starter, IMO.
>
> Such feedback is welcomed ;).
>
> > As an aside: I think Sparc ADI support in Linux actually side-stepped
> > this[1] (i.e. chose "solution 1"): "All addresses passed to kernel must
> > be non-ADI tagged addresses." (And sadly, "Kernel does not enable ADI
> > for kernel code.") I think this was a mistake we should not repeat for
> > arm64 (we do seem to be at least in agreement about this, I think).
> >
> > [1] https://lore.kernel.org/patchwork/patch/654481/
>
> I tried to drag the SPARC guys into this discussion but without much
> success.
>
> > > > This is a bit of a chicken-and-egg problem. In a world where memory
> > > > allocators on one or several popular platforms generate pointers with
> > > > non-zero tags, any such breakage will be caught in testing.
> > > > Unfortunately to reach that state we need the kernel to start
> > > > accepting tagged pointers first, and then hold on for a couple of
> > > > years until userspace catches up.
> > >
> > > Would the kernel also catch up with providing a stable ABI? Because we
> > > have two moving targets.
> > >
> > > On one hand, you have Android or some Linux distro that stick to a
> > > stable kernel version for some time, so they have better chance of
> > > clearing most of the problems. On the other hand, we have mainline
> > > kernel that gets over 500K lines every release. As maintainer, I can't
> > > rely on my testing alone as this is on a limited number of platforms. So
> > > my concern is that every kernel release has a significant chance of
> > > breaking the ABI, unless we have a better way of identifying potential
> > > issues.
> >
> > 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.
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.)
> > - 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.
>
> 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.)
i'm not sure i see this new way for a kernel update to break my system
and need to be fixed forward/rolled back as any different from any of
the existing ways in which this can happen :-) as an end-user i have
to rely on whoever's sending me software updates to test adequately
enough that they find the problems. as an end user, there isn't any
difference between "my phone rebooted when i tried to take a photo
because of a kernel/driver leak", say, and "my phone rebooted when i
tried to take a photo because of missing untagging of a pointer passed
via ioctl".
i suspect you and i have very different people in mind when we say "user" :-)
> > 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.
>
> Thanks.
>
> --
> Catalin
^ permalink raw reply
* Re: [PATCH v15 17/17] selftests, arm64: add a selftest for passing tagged pointers to kernel
From: Catalin Marinas @ 2019-05-22 14:16 UTC (permalink / raw)
To: Andrey Konovalov
Cc: linux-arm-kernel, linux-mm, linux-kernel, amd-gfx, dri-devel,
linux-rdma, linux-media, kvm, linux-kselftest, Vincenzo Frascino,
Will Deacon, Mark Rutland, Andrew Morton, Greg Kroah-Hartman,
Kees Cook, Yishai Hadas, Felix Kuehling, Alexander Deucher,
Christian Koenig, Mauro Carvalho Chehab, Jens Wiklander
In-Reply-To: <e31d9364eb0c2eba8ce246a558422e811d82d21b.1557160186.git.andreyknvl@google.com>
On Mon, May 06, 2019 at 06:31:03PM +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 adds a simple test, that calls the uname syscall with a
> tagged user pointer as an argument. Without the kernel accepting tagged
> user pointers the test fails with EFAULT.
That's probably sufficient for a simple example. Something we could add
to Documentation maybe is a small library that can be LD_PRELOAD'ed so
that you can run a lot more tests like LTP.
We could add this to selftests but I think it's too glibc specific.
--------------------8<------------------------------------
#include <stdlib.h>
#define TAG_SHIFT (56)
#define TAG_MASK (0xffUL << TAG_SHIFT)
void *__libc_malloc(size_t size);
void __libc_free(void *ptr);
void *__libc_realloc(void *ptr, size_t size);
void *__libc_calloc(size_t nmemb, size_t size);
static void *tag_ptr(void *ptr)
{
unsigned long tag = rand() & 0xff;
if (!ptr)
return ptr;
return (void *)((unsigned long)ptr | (tag << TAG_SHIFT));
}
static void *untag_ptr(void *ptr)
{
return (void *)((unsigned long)ptr & ~TAG_MASK);
}
void *malloc(size_t size)
{
return tag_ptr(__libc_malloc(size));
}
void free(void *ptr)
{
__libc_free(untag_ptr(ptr));
}
void *realloc(void *ptr, size_t size)
{
return tag_ptr(__libc_realloc(untag_ptr(ptr), size));
}
void *calloc(size_t nmemb, size_t size)
{
return tag_ptr(__libc_calloc(nmemb, size));
}
^ permalink raw reply
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