* Re: [PATCH v2 hmm 10/11] mm/hmm: Do not use list*_rcu() for hmm->ranges
From: John Hubbard @ 2019-06-07 3:40 UTC (permalink / raw)
To: Jason Gunthorpe, Jerome Glisse, Ralph Campbell, Felix.Kuehling
Cc: Andrea Arcangeli, linux-rdma, amd-gfx, linux-mm, Jason Gunthorpe,
dri-devel
In-Reply-To: <20190606184438.31646-11-jgg@ziepe.ca>
On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
> From: Jason Gunthorpe <jgg@mellanox.com>
>
> This list is always read and written while holding hmm->lock so there is
> no need for the confusing _rcu annotations.
>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> Reviewed-by: Jérôme Glisse <jglisse@redhat.com>
> ---
> mm/hmm.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/hmm.c b/mm/hmm.c
> index c2fecb3ecb11e1..709d138dd49027 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -911,7 +911,7 @@ int hmm_range_register(struct hmm_range *range,
> mutex_lock(&hmm->lock);
>
> range->hmm = hmm;
> - list_add_rcu(&range->list, &hmm->ranges);
> + list_add(&range->list, &hmm->ranges);
>
> /*
> * If there are any concurrent notifiers we have to wait for them for
> @@ -941,7 +941,7 @@ void hmm_range_unregister(struct hmm_range *range)
> return;
>
> mutex_lock(&hmm->lock);
> - list_del_rcu(&range->list);
> + list_del(&range->list);
> mutex_unlock(&hmm->lock);
>
> /* Drop reference taken by hmm_range_register() */
>
Good point. I'd overlooked this many times.
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
thanks,
--
John Hubbard
NVIDIA
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v2 hmm 11/11] mm/hmm: Remove confusing comment and logic from hmm_release
From: John Hubbard @ 2019-06-07 3:47 UTC (permalink / raw)
To: Jason Gunthorpe, Jerome Glisse, Ralph Campbell, Felix.Kuehling
Cc: Andrea Arcangeli, linux-rdma, amd-gfx, linux-mm, Jason Gunthorpe,
dri-devel
In-Reply-To: <20190606184438.31646-12-jgg@ziepe.ca>
On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
> From: Jason Gunthorpe <jgg@mellanox.com>
>
> hmm_release() is called exactly once per hmm. ops->release() cannot
> accidentally trigger any action that would recurse back onto
> hmm->mirrors_sem.
>
> This fixes a use after-free race of the form:
>
> CPU0 CPU1
> hmm_release()
> up_write(&hmm->mirrors_sem);
> hmm_mirror_unregister(mirror)
> down_write(&hmm->mirrors_sem);
> up_write(&hmm->mirrors_sem);
> kfree(mirror)
> mirror->ops->release(mirror)
>
> The only user we have today for ops->release is an empty function, so this
> is unambiguously safe.
>
> As a consequence of plugging this race drivers are not allowed to
> register/unregister mirrors from within a release op.
>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> ---
> mm/hmm.c | 28 +++++++++-------------------
> 1 file changed, 9 insertions(+), 19 deletions(-)
>
> diff --git a/mm/hmm.c b/mm/hmm.c
> index 709d138dd49027..3a45dd3d778248 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -136,26 +136,16 @@ static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
> WARN_ON(!list_empty(&hmm->ranges));
> mutex_unlock(&hmm->lock);
>
> - down_write(&hmm->mirrors_sem);
> - mirror = list_first_entry_or_null(&hmm->mirrors, struct hmm_mirror,
> - list);
> - while (mirror) {
> - list_del_init(&mirror->list);
> - if (mirror->ops->release) {
> - /*
> - * Drop mirrors_sem so the release callback can wait
> - * on any pending work that might itself trigger a
> - * mmu_notifier callback and thus would deadlock with
> - * us.
> - */
> - up_write(&hmm->mirrors_sem);
> + down_read(&hmm->mirrors_sem);
This is cleaner and simpler, but I suspect it is leading to the deadlock
that Ralph Campbell is seeing in his driver testing. (And in general, holding
a lock during a driver callback usually leads to deadlocks.)
Ralph, is this the one? It's the only place in this patchset where I can
see a lock around a callback to driver code, that wasn't there before. So
I'm pretty sure it is the one...
thanks,
--
John Hubbard
NVIDIA
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v16 14/16] tee, arm64: untag user pointers in tee_shm_register
From: Jens Wiklander @ 2019-06-07 5:33 UTC (permalink / raw)
To: Andrey Konovalov
Cc: Linux ARM, linux-mm, Linux Kernel Mailing List, amd-gfx,
dri-devel, linux-rdma, linux-media, kvm, linux-kselftest,
Catalin Marinas, Vincenzo Frascino, Will Deacon, Mark Rutland,
Andrew Morton, Greg Kroah-Hartman, Kees Cook, Yishai Hadas,
Felix Kuehling, Alexander Deucher, Christian Koenig
In-Reply-To: <dc3f3092abbc0d48e51b2e2a2ca8f4c4f69fa0f4.1559580831.git.andreyknvl@google.com>
On Mon, Jun 3, 2019 at 6:56 PM Andrey Konovalov <andreyknvl@google.com> 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.
>
> tee_shm_register()->optee_shm_unregister()->check_mem_type() uses provided
> user pointers for vma lookups (via __check_mem_type()), which can only by
> done with untagged pointers.
>
> Untag user pointers in this function.
>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
Acked-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
> drivers/tee/tee_shm.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> index 49fd7312e2aa..96945f4cefb8 100644
> --- a/drivers/tee/tee_shm.c
> +++ b/drivers/tee/tee_shm.c
> @@ -263,6 +263,7 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr,
> shm->teedev = teedev;
> shm->ctx = ctx;
> shm->id = -1;
> + addr = untagged_addr(addr);
> start = rounddown(addr, PAGE_SIZE);
> shm->offset = addr - start;
> shm->size = length;
> --
> 2.22.0.rc1.311.g5d7573a151-goog
>
^ permalink raw reply
* Re: properly communicate queue limits to the DMA layer
From: Jens Axboe @ 2019-06-07 5:52 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Sebastian Ott, Sagi Grimberg, Max Gurtovoy, Bart Van Assche,
Ulf Hansson, Alan Stern, Oliver Neukum, linux-block, linux-rdma,
linux-mmc, linux-nvme, linux-scsi, megaraidlinux.pdl,
MPT-FusionLinux.pdl, linux-hyperv, linux-usb, usb-storage,
linux-kernel
In-Reply-To: <20190605192405.GA18243@lst.de>
On 6/5/19 1:24 PM, Christoph Hellwig wrote:
> On Wed, Jun 05, 2019 at 01:17:15PM -0600, Jens Axboe wrote:
>> Since I'm heading out shortly and since I think this should make
>> the next -rc, I'll tentatively queue this up.
>
> The SCSI bits will need a bit more review, and possibly tweaking
> fo megaraid and mpt3sas. But they are really independent of the
> other patches, so maybe skip them for now and leave them for Martin
> to deal with.
I dropped the SCSI bits.
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH RFC 00/10] RDMA/FS DAX truncate proposal
From: Jan Kara @ 2019-06-07 11:04 UTC (permalink / raw)
To: Ira Weiny
Cc: Jan Kara, Dan Williams, Theodore Ts'o, Jeff Layton,
Dave Chinner, Matthew Wilcox, linux-xfs, Andrew Morton,
John Hubbard, Jérôme Glisse, linux-fsdevel,
linux-kernel, linux-nvdimm, linux-ext4, linux-mm, Jason Gunthorpe,
linux-rdma
In-Reply-To: <20190606220329.GA11698@iweiny-DESK2.sc.intel.com>
On Thu 06-06-19 15:03:30, Ira Weiny wrote:
> On Thu, Jun 06, 2019 at 12:42:03PM +0200, Jan Kara wrote:
> > On Wed 05-06-19 18:45:33, ira.weiny@intel.com wrote:
> > > From: Ira Weiny <ira.weiny@intel.com>
> >
> > So I'd like to actually mandate that you *must* hold the file lease until
> > you unpin all pages in the given range (not just that you have an option to
> > hold a lease). And I believe the kernel should actually enforce this. That
> > way we maintain a sane state that if someone uses a physical location of
> > logical file offset on disk, he has a layout lease. Also once this is done,
> > sysadmin has a reasonably easy way to discover run-away RDMA application
> > and kill it if he wishes so.
>
> Fair enough.
>
> I was kind of heading that direction but had not thought this far forward. I
> was exploring how to have a lease remain on the file even after a "lease
> break". But that is incompatible with the current semantics of a "layout"
> lease (as currently defined in the kernel). [In the end I wanted to get an RFC
> out to see what people think of this idea so I did not look at keeping the
> lease.]
>
> Also hitch is that currently a lease is forcefully broken after
> <sysfs>/lease-break-time. To do what you suggest I think we would need a new
> lease type with the semantics you describe.
I'd do what Dave suggested - add flag to mark lease as unbreakable by
truncate and teach file locking core to handle that. There actually is
support for locks that are not broken after given timeout so there
shouldn't be too many changes need.
> Previously I had thought this would be a good idea (for other reasons). But
> what does everyone think about using a "longterm lease" similar to [1] which
> has the semantics you proppose? In [1] I was not sure "longterm" was a good
> name but with your proposal I think it makes more sense.
As I wrote elsewhere in this thread I think FL_LAYOUT name still makes
sense and I'd add there FL_UNBREAKABLE to mark unusal behavior with
truncate.
> > - probably I'd just transition all gup_longterm()
> > users to a saner API similar to the one we have in mm/frame_vector.c where
> > we don't hand out page pointers but an encapsulating structure that does
> > all the necessary tracking.
>
> I'll take a look at that code. But that seems like a pretty big change.
I was looking into that yesterday before proposing this and there aren't
than many gup_longterm() users and most of them anyway just stick pages
array into their tracking structure and then release them once done. So it
shouldn't be that complex to convert to a new convention (and you have to
touch all gup_longterm() users anyway to teach them track leases etc.).
> > Removing a lease would need to block until all
> > pins are released - this is probably the most hairy part since we need to
> > handle a case if application just closes the file descriptor which would
> > release the lease but OTOH we need to make sure task exit does not deadlock.
> > Maybe we could block only on explicit lease unlock and just drop the layout
> > lease on file close and if there are still pinned pages, send SIGKILL to an
> > application as a reminder it did something stupid...
>
> As presented at LSFmm I'm not opposed to killing a process which does not
> "follow the rules". But I'm concerned about how to handle this across a fork.
>
> Limiting the open()/LEASE/GUP/close()/SIGKILL to a specific pid "leak"'s pins
> to a child through the RDMA context. This was the major issue Jason had with
> the SIGBUS proposal.
>
> Always sending a SIGKILL would prevent an RDMA process from doing something
> like system("ls") (would kill the child unnecessarily). Are we ok with that?
I answered this in another email but system("ls") won't kill anybody.
fork(2) just creates new file descriptor for the same file and possibly
then closes it but since there is still another file descriptor for the
same struct file, the "close" code won't trigger.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* [PATCH trivial] net/mlx5e: Spelling s/configuraion/configuration/
From: Geert Uytterhoeven @ 2019-06-07 11:10 UTC (permalink / raw)
To: Saeed Mahameed, Leon Romanovsky, David S . Miller, Jiri Kosina
Cc: netdev, linux-rdma, linux-kernel, Geert Uytterhoeven
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
drivers/net/ethernet/mellanox/mlx5/core/en/port_buffer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/port_buffer.c b/drivers/net/ethernet/mellanox/mlx5/core/en/port_buffer.c
index 633b117eb13e8143..7b672ada63a39733 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/port_buffer.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/port_buffer.c
@@ -175,7 +175,7 @@ static int update_xoff_threshold(struct mlx5e_port_buffer *port_buffer,
* @port_buffer: <output> port receive buffer configuration
* @change: <output>
*
- * Update buffer configuration based on pfc configuraiton and
+ * Update buffer configuration based on pfc configuration and
* priority to buffer mapping.
* Buffer's lossy bit is changed to:
* lossless if there is at least one PFC enabled priority
--
2.17.1
^ permalink raw reply related
* Re: [PATCH trivial] net/mlx5e: Spelling s/configuraion/configuration/
From: Sergei Shtylyov @ 2019-06-07 11:37 UTC (permalink / raw)
To: Geert Uytterhoeven, Saeed Mahameed, Leon Romanovsky,
David S . Miller, Jiri Kosina
Cc: netdev, linux-rdma, linux-kernel
In-Reply-To: <20190607111026.12995-1-geert+renesas@glider.be>
Hello!
On 06/07/2019 02:10 PM, Geert Uytterhoeven wrote:
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Unfortunately, while fixing this typo, you made another one, in your subject. ;-)
[...]
MBR, Sergei
^ permalink raw reply
* [PATCH for-rc 1/3] IB/hfi1: Validate fault injection opcode user input
From: Dennis Dalessandro @ 2019-06-07 12:25 UTC (permalink / raw)
To: jgg, dledford
Cc: linux-rdma, Mike Marciniszyn, stable, Dan Carpenter, Kaike Wan
In-Reply-To: <20190607113807.157915.48581.stgit@awfm-01.aw.intel.com>
From: Kaike Wan <kaike.wan@intel.com>
The opcode range for fault injection from user should be validated
before it is applied to the fault->opcodes[] bitmap to avoid
out-of-bound error. In addition, this patch also simplifies the code
by using the BIT macro.
Fixes: a74d5307caba ("IB/hfi1: Rework fault injection machinery")
Cc: <stable@vger.kernel.org>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel.com>
Signed-off-by: Kaike Wan <kaike.wan@intel.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
---
drivers/infiniband/hw/hfi1/fault.c | 5 +++++
drivers/infiniband/hw/hfi1/fault.h | 6 +++---
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/infiniband/hw/hfi1/fault.c b/drivers/infiniband/hw/hfi1/fault.c
index 3fd3315..13ba291 100644
--- a/drivers/infiniband/hw/hfi1/fault.c
+++ b/drivers/infiniband/hw/hfi1/fault.c
@@ -153,6 +153,7 @@ static ssize_t fault_opcodes_write(struct file *file, const char __user *buf,
char *dash;
unsigned long range_start, range_end, i;
bool remove = false;
+ unsigned long bound = BIT(BITS_PER_BYTE);
end = strchr(ptr, ',');
if (end)
@@ -178,6 +179,10 @@ static ssize_t fault_opcodes_write(struct file *file, const char __user *buf,
BITS_PER_BYTE);
break;
}
+ /* Check the inputs */
+ if (range_start >= bound || range_end >= bound)
+ break;
+
for (i = range_start; i <= range_end; i++) {
if (remove)
clear_bit(i, fault->opcodes);
diff --git a/drivers/infiniband/hw/hfi1/fault.h b/drivers/infiniband/hw/hfi1/fault.h
index a833827..c61035c 100644
--- a/drivers/infiniband/hw/hfi1/fault.h
+++ b/drivers/infiniband/hw/hfi1/fault.h
@@ -60,13 +60,13 @@
struct fault {
struct fault_attr attr;
struct dentry *dir;
- u64 n_rxfaults[(1U << BITS_PER_BYTE)];
- u64 n_txfaults[(1U << BITS_PER_BYTE)];
+ u64 n_rxfaults[BIT(BITS_PER_BYTE)];
+ u64 n_txfaults[BIT(BITS_PER_BYTE)];
u64 fault_skip;
u64 skip;
u64 fault_skip_usec;
unsigned long skip_usec;
- unsigned long opcodes[(1U << BITS_PER_BYTE) / BITS_PER_LONG];
+ unsigned long opcodes[BIT(BITS_PER_BYTE) / BITS_PER_LONG];
bool enable;
bool suppress_err;
bool opcode;
^ permalink raw reply related
* [PATCH for-rc 2/3] IB/hfi1: Close PSM sdma_progress sleep window
From: Dennis Dalessandro @ 2019-06-07 12:25 UTC (permalink / raw)
To: jgg, dledford; +Cc: linux-rdma, Mike Marciniszyn, stable, Gary Leshner
In-Reply-To: <20190607113807.157915.48581.stgit@awfm-01.aw.intel.com>
From: Mike Marciniszyn <mike.marciniszyn@intel.com>
The call to sdma_progress() is called outside the wait lock.
In this case, there is a race condition where sdma_progress() can return
false and the sdma_engine can idle. If that happens, there will be no
more sdma interrupts to cause the wakeup and the user_sdma xmit will hang.
Fix by moving the lock to enclose the sdma_progress() call.
Also, delete busycount. The need for this was removed by:
commit bcad29137a97 ("IB/hfi1: Serve the most starved iowait entry first")
Cc: <stable@vger.kernel.org>
Fixes: 7724105686e7 ("IB/hfi1: add driver files")
Reviewed-by: Gary Leshner <Gary.S.Leshner@intel.com>
Signed-off-by: Mike Marciniszyn <mike.marciniszyn@intel.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
---
drivers/infiniband/hw/hfi1/user_sdma.c | 12 ++++--------
drivers/infiniband/hw/hfi1/user_sdma.h | 1 -
2 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/infiniband/hw/hfi1/user_sdma.c b/drivers/infiniband/hw/hfi1/user_sdma.c
index 8bfbc6d..fd754a1 100644
--- a/drivers/infiniband/hw/hfi1/user_sdma.c
+++ b/drivers/infiniband/hw/hfi1/user_sdma.c
@@ -130,20 +130,16 @@ static int defer_packet_queue(
{
struct hfi1_user_sdma_pkt_q *pq =
container_of(wait->iow, struct hfi1_user_sdma_pkt_q, busy);
- struct user_sdma_txreq *tx =
- container_of(txreq, struct user_sdma_txreq, txreq);
- if (sdma_progress(sde, seq, txreq)) {
- if (tx->busycount++ < MAX_DEFER_RETRY_COUNT)
- goto eagain;
- }
+ write_seqlock(&sde->waitlock);
+ if (sdma_progress(sde, seq, txreq))
+ goto eagain;
/*
* We are assuming that if the list is enqueued somewhere, it
* is to the dmawait list since that is the only place where
* it is supposed to be enqueued.
*/
xchg(&pq->state, SDMA_PKT_Q_DEFERRED);
- write_seqlock(&sde->waitlock);
if (list_empty(&pq->busy.list)) {
iowait_get_priority(&pq->busy);
iowait_queue(pkts_sent, &pq->busy, &sde->dmawait);
@@ -151,6 +147,7 @@ static int defer_packet_queue(
write_sequnlock(&sde->waitlock);
return -EBUSY;
eagain:
+ write_sequnlock(&sde->waitlock);
return -EAGAIN;
}
@@ -804,7 +801,6 @@ static int user_sdma_send_pkts(struct user_sdma_request *req, u16 maxpkts)
tx->flags = 0;
tx->req = req;
- tx->busycount = 0;
INIT_LIST_HEAD(&tx->list);
/*
diff --git a/drivers/infiniband/hw/hfi1/user_sdma.h b/drivers/infiniband/hw/hfi1/user_sdma.h
index 14dfd75..4d8510b0 100644
--- a/drivers/infiniband/hw/hfi1/user_sdma.h
+++ b/drivers/infiniband/hw/hfi1/user_sdma.h
@@ -245,7 +245,6 @@ struct user_sdma_txreq {
struct list_head list;
struct user_sdma_request *req;
u16 flags;
- unsigned int busycount;
u16 seqnum;
};
^ permalink raw reply related
* [PATCH for-rc 3/3] IB/hfi1: Correct tid qp rcd to match verbs context
From: Dennis Dalessandro @ 2019-06-07 12:25 UTC (permalink / raw)
To: jgg, dledford; +Cc: linux-rdma, Mike Marciniszyn, stable, Kaike Wan
In-Reply-To: <20190607113807.157915.48581.stgit@awfm-01.aw.intel.com>
From: Mike Marciniszyn <mike.marciniszyn@intel.com>
The qp priv rcd pointer doesn't match the context being
used for verbs causing issues when 9B and kdeth packets
are processed by different receive contexts and hence
different CPUs.
When running on different CPUs the following panic can
occur:
[476262.398106] WARNING: CPU: 3 PID: 2584 at lib/list_debug.c:59 __list_del_entry+0xa1/0xd0
[476262.398109] list_del corruption. prev->next should be ffff9a7ac31f7a30, but was ffff9a7c3bc89230
[476262.398266] CPU: 3 PID: 2584 Comm: z_wr_iss Kdump: loaded Tainted: P OE ------------ 3.10.0-862.2.3.el7_lustre.x86_64 #1
[476262.398272] Call Trace:
[476262.398277] <IRQ> [<ffffffffb7b0d78e>] dump_stack+0x19/0x1b
[476262.398314] [<ffffffffb74916d8>] __warn+0xd8/0x100
[476262.398317] [<ffffffffb749175f>] warn_slowpath_fmt+0x5f/0x80
[476262.398320] [<ffffffffb7768671>] __list_del_entry+0xa1/0xd0
[476262.398402] [<ffffffffc0c7a945>] process_rcv_qp_work+0xb5/0x160 [hfi1]
[476262.398424] [<ffffffffc0c7bc2b>] handle_receive_interrupt_nodma_rtail+0x20b/0x2b0 [hfi1]
[476262.398438] [<ffffffffc0c70683>] receive_context_interrupt+0x23/0x40 [hfi1]
[476262.398447] [<ffffffffb7540a94>] __handle_irq_event_percpu+0x44/0x1c0
[476262.398450] [<ffffffffb7540c42>] handle_irq_event_percpu+0x32/0x80
[476262.398454] [<ffffffffb7540ccc>] handle_irq_event+0x3c/0x60
[476262.398460] [<ffffffffb7543a1f>] handle_edge_irq+0x7f/0x150
[476262.398469] [<ffffffffb742d504>] handle_irq+0xe4/0x1a0
[476262.398475] [<ffffffffb7b23f7d>] do_IRQ+0x4d/0xf0
[476262.398481] [<ffffffffb7b16362>] common_interrupt+0x162/0x162
[476262.398482] <EOI> [<ffffffffb775a326>] ? memcpy+0x6/0x110
[476262.398645] [<ffffffffc109210d>] ? abd_copy_from_buf_off_cb+0x1d/0x30 [zfs]
[476262.398678] [<ffffffffc10920f0>] ? abd_copy_to_buf_off_cb+0x30/0x30 [zfs]
[476262.398696] [<ffffffffc1093257>] abd_iterate_func+0x97/0x120 [zfs]
[476262.398710] [<ffffffffc10934d9>] abd_copy_from_buf_off+0x39/0x60 [zfs]
[476262.398726] [<ffffffffc109b828>] arc_write_ready+0x178/0x300 [zfs]
[476262.398732] [<ffffffffb7b11032>] ? mutex_lock+0x12/0x2f
[476262.398734] [<ffffffffb7b11032>] ? mutex_lock+0x12/0x2f
[476262.398837] [<ffffffffc1164d05>] zio_ready+0x65/0x3d0 [zfs]
[476262.398884] [<ffffffffc04d725e>] ? tsd_get_by_thread+0x2e/0x50 [spl]
[476262.398893] [<ffffffffc04d1318>] ? taskq_member+0x18/0x30 [spl]
[476262.398968] [<ffffffffc115ef22>] zio_execute+0xa2/0x100 [zfs]
[476262.398982] [<ffffffffc04d1d2c>] taskq_thread+0x2ac/0x4f0 [spl]
[476262.399001] [<ffffffffb74cee80>] ? wake_up_state+0x20/0x20
[476262.399043] [<ffffffffc115ee80>] ? zio_taskq_member.isra.7.constprop.10+0x80/0x80 [zfs]
[476262.399055] [<ffffffffc04d1a80>] ? taskq_thread_spawn+0x60/0x60 [spl]
[476262.399067] [<ffffffffb74bae31>] kthread+0xd1/0xe0
[476262.399072] [<ffffffffb74bad60>] ? insert_kthread_work+0x40/0x40
[476262.399082] [<ffffffffb7b1f5f7>] ret_from_fork_nospec_begin+0x21/0x21
[476262.399087] [<ffffffffb74bad60>] ? insert_kthread_work+0x40/0x40
Fix by reading the map entry in the same manner as the
hardware so that the kdeth and verbs contexts match.
Fixes: 5190f052a365 ("IB/hfi1: Allow the driver to initialize QP priv struct")
Cc: <stable@vger.kernel.org>
Reviewed-by: Kaike Wan <kaike.wan@intel.com>
Signed-off-by: Mike Marciniszyn <mike.marciniszyn@intel.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
---
drivers/infiniband/hw/hfi1/chip.c | 13 +++++++++++++
drivers/infiniband/hw/hfi1/chip.h | 1 +
drivers/infiniband/hw/hfi1/tid_rdma.c | 5 ++---
3 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/drivers/infiniband/hw/hfi1/chip.c b/drivers/infiniband/hw/hfi1/chip.c
index 4221a99e..674f62a 100644
--- a/drivers/infiniband/hw/hfi1/chip.c
+++ b/drivers/infiniband/hw/hfi1/chip.c
@@ -14032,6 +14032,19 @@ static void init_kdeth_qp(struct hfi1_devdata *dd)
}
/**
+ * hfi1_get_qp_map
+ * @dd: device data
+ * @idx: index to read
+ */
+u8 hfi1_get_qp_map(struct hfi1_devdata *dd, u8 idx)
+{
+ u64 reg = read_csr(dd, RCV_QP_MAP_TABLE + (idx / 8) * 8);
+
+ reg >>= (idx % 8) * 8;
+ return (u8)reg;
+}
+
+/**
* init_qpmap_table
* @dd - device data
* @first_ctxt - first context
diff --git a/drivers/infiniband/hw/hfi1/chip.h b/drivers/infiniband/hw/hfi1/chip.h
index 4e6c355..b76cf81 100644
--- a/drivers/infiniband/hw/hfi1/chip.h
+++ b/drivers/infiniband/hw/hfi1/chip.h
@@ -1445,6 +1445,7 @@ int hfi1_set_ctxt_pkey(struct hfi1_devdata *dd, struct hfi1_ctxtdata *ctxt,
void remap_intr(struct hfi1_devdata *dd, int isrc, int msix_intr);
void remap_sdma_interrupts(struct hfi1_devdata *dd, int engine, int msix_intr);
void reset_interrupts(struct hfi1_devdata *dd);
+u8 hfi1_get_qp_map(struct hfi1_devdata *dd, u8 idx);
/*
* Interrupt source table.
diff --git a/drivers/infiniband/hw/hfi1/tid_rdma.c b/drivers/infiniband/hw/hfi1/tid_rdma.c
index 6fb9303..d77276d 100644
--- a/drivers/infiniband/hw/hfi1/tid_rdma.c
+++ b/drivers/infiniband/hw/hfi1/tid_rdma.c
@@ -312,9 +312,8 @@ static struct hfi1_ctxtdata *qp_to_rcd(struct rvt_dev_info *rdi,
if (qp->ibqp.qp_num == 0)
ctxt = 0;
else
- ctxt = ((qp->ibqp.qp_num >> dd->qos_shift) %
- (dd->n_krcv_queues - 1)) + 1;
-
+ ctxt = hfi1_get_qp_map(dd,
+ (u8)(qp->ibqp.qp_num >> dd->qos_shift));
return dd->rcd[ctxt];
}
^ permalink raw reply related
* Re: [PATCH v2 hmm 01/11] mm/hmm: fix use after free with struct hmm in the mmu notifiers
From: Jason Gunthorpe @ 2019-06-07 12:34 UTC (permalink / raw)
To: John Hubbard
Cc: Andrea Arcangeli, Ralph Campbell,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, Felix.Kuehling-5C7GfCeVMHo,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Jerome Glisse,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
In-Reply-To: <9c72d18d-2924-cb90-ea44-7cd4b10b5bc2-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
On Thu, Jun 06, 2019 at 07:29:08PM -0700, John Hubbard wrote:
> On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
> > From: Jason Gunthorpe <jgg@mellanox.com>
> ...
> > diff --git a/mm/hmm.c b/mm/hmm.c
> > index 8e7403f081f44a..547002f56a163d 100644
> > +++ b/mm/hmm.c
> ...
> > @@ -125,7 +130,7 @@ static void hmm_free(struct kref *kref)
> > mm->hmm = NULL;
> > spin_unlock(&mm->page_table_lock);
> >
> > - kfree(hmm);
> > + mmu_notifier_call_srcu(&hmm->rcu, hmm_free_rcu);
>
>
> It occurred to me to wonder if it is best to use the MMU notifier's
> instance of srcu, instead of creating a separate instance for HMM.
It *has* to be the MMU notifier SRCU because we are synchornizing
against the read side of that SRU inside the mmu notifier code, ie:
int __mmu_notifier_invalidate_range_start(struct mmu_notifier_range *range)
id = srcu_read_lock(&srcu);
hlist_for_each_entry_rcu(mn, &range->mm->mmu_notifier_mm->list, hlist) {
if (mn->ops->invalidate_range_start) {
^^^^^
Here 'mn' is really hmm (hmm = container_of(mn, struct hmm,
mmu_notifier)), so we must protect the memory against free for the mmu
notifier core.
Thus we have no choice but to use its SRCU.
CH also pointed out a more elegant solution, which is to get the write
side of the mmap_sem during hmm_mirror_unregister - no notifier
callback can be running in this case. Then we delete the kref, srcu
and so forth.
This is much clearer/saner/better, but.. requries the callers of
hmm_mirror_unregister to be safe to get the mmap_sem write side.
I think this is true, so maybe this patch should be switched, what do
you think?
> > @@ -153,10 +158,14 @@ void hmm_mm_destroy(struct mm_struct *mm)
> >
> > static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
> > {
> > - struct hmm *hmm = mm_get_hmm(mm);
> > + struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
> > struct hmm_mirror *mirror;
> > struct hmm_range *range;
> >
> > + /* hmm is in progress to free */
>
> Well, sometimes, yes. :)
It think it is in all cases actually.. The only way we see a 0 kref
and still reach this code path is if another thread has alreay setup
the hmm_free in the call_srcu..
> Maybe this wording is clearer (if we need any comment at all):
I always find this hard.. This is a very standard pattern when working
with RCU - however in my experience few people actually know the RCU
patterns, and missing the _unless_zero is a common bug I find when
looking at code.
This is mm/ so I can drop it, what do you think?
Thanks,
Jason
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply
* Re: [PATCH v2 hmm 03/11] mm/hmm: Hold a mmgrab from hmm to mm
From: Jason Gunthorpe @ 2019-06-07 12:36 UTC (permalink / raw)
To: John Hubbard
Cc: Andrea Arcangeli, Ralph Campbell,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, Felix.Kuehling-5C7GfCeVMHo,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Jerome Glisse,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
In-Reply-To: <48fcaa19-6ac3-59d0-cd51-455abeca7cdb-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
On Thu, Jun 06, 2019 at 07:44:58PM -0700, John Hubbard wrote:
> On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
> > From: Jason Gunthorpe <jgg@mellanox.com>
> >
> > So long a a struct hmm pointer exists, so should the struct mm it is
> > linked too. Hold the mmgrab() as soon as a hmm is created, and mmdrop() it
> > once the hmm refcount goes to zero.
> >
> > Since mmdrop() (ie a 0 kref on struct mm) is now impossible with a !NULL
> > mm->hmm delete the hmm_hmm_destroy().
> >
> > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> > Reviewed-by: Jérôme Glisse <jglisse@redhat.com>
> > v2:
> > - Fix error unwind paths in hmm_get_or_create (Jerome/Jason)
> > include/linux/hmm.h | 3 ---
> > kernel/fork.c | 1 -
> > mm/hmm.c | 22 ++++------------------
> > 3 files changed, 4 insertions(+), 22 deletions(-)
> >
> > diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> > index 2d519797cb134a..4ee3acabe5ed22 100644
> > +++ b/include/linux/hmm.h
> > @@ -586,14 +586,11 @@ static inline int hmm_vma_fault(struct hmm_mirror *mirror,
> > }
> >
> > /* Below are for HMM internal use only! Not to be used by device driver! */
> > -void hmm_mm_destroy(struct mm_struct *mm);
> > -
> > static inline void hmm_mm_init(struct mm_struct *mm)
> > {
> > mm->hmm = NULL;
> > }
> > #else /* IS_ENABLED(CONFIG_HMM_MIRROR) */
> > -static inline void hmm_mm_destroy(struct mm_struct *mm) {}
> > static inline void hmm_mm_init(struct mm_struct *mm) {}
> > #endif /* IS_ENABLED(CONFIG_HMM_MIRROR) */
> >
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index b2b87d450b80b5..588c768ae72451 100644
> > +++ b/kernel/fork.c
> > @@ -673,7 +673,6 @@ void __mmdrop(struct mm_struct *mm)
> > WARN_ON_ONCE(mm == current->active_mm);
> > mm_free_pgd(mm);
> > destroy_context(mm);
> > - hmm_mm_destroy(mm);
>
>
> This is particularly welcome, not to have an "HMM is special" case
> in such a core part of process/mm code.
I would very much like to propose something like 'per-net' for struct
mm, as rdma also need to add some data to each mm to make it's use of
mmu notifiers work (for basically this same reason as HMM)
Thanks,
Jason
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply
* Re: [PATCH v2 hmm 05/11] mm/hmm: Remove duplicate condition test before wait_event_timeout
From: Jason Gunthorpe @ 2019-06-07 12:47 UTC (permalink / raw)
To: John Hubbard
Cc: Andrea Arcangeli, Ralph Campbell,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, Felix.Kuehling-5C7GfCeVMHo,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Jerome Glisse,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
In-Reply-To: <86962e22-88b1-c1bf-d704-d5a5053fa100-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
On Thu, Jun 06, 2019 at 08:06:52PM -0700, John Hubbard wrote:
> On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
> > From: Jason Gunthorpe <jgg@mellanox.com>
> >
> > The wait_event_timeout macro already tests the condition as its first
> > action, so there is no reason to open code another version of this, all
> > that does is skip the might_sleep() debugging in common cases, which is
> > not helpful.
> >
> > Further, based on prior patches, we can no simplify the required condition
>
> "now simplify"
>
> > test:
> > - If range is valid memory then so is range->hmm
> > - If hmm_release() has run then range->valid is set to false
> > at the same time as dead, so no reason to check both.
> > - A valid hmm has a valid hmm->mm.
> >
> > Also, add the READ_ONCE for range->valid as there is no lock held here.
> >
> > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> > Reviewed-by: Jérôme Glisse <jglisse@redhat.com>
> > include/linux/hmm.h | 12 ++----------
> > 1 file changed, 2 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> > index 4ee3acabe5ed22..2ab35b40992b24 100644
> > +++ b/include/linux/hmm.h
> > @@ -218,17 +218,9 @@ static inline unsigned long hmm_range_page_size(const struct hmm_range *range)
> > static inline bool hmm_range_wait_until_valid(struct hmm_range *range,
> > unsigned long timeout)
> > {
> > - /* Check if mm is dead ? */
> > - if (range->hmm == NULL || range->hmm->dead || range->hmm->mm == NULL) {
> > - range->valid = false;
> > - return false;
> > - }
> > - if (range->valid)
> > - return true;
> > - wait_event_timeout(range->hmm->wq, range->valid || range->hmm->dead,
> > + wait_event_timeout(range->hmm->wq, range->valid,
> > msecs_to_jiffies(timeout));
> > - /* Return current valid status just in case we get lucky */
> > - return range->valid;
> > + return READ_ONCE(range->valid);
>
> Just to ensure that I actually understand the model: I'm assuming that the
> READ_ONCE is there solely to ensure that range->valid is read *after* the
> wait_event_timeout() returns. Is that correct?
No, wait_event_timout already has internal barriers that make sure
things don't leak across it.
The READ_ONCE is required any time a thread is reading a value that
another thread can be concurrently changing - ie in this case there is
no lock protecting range->valid so the write side could be running.
Without the READ_ONCE the compiler is allowed to read the value twice
and assume it gets the same result, which may not be true with a
parallel writer, and thus may compromise the control flow in some
unknown way.
It is also good documentation for the locking scheme in use as it
marks shared data that is not being locked.
However, now that dead is gone we can just write the above more simply
as:
static inline bool hmm_range_wait_until_valid(struct hmm_range *range,
unsigned long timeout)
{
return wait_event_timeout(range->hmm->wq, range->valid,
msecs_to_jiffies(timeout)) != 0;
}
Which relies on the internal barriers of wait_event_timeout, I'll fix
it up..
Thanks,
Jason
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply
* Re: [PATCH v2 hmm 11/11] mm/hmm: Remove confusing comment and logic from hmm_release
From: Jason Gunthorpe @ 2019-06-07 12:58 UTC (permalink / raw)
To: John Hubbard
Cc: Andrea Arcangeli, Ralph Campbell,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, Felix.Kuehling-5C7GfCeVMHo,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Jerome Glisse,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
In-Reply-To: <3edc47bd-e8f6-0e65-5844-d16901890637-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
On Thu, Jun 06, 2019 at 08:47:28PM -0700, John Hubbard wrote:
> On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
> > From: Jason Gunthorpe <jgg@mellanox.com>
> >
> > hmm_release() is called exactly once per hmm. ops->release() cannot
> > accidentally trigger any action that would recurse back onto
> > hmm->mirrors_sem.
> >
> > This fixes a use after-free race of the form:
> >
> > CPU0 CPU1
> > hmm_release()
> > up_write(&hmm->mirrors_sem);
> > hmm_mirror_unregister(mirror)
> > down_write(&hmm->mirrors_sem);
> > up_write(&hmm->mirrors_sem);
> > kfree(mirror)
> > mirror->ops->release(mirror)
> >
> > The only user we have today for ops->release is an empty function, so this
> > is unambiguously safe.
> >
> > As a consequence of plugging this race drivers are not allowed to
> > register/unregister mirrors from within a release op.
> >
> > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> > mm/hmm.c | 28 +++++++++-------------------
> > 1 file changed, 9 insertions(+), 19 deletions(-)
> >
> > diff --git a/mm/hmm.c b/mm/hmm.c
> > index 709d138dd49027..3a45dd3d778248 100644
> > +++ b/mm/hmm.c
> > @@ -136,26 +136,16 @@ static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
> > WARN_ON(!list_empty(&hmm->ranges));
> > mutex_unlock(&hmm->lock);
> >
> > - down_write(&hmm->mirrors_sem);
> > - mirror = list_first_entry_or_null(&hmm->mirrors, struct hmm_mirror,
> > - list);
> > - while (mirror) {
> > - list_del_init(&mirror->list);
> > - if (mirror->ops->release) {
> > - /*
> > - * Drop mirrors_sem so the release callback can wait
> > - * on any pending work that might itself trigger a
> > - * mmu_notifier callback and thus would deadlock with
> > - * us.
> > - */
> > - up_write(&hmm->mirrors_sem);
> > + down_read(&hmm->mirrors_sem);
>
> This is cleaner and simpler, but I suspect it is leading to the deadlock
> that Ralph Campbell is seeing in his driver testing. (And in general, holding
> a lock during a driver callback usually leads to deadlocks.)
I think Ralph has never seen this patch (it is new), so it must be one
of the earlier patches..
> Ralph, is this the one? It's the only place in this patchset where I can
> see a lock around a callback to driver code, that wasn't there before. So
> I'm pretty sure it is the one...
Can you share the lockdep report please?
Thanks,
Jason
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply
* [PATCH v3 hmm 05/11] mm/hmm: Remove duplicate condition test before wait_event_timeout
From: Jason Gunthorpe @ 2019-06-07 13:31 UTC (permalink / raw)
To: John Hubbard
Cc: Andrea Arcangeli, Ralph Campbell,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, Felix.Kuehling-5C7GfCeVMHo,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Jerome Glisse,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
In-Reply-To: <86962e22-88b1-c1bf-d704-d5a5053fa100-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
The wait_event_timeout macro already tests the condition as its first
action, so there is no reason to open code another version of this, all
that does is skip the might_sleep() debugging in common cases, which is
not helpful.
Further, based on prior patches, we can now simplify the required condition
test:
- If range is valid memory then so is range->hmm
- If hmm_release() has run then range->valid is set to false
at the same time as dead, so no reason to check both.
- A valid hmm has a valid hmm->mm.
Allowing the return value of wait_event_timeout() (along with its internal
barriers) to compute the result of the function.
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
---
v3
- Simplify the wait_event_timeout to not check valid
---
include/linux/hmm.h | 13 ++-----------
1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 1d97b6d62c5bcf..26e7c477490c4e 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -209,17 +209,8 @@ static inline unsigned long hmm_range_page_size(const struct hmm_range *range)
static inline bool hmm_range_wait_until_valid(struct hmm_range *range,
unsigned long timeout)
{
- /* Check if mm is dead ? */
- if (range->hmm == NULL || range->hmm->dead || range->hmm->mm == NULL) {
- range->valid = false;
- return false;
- }
- if (range->valid)
- return true;
- wait_event_timeout(range->hmm->wq, range->valid || range->hmm->dead,
- msecs_to_jiffies(timeout));
- /* Return current valid status just in case we get lucky */
- return range->valid;
+ return wait_event_timeout(range->hmm->wq, range->valid,
+ msecs_to_jiffies(timeout)) != 0;
}
/*
--
2.21.0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related
* Re: [PATCH v2 hmm 01/11] mm/hmm: fix use after free with struct hmm in the mmu notifiers
From: Jason Gunthorpe @ 2019-06-07 13:42 UTC (permalink / raw)
To: John Hubbard
Cc: Andrea Arcangeli, Ralph Campbell,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, Felix.Kuehling-5C7GfCeVMHo,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Jerome Glisse,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
In-Reply-To: <20190607123432.GB14802-uk2M96/98Pc@public.gmane.org>
On Fri, Jun 07, 2019 at 09:34:32AM -0300, Jason Gunthorpe wrote:
> CH also pointed out a more elegant solution, which is to get the write
> side of the mmap_sem during hmm_mirror_unregister - no notifier
> callback can be running in this case. Then we delete the kref, srcu
> and so forth.
Oops, it turns out this is only the case for invalidate_start/end, not
release, so this doesn't help with the SRCU unless we also change
exit_mmap to call release with the mmap sem held.
So I think we have to stick with this for now.
Jason
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply
* Re: [PATCH v2 hmm 08/11] mm/hmm: Remove racy protection against double-unregistration
From: Jason Gunthorpe @ 2019-06-07 13:57 UTC (permalink / raw)
To: John Hubbard
Cc: Andrea Arcangeli, Ralph Campbell,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, Felix.Kuehling-5C7GfCeVMHo,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Jerome Glisse,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
In-Reply-To: <88400de9-e1ae-509b-718f-c6b0f726b14c-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
On Thu, Jun 06, 2019 at 08:29:10PM -0700, John Hubbard wrote:
> On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
> > From: Jason Gunthorpe <jgg@mellanox.com>
> >
> > No other register/unregister kernel API attempts to provide this kind of
> > protection as it is inherently racy, so just drop it.
> >
> > Callers should provide their own protection, it appears nouveau already
> > does, but just in case drop a debugging POISON.
> >
> > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> > Reviewed-by: Jérôme Glisse <jglisse@redhat.com>
> > mm/hmm.c | 9 ++-------
> > 1 file changed, 2 insertions(+), 7 deletions(-)
> >
> > diff --git a/mm/hmm.c b/mm/hmm.c
> > index c702cd72651b53..6802de7080d172 100644
> > +++ b/mm/hmm.c
> > @@ -284,18 +284,13 @@ EXPORT_SYMBOL(hmm_mirror_register);
> > */
> > void hmm_mirror_unregister(struct hmm_mirror *mirror)
> > {
> > - struct hmm *hmm = READ_ONCE(mirror->hmm);
> > -
> > - if (hmm == NULL)
> > - return;
> > + struct hmm *hmm = mirror->hmm;
> >
> > down_write(&hmm->mirrors_sem);
> > list_del_init(&mirror->list);
> > - /* To protect us against double unregister ... */
> > - mirror->hmm = NULL;
> > up_write(&hmm->mirrors_sem);
> > -
> > hmm_put(hmm);
> > + memset(&mirror->hmm, POISON_INUSE, sizeof(mirror->hmm));
>
> I hadn't thought of POISON_* for these types of cases, it's a
> good technique to remember.
>
> I noticed that this is now done outside of the lock, but that
> follows directly from your commit description, so that all looks
> correct.
Yes, the thing about POISON is that if you ever read it then you have
found a use after free bug - thus we should never need to write it
under a lock (just after a serializing lock)
Normally I wouldn't bother as kfree does poison as well, but since we
can't easily audit the patches yet to be submitted this seems safer
and will reliably cause those patches to explode with an oops in
testing.
Thanks,
Jason
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply
* Re: [PATCH v2 hmm 09/11] mm/hmm: Poison hmm_range during unregister
From: Jason Gunthorpe @ 2019-06-07 14:03 UTC (permalink / raw)
To: John Hubbard
Cc: Andrea Arcangeli, Ralph Campbell,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, Felix.Kuehling-5C7GfCeVMHo,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Jerome Glisse,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
In-Reply-To: <c00da0f2-b4b8-813b-0441-a50d4de9d8be-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
On Thu, Jun 06, 2019 at 08:37:42PM -0700, John Hubbard wrote:
> On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
> > From: Jason Gunthorpe <jgg@mellanox.com>
> >
> > Trying to misuse a range outside its lifetime is a kernel bug. Use WARN_ON
> > and poison bytes to detect this condition.
> >
> > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> > Reviewed-by: Jérôme Glisse <jglisse@redhat.com>
> > v2
> > - Keep range start/end valid after unregistration (Jerome)
> > mm/hmm.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/hmm.c b/mm/hmm.c
> > index 6802de7080d172..c2fecb3ecb11e1 100644
> > +++ b/mm/hmm.c
> > @@ -937,7 +937,7 @@ void hmm_range_unregister(struct hmm_range *range)
> > struct hmm *hmm = range->hmm;
> >
> > /* Sanity check this really should not happen. */
>
> That comment can also be deleted, as it has the same meaning as
> the WARN_ON() that you just added.
>
> > - if (hmm == NULL || range->end <= range->start)
> > + if (WARN_ON(range->end <= range->start))
> > return;
> >
> > mutex_lock(&hmm->lock);
> > @@ -948,7 +948,10 @@ void hmm_range_unregister(struct hmm_range *range)
> > range->valid = false;
> > mmput(hmm->mm);
> > hmm_put(hmm);
> > - range->hmm = NULL;
> > +
> > + /* The range is now invalid, leave it poisoned. */
>
> To be precise, we are poisoning the range's back pointer to it's
> owning hmm instance. Maybe this is clearer:
>
> /*
> * The range is now invalid, so poison it's hmm pointer.
> * Leave other range-> fields in place, for the caller's use.
> */
>
> ...or something like that?
>
> > + range->valid = false;
> > + memset(&range->hmm, POISON_INUSE, sizeof(range->hmm));
> > }
> > EXPORT_SYMBOL(hmm_range_unregister);
> >
> >
>
> The above are very minor documentation points, so:
>
> Reviewed-by: John Hubbard <jhubbard@nvidia.com>
done thanks
Jason
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply
* [PATCH v2 12/11] mm/hmm: Fix error flows in hmm_invalidate_range_start
From: Jason Gunthorpe @ 2019-06-07 16:05 UTC (permalink / raw)
To: Jerome Glisse, Ralph Campbell, John Hubbard,
Felix.Kuehling-5C7GfCeVMHo
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Andrea Arcangeli,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
In-Reply-To: <20190606184438.31646-1-jgg-uk2M96/98Pc@public.gmane.org>
If the trylock on the hmm->mirrors_sem fails the function will return
without decrementing the notifiers that were previously incremented. Since
the caller will not call invalidate_range_end() on EAGAIN this will result
in notifiers becoming permanently incremented and deadlock.
If the sync_cpu_device_pagetables() required blocking the function will
not return EAGAIN even though the device continues to touch the
pages. This is a violation of the mmu notifier contract.
Switch, and rename, the ranges_lock to a spin lock so we can reliably
obtain it without blocking during error unwind.
The error unwind is necessary since the notifiers count must be held
incremented across the call to sync_cpu_device_pagetables() as we cannot
allow the range to become marked valid by a parallel
invalidate_start/end() pair while doing sync_cpu_device_pagetables().
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
---
include/linux/hmm.h | 2 +-
mm/hmm.c | 77 +++++++++++++++++++++++++++------------------
2 files changed, 48 insertions(+), 31 deletions(-)
I almost lost this patch - it is part of the series, hasn't been
posted before, and wasn't sent with the rest, sorry.
diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index bf013e96525771..0fa8ea34ccef6d 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -86,7 +86,7 @@
struct hmm {
struct mm_struct *mm;
struct kref kref;
- struct mutex lock;
+ spinlock_t ranges_lock;
struct list_head ranges;
struct list_head mirrors;
struct mmu_notifier mmu_notifier;
diff --git a/mm/hmm.c b/mm/hmm.c
index 4215edf737ef5b..10103a24e9b7b3 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -68,7 +68,7 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
init_rwsem(&hmm->mirrors_sem);
hmm->mmu_notifier.ops = NULL;
INIT_LIST_HEAD(&hmm->ranges);
- mutex_init(&hmm->lock);
+ spin_lock_init(&hmm->ranges_lock);
kref_init(&hmm->kref);
hmm->notifiers = 0;
hmm->mm = mm;
@@ -114,18 +114,19 @@ static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
{
struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
struct hmm_mirror *mirror;
+ unsigned long flags;
/* Bail out if hmm is in the process of being freed */
if (!kref_get_unless_zero(&hmm->kref))
return;
- mutex_lock(&hmm->lock);
+ spin_lock_irqsave(&hmm->ranges_lock, flags);
/*
* Since hmm_range_register() holds the mmget() lock hmm_release() is
* prevented as long as a range exists.
*/
WARN_ON(!list_empty(&hmm->ranges));
- mutex_unlock(&hmm->lock);
+ spin_unlock_irqrestore(&hmm->ranges_lock, flags);
down_read(&hmm->mirrors_sem);
list_for_each_entry(mirror, &hmm->mirrors, list) {
@@ -141,6 +142,23 @@ static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
hmm_put(hmm);
}
+static void notifiers_decrement(struct hmm *hmm)
+{
+ lockdep_assert_held(&hmm->ranges_lock);
+
+ hmm->notifiers--;
+ if (!hmm->notifiers) {
+ struct hmm_range *range;
+
+ list_for_each_entry(range, &hmm->ranges, list) {
+ if (range->valid)
+ continue;
+ range->valid = true;
+ }
+ wake_up_all(&hmm->wq);
+ }
+}
+
static int hmm_invalidate_range_start(struct mmu_notifier *mn,
const struct mmu_notifier_range *nrange)
{
@@ -148,6 +166,7 @@ static int hmm_invalidate_range_start(struct mmu_notifier *mn,
struct hmm_mirror *mirror;
struct hmm_update update;
struct hmm_range *range;
+ unsigned long flags;
int ret = 0;
if (!kref_get_unless_zero(&hmm->kref))
@@ -158,12 +177,7 @@ static int hmm_invalidate_range_start(struct mmu_notifier *mn,
update.event = HMM_UPDATE_INVALIDATE;
update.blockable = mmu_notifier_range_blockable(nrange);
- if (mmu_notifier_range_blockable(nrange))
- mutex_lock(&hmm->lock);
- else if (!mutex_trylock(&hmm->lock)) {
- ret = -EAGAIN;
- goto out;
- }
+ spin_lock_irqsave(&hmm->ranges_lock, flags);
hmm->notifiers++;
list_for_each_entry(range, &hmm->ranges, list) {
if (update.end < range->start || update.start >= range->end)
@@ -171,7 +185,7 @@ static int hmm_invalidate_range_start(struct mmu_notifier *mn,
range->valid = false;
}
- mutex_unlock(&hmm->lock);
+ spin_unlock_irqrestore(&hmm->ranges_lock, flags);
if (mmu_notifier_range_blockable(nrange))
down_read(&hmm->mirrors_sem);
@@ -179,16 +193,26 @@ static int hmm_invalidate_range_start(struct mmu_notifier *mn,
ret = -EAGAIN;
goto out;
}
+
list_for_each_entry(mirror, &hmm->mirrors, list) {
- int ret;
+ int rc;
- ret = mirror->ops->sync_cpu_device_pagetables(mirror, &update);
- if (!update.blockable && ret == -EAGAIN)
+ rc = mirror->ops->sync_cpu_device_pagetables(mirror, &update);
+ if (rc) {
+ if (WARN_ON(update.blockable || rc != -EAGAIN))
+ continue;
+ ret = -EAGAIN;
break;
+ }
}
up_read(&hmm->mirrors_sem);
out:
+ if (ret) {
+ spin_lock_irqsave(&hmm->ranges_lock, flags);
+ notifiers_decrement(hmm);
+ spin_unlock_irqrestore(&hmm->ranges_lock, flags);
+ }
hmm_put(hmm);
return ret;
}
@@ -197,23 +221,14 @@ static void hmm_invalidate_range_end(struct mmu_notifier *mn,
const struct mmu_notifier_range *nrange)
{
struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
+ unsigned long flags;
if (!kref_get_unless_zero(&hmm->kref))
return;
- mutex_lock(&hmm->lock);
- hmm->notifiers--;
- if (!hmm->notifiers) {
- struct hmm_range *range;
-
- list_for_each_entry(range, &hmm->ranges, list) {
- if (range->valid)
- continue;
- range->valid = true;
- }
- wake_up_all(&hmm->wq);
- }
- mutex_unlock(&hmm->lock);
+ spin_lock_irqsave(&hmm->ranges_lock, flags);
+ notifiers_decrement(hmm);
+ spin_unlock_irqrestore(&hmm->ranges_lock, flags);
hmm_put(hmm);
}
@@ -866,6 +881,7 @@ int hmm_range_register(struct hmm_range *range,
{
unsigned long mask = ((1UL << page_shift) - 1UL);
struct hmm *hmm = mirror->hmm;
+ unsigned long flags;
range->valid = false;
range->hmm = NULL;
@@ -887,7 +903,7 @@ int hmm_range_register(struct hmm_range *range,
kref_get(&hmm->kref);
/* Initialize range to track CPU page table updates. */
- mutex_lock(&hmm->lock);
+ spin_lock_irqsave(&hmm->ranges_lock, flags);
range->hmm = hmm;
list_add(&range->list, &hmm->ranges);
@@ -898,7 +914,7 @@ int hmm_range_register(struct hmm_range *range,
*/
if (!hmm->notifiers)
range->valid = true;
- mutex_unlock(&hmm->lock);
+ spin_unlock_irqrestore(&hmm->ranges_lock, flags);
return 0;
}
@@ -914,13 +930,14 @@ EXPORT_SYMBOL(hmm_range_register);
void hmm_range_unregister(struct hmm_range *range)
{
struct hmm *hmm = range->hmm;
+ unsigned long flags;
if (WARN_ON(range->end <= range->start))
return;
- mutex_lock(&hmm->lock);
+ spin_lock_irqsave(&hmm->ranges_lock, flags);
list_del(&range->list);
- mutex_unlock(&hmm->lock);
+ spin_unlock_irqrestore(&hmm->ranges_lock, flags);
/* Drop reference taken by hmm_range_register() */
range->valid = false;
--
2.21.0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related
* Re: [GIT PULL] Please pull RDMA subsystem changes
From: pr-tracker-bot @ 2019-06-07 16:45 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Linus Torvalds, Doug Ledford, linux-rdma@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20190606201420.GA9763@ziepe.ca>
The pull request you sent on Thu, 6 Jun 2019 20:14:24 +0000:
> git://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git tags/for-linus
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/6e38335dcc70f03faba26bf1260ee024d930afe1
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: [PATCH, RESEND] RDMA/srp: Accept again source addresses that do not have a port number
From: Jason Gunthorpe @ 2019-06-07 16:53 UTC (permalink / raw)
To: Bart Van Assche
Cc: Leon Romanovsky, Doug Ledford, linux-rdma, Laurence Oberman,
stable
In-Reply-To: <20190529163831.138926-1-bvanassche@acm.org>
On Wed, May 29, 2019 at 09:38:31AM -0700, Bart Van Assche wrote:
> The function srp_parse_in() is used both for parsing source address
> specifications and for target address specifications. Target addresses
> must have a port number. Having to specify a port number for source
> addresses is inconvenient. Make sure that srp_parse_in() supports again
> parsing addresses with no port number.
>
> Cc: Laurence Oberman <loberman@redhat.com>
> Cc: <stable@vger.kernel.org>
> Fixes: c62adb7def71 ("IB/srp: Fix IPv6 address parsing") # v4.17.
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> drivers/infiniband/ulp/srp/ib_srp.c | 21 +++++++++++++++------
> 1 file changed, 15 insertions(+), 6 deletions(-)
Applied to for-next, thanks
Jason
^ permalink raw reply
* Re: properly communicate queue limits to the DMA layer
From: Martin K. Petersen @ 2019-06-07 17:30 UTC (permalink / raw)
To: Jens Axboe
Cc: Christoph Hellwig, Sebastian Ott, Sagi Grimberg, Max Gurtovoy,
Bart Van Assche, Ulf Hansson, Alan Stern, Oliver Neukum,
linux-block, linux-rdma, linux-mmc, linux-nvme, linux-scsi,
megaraidlinux.pdl, MPT-FusionLinux.pdl, linux-hyperv, linux-usb,
usb-storage, linux-kernel
In-Reply-To: <f07d0abf-b3eb-f530-37b9-e66454740b3f@kernel.dk>
Jens,
>> The SCSI bits will need a bit more review, and possibly tweaking
>> fo megaraid and mpt3sas. But they are really independent of the
>> other patches, so maybe skip them for now and leave them for Martin
>> to deal with.
>
> I dropped the SCSI bits.
I'll monitor and merge them.
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply
* Re: [PATCH 2/2][next] RDMA/hns: fix inverted logic of readl read and shift
From: Jason Gunthorpe @ 2019-06-07 17:57 UTC (permalink / raw)
To: Colin King
Cc: Lijun Ou, Wei Hu, Doug Ledford, linux-rdma, kernel-janitors,
linux-kernel
In-Reply-To: <20190531092101.28772-2-colin.king@canonical.com>
On Fri, May 31, 2019 at 10:21:01AM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> A previous change incorrectly changed the inverted logic and logically
> negated the readl rather than the shifted readl result. Fix this by
> adding in missing parentheses around the expression that needs to be
> logically negated.
>
> Addresses-Coverity: ("Logically dead code")
> Fixes: 669cefb654cb ("RDMA/hns: Remove jiffies operation in disable interrupt context")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/infiniband/hw/hns/hns_roce_hem.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Applied to for-next, thanks
Jason
^ permalink raw reply
* Re: [PATCH 1/2][next] RDMA/hns: fix comparison of unsigned long variable 'end' with less than zero
From: Jason Gunthorpe @ 2019-06-07 17:57 UTC (permalink / raw)
To: Colin King
Cc: Lijun Ou, Wei Hu, Doug Ledford, linux-rdma, kernel-janitors,
linux-kernel
In-Reply-To: <20190531092101.28772-1-colin.king@canonical.com>
On Fri, May 31, 2019 at 10:21:00AM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Currently the comparison of end with less than zero is always false
> because end is an unsigned long. Also, replace checks of end with
> non-zero with end > 0 as it is possible that the #defined decrement
> may be changed in the future causing end to step over zero and go
> negative.
>
> The initialization of end with 0 is also redundant as this value is
> never read and is later set to HW_SYNC_TIMEOUT_MSECS, so fix this by
> initializing it with this value to begin with.
>
> Addresses-Coverity: ("Unsigned compared against 0")
> Fixes: 669cefb654cb ("RDMA/hns: Remove jiffies operation in disable interrupt context")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/infiniband/hw/hns/hns_roce_hem.c | 4 ++--
> drivers/infiniband/hw/hns/hns_roce_hw_v1.c | 12 ++++++------
> 2 files changed, 8 insertions(+), 8 deletions(-)
hns team: Can you please comment on this patch
Jason
^ permalink raw reply
* Re: [PATCH] RDMA/ucma: Use struct_size() helper
From: Jason Gunthorpe @ 2019-06-07 18:05 UTC (permalink / raw)
To: Gustavo A. R. Silva; +Cc: Doug Ledford, linux-rdma, linux-kernel
In-Reply-To: <20190604154222.GA8938@embeddedor>
On Tue, Jun 04, 2019 at 10:42:22AM -0500, Gustavo A. R. Silva wrote:
> Make use of the struct_size() helper instead of an open-coded version
> in order to avoid any potential type mistakes, in particular in the
> context in which this code is being used.
>
> So, replace the following form:
>
> sizeof(*resp) + (i * sizeof(struct ib_path_rec_data))
>
> with:
>
> struct_size(resp, path_data, i)
>
> This code was detected with the help of Coccinelle.
>
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
> drivers/infiniband/core/ucma.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
Applied to for-next, thanks
Jason
^ 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