* [PATCH v3 hmm 00/12] mm/hmm: Various revisions from a locking/code review
From: Jason Gunthorpe @ 2019-06-14 0:44 UTC (permalink / raw)
To: Jerome Glisse, Ralph Campbell, John Hubbard, Felix.Kuehling
Cc: Andrea Arcangeli, linux-rdma, amd-gfx, linux-mm, Jason Gunthorpe,
dri-devel, Ben Skeggs
From: Jason Gunthorpe <jgg@mellanox.com>
This patch series arised out of discussions with Jerome when looking at the
ODP changes, particularly informed by use after free races we have already
found and fixed in the ODP code (thanks to syzkaller) working with mmu
notifiers, and the discussion with Ralph on how to resolve the lifetime model.
Overall this brings in a simplified locking scheme and easy to explain
lifetime model:
If a hmm_range is valid, then the hmm is valid, if a hmm is valid then the mm
is allocated memory.
If the mm needs to still be alive (ie to lock the mmap_sem, find a vma, etc)
then the mmget must be obtained via mmget_not_zero().
The use unlocked reads on 'hmm->dead' are also eliminated in favour of using
standard mmget() locking to prevent the mm from being released. Many of the
debugging checks of !range->hmm and !hmm->mm are dropped in favour of poison -
which is much clearer as to the lifetime intent.
The trailing patches are just some random cleanups I noticed when reviewing
this code.
I would like to run some testing with the ODP patch, but haven't
yet. Otherwise I think this is reviewed enough, and if there is nothing more
say I hope to apply it next week.
I plan to continue to work on the idea with CH to move more of this mirror
code into mmu notifiers and other places, but this will take some time and
research.
Thanks to everyone who took time to look at this!
Jason Gunthorpe (12):
mm/hmm: fix use after free with struct hmm in the mmu notifiers
mm/hmm: Use hmm_mirror not mm as an argument for hmm_range_register
mm/hmm: Hold a mmgrab from hmm to mm
mm/hmm: Simplify hmm_get_or_create and make it reliable
mm/hmm: Remove duplicate condition test before wait_event_timeout
mm/hmm: Hold on to the mmget for the lifetime of the range
mm/hmm: Use lockdep instead of comments
mm/hmm: Remove racy protection against double-unregistration
mm/hmm: Poison hmm_range during unregister
mm/hmm: Do not use list*_rcu() for hmm->ranges
mm/hmm: Remove confusing comment and logic from hmm_release
mm/hmm: Fix error flows in hmm_invalidate_range_start
drivers/gpu/drm/nouveau/nouveau_svm.c | 2 +-
include/linux/hmm.h | 52 +----
kernel/fork.c | 1 -
mm/hmm.c | 286 ++++++++++++--------------
4 files changed, 140 insertions(+), 201 deletions(-)
--
2.21.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* [PATCH v3 hmm 01/12] mm/hmm: fix use after free with struct hmm in the mmu notifiers
From: Jason Gunthorpe @ 2019-06-14 0:44 UTC (permalink / raw)
To: Jerome Glisse, Ralph Campbell, John Hubbard, Felix.Kuehling
Cc: Andrea Arcangeli, Philip Yang, linux-rdma, amd-gfx, linux-mm,
Jason Gunthorpe, dri-devel, Ira Weiny, Ben Skeggs
In-Reply-To: <20190614004450.20252-1-jgg@ziepe.ca>
From: Jason Gunthorpe <jgg@mellanox.com>
mmu_notifier_unregister_no_release() is not a fence and the mmu_notifier
system will continue to reference hmm->mn until the srcu grace period
expires.
Resulting in use after free races like this:
CPU0 CPU1
__mmu_notifier_invalidate_range_start()
srcu_read_lock
hlist_for_each ()
// mn == hmm->mn
hmm_mirror_unregister()
hmm_put()
hmm_free()
mmu_notifier_unregister_no_release()
hlist_del_init_rcu(hmm-mn->list)
mn->ops->invalidate_range_start(mn, range);
mm_get_hmm()
mm->hmm = NULL;
kfree(hmm)
mutex_lock(&hmm->lock);
Use SRCU to kfree the hmm memory so that the notifiers can rely on hmm
existing. Get the now-safe hmm struct through container_of and directly
check kref_get_unless_zero to lock it against free.
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>
Tested-by: Philip Yang <Philip.Yang@amd.com>
---
v2:
- Spell 'free' properly (Jerome/Ralph)
v3:
- Have only one clearer comment about kref_get_unless_zero (John)
---
include/linux/hmm.h | 1 +
mm/hmm.c | 23 +++++++++++++++++------
2 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 7007123842ba76..cb01cf1fa3c08b 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -93,6 +93,7 @@ struct hmm {
struct mmu_notifier mmu_notifier;
struct rw_semaphore mirrors_sem;
wait_queue_head_t wq;
+ struct rcu_head rcu;
long notifiers;
bool dead;
};
diff --git a/mm/hmm.c b/mm/hmm.c
index 826816ab237799..f6956d78e3cb25 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -104,6 +104,11 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
return NULL;
}
+static void hmm_free_rcu(struct rcu_head *rcu)
+{
+ kfree(container_of(rcu, struct hmm, rcu));
+}
+
static void hmm_free(struct kref *kref)
{
struct hmm *hmm = container_of(kref, struct hmm, kref);
@@ -116,7 +121,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);
}
static inline void hmm_put(struct hmm *hmm)
@@ -144,10 +149,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;
+ /* Bail out if hmm is in the process of being freed */
+ if (!kref_get_unless_zero(&hmm->kref))
+ return;
+
/* Report this HMM as dying. */
hmm->dead = true;
@@ -185,13 +194,14 @@ static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
static int hmm_invalidate_range_start(struct mmu_notifier *mn,
const struct mmu_notifier_range *nrange)
{
- struct hmm *hmm = mm_get_hmm(nrange->mm);
+ struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
struct hmm_mirror *mirror;
struct hmm_update update;
struct hmm_range *range;
int ret = 0;
- VM_BUG_ON(!hmm);
+ if (!kref_get_unless_zero(&hmm->kref))
+ return 0;
update.start = nrange->start;
update.end = nrange->end;
@@ -236,9 +246,10 @@ static int hmm_invalidate_range_start(struct mmu_notifier *mn,
static void hmm_invalidate_range_end(struct mmu_notifier *mn,
const struct mmu_notifier_range *nrange)
{
- struct hmm *hmm = mm_get_hmm(nrange->mm);
+ struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
- VM_BUG_ON(!hmm);
+ if (!kref_get_unless_zero(&hmm->kref))
+ return;
mutex_lock(&hmm->lock);
hmm->notifiers--;
--
2.21.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* [PATCH v3 hmm 02/12] mm/hmm: Use hmm_mirror not mm as an argument for hmm_range_register
From: Jason Gunthorpe @ 2019-06-14 0:44 UTC (permalink / raw)
To: Jerome Glisse, Ralph Campbell, John Hubbard, Felix.Kuehling
Cc: Andrea Arcangeli, Philip Yang, linux-rdma, amd-gfx, linux-mm,
Jason Gunthorpe, dri-devel, Ira Weiny, Ben Skeggs
In-Reply-To: <20190614004450.20252-1-jgg@ziepe.ca>
From: Jason Gunthorpe <jgg@mellanox.com>
Ralph observes that hmm_range_register() can only be called by a driver
while a mirror is registered. Make this clear in the API by passing in the
mirror structure as a parameter.
This also simplifies understanding the lifetime model for struct hmm, as
the hmm pointer must be valid as part of a registered mirror so all we
need in hmm_register_range() is a simple kref_get.
Suggested-by: Ralph Campbell <rcampbell@nvidia.com>
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Tested-by: Philip Yang <Philip.Yang@amd.com>
---
v2
- Include the oneline patch to nouveau_svm.c
---
drivers/gpu/drm/nouveau/nouveau_svm.c | 2 +-
include/linux/hmm.h | 7 ++++---
mm/hmm.c | 13 ++++---------
3 files changed, 9 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c
index 93ed43c413f0bb..8c92374afcf227 100644
--- a/drivers/gpu/drm/nouveau/nouveau_svm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
@@ -649,7 +649,7 @@ nouveau_svm_fault(struct nvif_notify *notify)
range.values = nouveau_svm_pfn_values;
range.pfn_shift = NVIF_VMM_PFNMAP_V0_ADDR_SHIFT;
again:
- ret = hmm_vma_fault(&range, true);
+ ret = hmm_vma_fault(&svmm->mirror, &range, true);
if (ret == 0) {
mutex_lock(&svmm->mutex);
if (!hmm_vma_range_done(&range)) {
diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index cb01cf1fa3c08b..1fba6979adf460 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -496,7 +496,7 @@ static inline bool hmm_mirror_mm_is_alive(struct hmm_mirror *mirror)
* Please see Documentation/vm/hmm.rst for how to use the range API.
*/
int hmm_range_register(struct hmm_range *range,
- struct mm_struct *mm,
+ struct hmm_mirror *mirror,
unsigned long start,
unsigned long end,
unsigned page_shift);
@@ -532,7 +532,8 @@ static inline bool hmm_vma_range_done(struct hmm_range *range)
}
/* This is a temporary helper to avoid merge conflict between trees. */
-static inline int hmm_vma_fault(struct hmm_range *range, bool block)
+static inline int hmm_vma_fault(struct hmm_mirror *mirror,
+ struct hmm_range *range, bool block)
{
long ret;
@@ -545,7 +546,7 @@ static inline int hmm_vma_fault(struct hmm_range *range, bool block)
range->default_flags = 0;
range->pfn_flags_mask = -1UL;
- ret = hmm_range_register(range, range->vma->vm_mm,
+ ret = hmm_range_register(range, mirror,
range->start, range->end,
PAGE_SHIFT);
if (ret)
diff --git a/mm/hmm.c b/mm/hmm.c
index f6956d78e3cb25..22a97ada108b4e 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -914,13 +914,13 @@ static void hmm_pfns_clear(struct hmm_range *range,
* Track updates to the CPU page table see include/linux/hmm.h
*/
int hmm_range_register(struct hmm_range *range,
- struct mm_struct *mm,
+ struct hmm_mirror *mirror,
unsigned long start,
unsigned long end,
unsigned page_shift)
{
unsigned long mask = ((1UL << page_shift) - 1UL);
- struct hmm *hmm;
+ struct hmm *hmm = mirror->hmm;
range->valid = false;
range->hmm = NULL;
@@ -934,20 +934,15 @@ int hmm_range_register(struct hmm_range *range,
range->start = start;
range->end = end;
- hmm = hmm_get_or_create(mm);
- if (!hmm)
- return -EFAULT;
-
/* Check if hmm_mm_destroy() was call. */
- if (hmm->mm == NULL || hmm->dead) {
- hmm_put(hmm);
+ if (hmm->mm == NULL || hmm->dead)
return -EFAULT;
- }
/* Initialize range to track CPU page table updates. */
mutex_lock(&hmm->lock);
range->hmm = hmm;
+ kref_get(&hmm->kref);
list_add_rcu(&range->list, &hmm->ranges);
/*
--
2.21.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* [PATCH v3 hmm 03/12] mm/hmm: Hold a mmgrab from hmm to mm
From: Jason Gunthorpe @ 2019-06-14 0:44 UTC (permalink / raw)
To: Jerome Glisse, Ralph Campbell, John Hubbard, Felix.Kuehling
Cc: Andrea Arcangeli, Philip Yang, linux-rdma, amd-gfx, linux-mm,
Jason Gunthorpe, dri-devel, Ira Weiny, Ben Skeggs
In-Reply-To: <20190614004450.20252-1-jgg@ziepe.ca>
From: Jason Gunthorpe <jgg@mellanox.com>
So long as 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>
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Tested-by: Philip Yang <Philip.Yang@amd.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 1fba6979adf460..1d97b6d62c5bcf 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -577,14 +577,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 75675b9bf6dfd3..c704c3cedee78d 100644
--- a/kernel/fork.c
+++ 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);
mmu_notifier_mm_destroy(mm);
check_mm(mm);
put_user_ns(mm->user_ns);
diff --git a/mm/hmm.c b/mm/hmm.c
index 22a97ada108b4e..080b17a2e87e2d 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -20,6 +20,7 @@
#include <linux/swapops.h>
#include <linux/hugetlb.h>
#include <linux/memremap.h>
+#include <linux/sched/mm.h>
#include <linux/jump_label.h>
#include <linux/dma-mapping.h>
#include <linux/mmu_notifier.h>
@@ -73,6 +74,7 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
hmm->notifiers = 0;
hmm->dead = false;
hmm->mm = mm;
+ mmgrab(hmm->mm);
spin_lock(&mm->page_table_lock);
if (!mm->hmm)
@@ -100,6 +102,7 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
mm->hmm = NULL;
spin_unlock(&mm->page_table_lock);
error:
+ mmdrop(hmm->mm);
kfree(hmm);
return NULL;
}
@@ -121,6 +124,7 @@ static void hmm_free(struct kref *kref)
mm->hmm = NULL;
spin_unlock(&mm->page_table_lock);
+ mmdrop(hmm->mm);
mmu_notifier_call_srcu(&hmm->rcu, hmm_free_rcu);
}
@@ -129,24 +133,6 @@ static inline void hmm_put(struct hmm *hmm)
kref_put(&hmm->kref, hmm_free);
}
-void hmm_mm_destroy(struct mm_struct *mm)
-{
- struct hmm *hmm;
-
- spin_lock(&mm->page_table_lock);
- hmm = mm_get_hmm(mm);
- mm->hmm = NULL;
- if (hmm) {
- hmm->mm = NULL;
- hmm->dead = true;
- spin_unlock(&mm->page_table_lock);
- hmm_put(hmm);
- return;
- }
-
- spin_unlock(&mm->page_table_lock);
-}
-
static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
{
struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
--
2.21.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* [PATCH v3 hmm 04/12] mm/hmm: Simplify hmm_get_or_create and make it reliable
From: Jason Gunthorpe @ 2019-06-14 0:44 UTC (permalink / raw)
To: Jerome Glisse, Ralph Campbell, John Hubbard, Felix.Kuehling
Cc: Andrea Arcangeli, Philip Yang, linux-rdma, amd-gfx, linux-mm,
Jason Gunthorpe, dri-devel, Ira Weiny, Ben Skeggs
In-Reply-To: <20190614004450.20252-1-jgg@ziepe.ca>
From: Jason Gunthorpe <jgg@mellanox.com>
As coded this function can false-fail in various racy situations. Make it
reliable and simpler by running under the write side of the mmap_sem and
avoiding the false-failing compare/exchange pattern. Due to the mmap_sem
this no longer has to avoid racing with a 2nd parallel
hmm_get_or_create().
Unfortunately this still has to use the page_table_lock as the
non-sleeping lock protecting mm->hmm, since the contexts where we free the
hmm are incompatible with mmap_sem.
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Tested-by: Philip Yang <Philip.Yang@amd.com>
---
v2:
- Fix error unwind of mmgrab (Jerome)
- Use hmm local instead of 2nd container_of (Jerome)
v3:
- Can't use mmap_sem in the SRCU callback, keep using the
page_table_lock (Philip)
---
mm/hmm.c | 84 ++++++++++++++++++++++++--------------------------------
1 file changed, 36 insertions(+), 48 deletions(-)
diff --git a/mm/hmm.c b/mm/hmm.c
index 080b17a2e87e2d..4c64d4c32f4825 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -31,16 +31,6 @@
#if IS_ENABLED(CONFIG_HMM_MIRROR)
static const struct mmu_notifier_ops hmm_mmu_notifier_ops;
-static inline struct hmm *mm_get_hmm(struct mm_struct *mm)
-{
- struct hmm *hmm = READ_ONCE(mm->hmm);
-
- if (hmm && kref_get_unless_zero(&hmm->kref))
- return hmm;
-
- return NULL;
-}
-
/**
* hmm_get_or_create - register HMM against an mm (HMM internal)
*
@@ -55,11 +45,19 @@ static inline struct hmm *mm_get_hmm(struct mm_struct *mm)
*/
static struct hmm *hmm_get_or_create(struct mm_struct *mm)
{
- struct hmm *hmm = mm_get_hmm(mm);
- bool cleanup = false;
+ struct hmm *hmm;
- if (hmm)
- return hmm;
+ lockdep_assert_held_exclusive(&mm->mmap_sem);
+
+ /* Abuse the page_table_lock to also protect mm->hmm. */
+ spin_lock(&mm->page_table_lock);
+ if (mm->hmm) {
+ if (kref_get_unless_zero(&mm->hmm->kref)) {
+ spin_unlock(&mm->page_table_lock);
+ return mm->hmm;
+ }
+ }
+ spin_unlock(&mm->page_table_lock);
hmm = kmalloc(sizeof(*hmm), GFP_KERNEL);
if (!hmm)
@@ -74,57 +72,47 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
hmm->notifiers = 0;
hmm->dead = false;
hmm->mm = mm;
- mmgrab(hmm->mm);
- spin_lock(&mm->page_table_lock);
- if (!mm->hmm)
- mm->hmm = hmm;
- else
- cleanup = true;
- spin_unlock(&mm->page_table_lock);
+ hmm->mmu_notifier.ops = &hmm_mmu_notifier_ops;
+ if (__mmu_notifier_register(&hmm->mmu_notifier, mm)) {
+ kfree(hmm);
+ return NULL;
+ }
- if (cleanup)
- goto error;
+ mmgrab(hmm->mm);
/*
- * We should only get here if hold the mmap_sem in write mode ie on
- * registration of first mirror through hmm_mirror_register()
+ * We hold the exclusive mmap_sem here so we know that mm->hmm is
+ * still NULL or 0 kref, and is safe to update.
*/
- hmm->mmu_notifier.ops = &hmm_mmu_notifier_ops;
- if (__mmu_notifier_register(&hmm->mmu_notifier, mm))
- goto error_mm;
-
- return hmm;
-
-error_mm:
spin_lock(&mm->page_table_lock);
- if (mm->hmm == hmm)
- mm->hmm = NULL;
+ mm->hmm = hmm;
spin_unlock(&mm->page_table_lock);
-error:
- mmdrop(hmm->mm);
- kfree(hmm);
- return NULL;
+ return hmm;
}
static void hmm_free_rcu(struct rcu_head *rcu)
{
- kfree(container_of(rcu, struct hmm, rcu));
+ struct hmm *hmm = container_of(rcu, struct hmm, rcu);
+
+ /*
+ * The mm->hmm pointer is kept valid while notifier ops can be running
+ * so they don't have to deal with a NULL mm->hmm value
+ */
+ spin_lock(&hmm->mm->page_table_lock);
+ if (hmm->mm->hmm == hmm)
+ hmm->mm->hmm = NULL;
+ spin_unlock(&hmm->mm->page_table_lock);
+ mmdrop(hmm->mm);
+
+ kfree(hmm);
}
static void hmm_free(struct kref *kref)
{
struct hmm *hmm = container_of(kref, struct hmm, kref);
- struct mm_struct *mm = hmm->mm;
-
- mmu_notifier_unregister_no_release(&hmm->mmu_notifier, mm);
- spin_lock(&mm->page_table_lock);
- if (mm->hmm == hmm)
- mm->hmm = NULL;
- spin_unlock(&mm->page_table_lock);
-
- mmdrop(hmm->mm);
+ mmu_notifier_unregister_no_release(&hmm->mmu_notifier, hmm->mm);
mmu_notifier_call_srcu(&hmm->rcu, hmm_free_rcu);
}
--
2.21.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* [PATCH v3 hmm 05/12] mm/hmm: Remove duplicate condition test before wait_event_timeout
From: Jason Gunthorpe @ 2019-06-14 0:44 UTC (permalink / raw)
To: Jerome Glisse, Ralph Campbell, John Hubbard, Felix.Kuehling
Cc: Andrea Arcangeli, Philip Yang, linux-rdma, amd-gfx, linux-mm,
Jason Gunthorpe, dri-devel, Ira Weiny, Ben Skeggs
In-Reply-To: <20190614004450.20252-1-jgg@ziepe.ca>
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 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>
Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Tested-by: Philip Yang <Philip.Yang@amd.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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* [PATCH v3 hmm 06/12] mm/hmm: Hold on to the mmget for the lifetime of the range
From: Jason Gunthorpe @ 2019-06-14 0:44 UTC (permalink / raw)
To: Jerome Glisse, Ralph Campbell, John Hubbard, Felix.Kuehling
Cc: Andrea Arcangeli, Philip Yang, linux-rdma, amd-gfx, linux-mm,
Jason Gunthorpe, dri-devel, Ben Skeggs
In-Reply-To: <20190614004450.20252-1-jgg@ziepe.ca>
From: Jason Gunthorpe <jgg@mellanox.com>
Range functions like hmm_range_snapshot() and hmm_range_fault() call
find_vma, which requires hodling the mmget() and the mmap_sem for the mm.
Make this simpler for the callers by holding the mmget() inside the range
for the lifetime of the range. Other functions that accept a range should
only be called if the range is registered.
This has the side effect of directly preventing hmm_release() from
happening while a range is registered. That means range->dead cannot be
false during the lifetime of the range, so remove dead and
hmm_mirror_mm_is_alive() entirely.
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>
Tested-by: Philip Yang <Philip.Yang@amd.com>
---
v2:
- Use Jerome's idea of just holding the mmget() for the range lifetime,
rework the patch to use that as as simplification to remove dead in
one step
---
include/linux/hmm.h | 26 --------------------------
mm/hmm.c | 28 ++++++++++------------------
2 files changed, 10 insertions(+), 44 deletions(-)
diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 26e7c477490c4e..bf013e96525771 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -82,7 +82,6 @@
* @mirrors_sem: read/write semaphore protecting the mirrors list
* @wq: wait queue for user waiting on a range invalidation
* @notifiers: count of active mmu notifiers
- * @dead: is the mm dead ?
*/
struct hmm {
struct mm_struct *mm;
@@ -95,7 +94,6 @@ struct hmm {
wait_queue_head_t wq;
struct rcu_head rcu;
long notifiers;
- bool dead;
};
/*
@@ -459,30 +457,6 @@ struct hmm_mirror {
int hmm_mirror_register(struct hmm_mirror *mirror, struct mm_struct *mm);
void hmm_mirror_unregister(struct hmm_mirror *mirror);
-/*
- * hmm_mirror_mm_is_alive() - test if mm is still alive
- * @mirror: the HMM mm mirror for which we want to lock the mmap_sem
- * Return: false if the mm is dead, true otherwise
- *
- * This is an optimization, it will not always accurately return false if the
- * mm is dead; i.e., there can be false negatives (process is being killed but
- * HMM is not yet informed of that). It is only intended to be used to optimize
- * out cases where the driver is about to do something time consuming and it
- * would be better to skip it if the mm is dead.
- */
-static inline bool hmm_mirror_mm_is_alive(struct hmm_mirror *mirror)
-{
- struct mm_struct *mm;
-
- if (!mirror || !mirror->hmm)
- return false;
- mm = READ_ONCE(mirror->hmm->mm);
- if (mirror->hmm->dead || !mm)
- return false;
-
- return true;
-}
-
/*
* Please see Documentation/vm/hmm.rst for how to use the range API.
*/
diff --git a/mm/hmm.c b/mm/hmm.c
index 4c64d4c32f4825..58712d74edd585 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -70,7 +70,6 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
mutex_init(&hmm->lock);
kref_init(&hmm->kref);
hmm->notifiers = 0;
- hmm->dead = false;
hmm->mm = mm;
hmm->mmu_notifier.ops = &hmm_mmu_notifier_ops;
@@ -125,20 +124,17 @@ 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;
- struct hmm_range *range;
/* Bail out if hmm is in the process of being freed */
if (!kref_get_unless_zero(&hmm->kref))
return;
- /* Report this HMM as dying. */
- hmm->dead = true;
-
- /* Wake-up everyone waiting on any range. */
mutex_lock(&hmm->lock);
- list_for_each_entry(range, &hmm->ranges, list)
- range->valid = false;
- wake_up_all(&hmm->wq);
+ /*
+ * 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);
down_write(&hmm->mirrors_sem);
@@ -908,8 +904,8 @@ int hmm_range_register(struct hmm_range *range,
range->start = start;
range->end = end;
- /* Check if hmm_mm_destroy() was call. */
- if (hmm->mm == NULL || hmm->dead)
+ /* Prevent hmm_release() from running while the range is valid */
+ if (!mmget_not_zero(hmm->mm))
return -EFAULT;
/* Initialize range to track CPU page table updates. */
@@ -952,6 +948,7 @@ void hmm_range_unregister(struct hmm_range *range)
/* Drop reference taken by hmm_range_register() */
range->valid = false;
+ mmput(hmm->mm);
hmm_put(hmm);
range->hmm = NULL;
}
@@ -979,10 +976,7 @@ long hmm_range_snapshot(struct hmm_range *range)
struct vm_area_struct *vma;
struct mm_walk mm_walk;
- /* Check if hmm_mm_destroy() was call. */
- if (hmm->mm == NULL || hmm->dead)
- return -EFAULT;
-
+ lockdep_assert_held(&hmm->mm->mmap_sem);
do {
/* If range is no longer valid force retry. */
if (!range->valid)
@@ -1077,9 +1071,7 @@ long hmm_range_fault(struct hmm_range *range, bool block)
struct mm_walk mm_walk;
int ret;
- /* Check if hmm_mm_destroy() was call. */
- if (hmm->mm == NULL || hmm->dead)
- return -EFAULT;
+ lockdep_assert_held(&hmm->mm->mmap_sem);
do {
/* If range is no longer valid force retry. */
--
2.21.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* [PATCH v3 hmm 07/12] mm/hmm: Use lockdep instead of comments
From: Jason Gunthorpe @ 2019-06-14 0:44 UTC (permalink / raw)
To: Jerome Glisse, Ralph Campbell, John Hubbard, Felix.Kuehling
Cc: Andrea Arcangeli, Philip Yang, linux-rdma, amd-gfx,
Souptick Joarder, linux-mm, Jason Gunthorpe, dri-devel,
Ben Skeggs
In-Reply-To: <20190614004450.20252-1-jgg@ziepe.ca>
From: Jason Gunthorpe <jgg@mellanox.com>
So we can check locking at runtime.
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Reviewed-by: Jérôme Glisse <jglisse@redhat.com>
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>
Acked-by: Souptick Joarder <jrdr.linux@gmail.com>
Tested-by: Philip Yang <Philip.Yang@amd.com>
---
v2
- Fix missing & in lockdeps (Jason)
---
mm/hmm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/mm/hmm.c b/mm/hmm.c
index 58712d74edd585..c0f622f86223c2 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -253,11 +253,11 @@ static const struct mmu_notifier_ops hmm_mmu_notifier_ops = {
*
* To start mirroring a process address space, the device driver must register
* an HMM mirror struct.
- *
- * THE mm->mmap_sem MUST BE HELD IN WRITE MODE !
*/
int hmm_mirror_register(struct hmm_mirror *mirror, struct mm_struct *mm)
{
+ lockdep_assert_held_exclusive(&mm->mmap_sem);
+
/* Sanity check */
if (!mm || !mirror || !mirror->ops)
return -EINVAL;
--
2.21.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* [PATCH v3 hmm 08/12] mm/hmm: Remove racy protection against double-unregistration
From: Jason Gunthorpe @ 2019-06-14 0:44 UTC (permalink / raw)
To: Jerome Glisse, Ralph Campbell, John Hubbard, Felix.Kuehling
Cc: Andrea Arcangeli, Philip Yang, linux-rdma, amd-gfx, linux-mm,
Jason Gunthorpe, dri-devel, Ben Skeggs
In-Reply-To: <20190614004450.20252-1-jgg@ziepe.ca>
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>
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>
Tested-by: Philip Yang <Philip.Yang@amd.com>
---
mm/hmm.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/mm/hmm.c b/mm/hmm.c
index c0f622f86223c2..e3e0a811a3a774 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -283,18 +283,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));
}
EXPORT_SYMBOL(hmm_mirror_unregister);
--
2.21.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* [PATCH v3 hmm 09/12] mm/hmm: Poison hmm_range during unregister
From: Jason Gunthorpe @ 2019-06-14 0:44 UTC (permalink / raw)
To: Jerome Glisse, Ralph Campbell, John Hubbard, Felix.Kuehling
Cc: Andrea Arcangeli, Philip Yang, linux-rdma, amd-gfx,
Souptick Joarder, linux-mm, Jason Gunthorpe, dri-devel, Ira Weiny,
Ben Skeggs
In-Reply-To: <20190614004450.20252-1-jgg@ziepe.ca>
From: Jason Gunthorpe <jgg@mellanox.com>
Trying to misuse a range outside its lifetime is a kernel bug. Use poison
bytes to help detect this condition. Double unregister will reliably crash.
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Reviewed-by: Jérôme Glisse <jglisse@redhat.com>
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
Acked-by: Souptick Joarder <jrdr.linux@gmail.com>
Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Tested-by: Philip Yang <Philip.Yang@amd.com>
---
v2
- Keep range start/end valid after unregistration (Jerome)
v3
- Revise some comments (John)
- Remove start/end WARN_ON (Souptick)
---
mm/hmm.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/mm/hmm.c b/mm/hmm.c
index e3e0a811a3a774..e214668cba3474 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -933,19 +933,21 @@ void hmm_range_unregister(struct hmm_range *range)
{
struct hmm *hmm = range->hmm;
- /* Sanity check this really should not happen. */
- if (hmm == NULL || range->end <= range->start)
- return;
-
mutex_lock(&hmm->lock);
list_del_rcu(&range->list);
mutex_unlock(&hmm->lock);
/* Drop reference taken by hmm_range_register() */
- range->valid = false;
mmput(hmm->mm);
hmm_put(hmm);
- range->hmm = NULL;
+
+ /*
+ * The range is now invalid and the ref on the hmm is dropped, so
+ * poison the pointer. Leave other fields in place, for the caller's
+ * use.
+ */
+ range->valid = false;
+ memset(&range->hmm, POISON_INUSE, sizeof(range->hmm));
}
EXPORT_SYMBOL(hmm_range_unregister);
--
2.21.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* [PATCH v3 hmm 10/12] mm/hmm: Do not use list*_rcu() for hmm->ranges
From: Jason Gunthorpe @ 2019-06-14 0:44 UTC (permalink / raw)
To: Jerome Glisse, Ralph Campbell, John Hubbard, Felix.Kuehling
Cc: Andrea Arcangeli, Philip Yang, linux-rdma, Ira Weiny, amd-gfx,
Souptick Joarder, linux-mm, Jason Gunthorpe, dri-devel,
Ben Skeggs
In-Reply-To: <20190614004450.20252-1-jgg@ziepe.ca>
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>
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
Acked-by: Souptick Joarder <jrdr.linux@gmail.com>
Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>
Acked-by: Souptick Joarder <jrdr.linux@gmail.com>
Reviewed-by: Ira Weiny <iweiny@intel.com>
Tested-by: Philip Yang <Philip.Yang@amd.com>
---
mm/hmm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/mm/hmm.c b/mm/hmm.c
index e214668cba3474..26af511cbdd075 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -908,7 +908,7 @@ int hmm_range_register(struct hmm_range *range,
range->hmm = hmm;
kref_get(&hmm->kref);
- 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
@@ -934,7 +934,7 @@ void hmm_range_unregister(struct hmm_range *range)
struct hmm *hmm = range->hmm;
mutex_lock(&hmm->lock);
- list_del_rcu(&range->list);
+ list_del(&range->list);
mutex_unlock(&hmm->lock);
/* Drop reference taken by hmm_range_register() */
--
2.21.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* [PATCH v3 hmm 11/12] mm/hmm: Remove confusing comment and logic from hmm_release
From: Jason Gunthorpe @ 2019-06-14 0:44 UTC (permalink / raw)
To: Jerome Glisse, Ralph Campbell, John Hubbard, Felix.Kuehling
Cc: Andrea Arcangeli, Philip Yang, linux-rdma, amd-gfx, linux-mm,
Jason Gunthorpe, dri-devel, Ben Skeggs
In-Reply-To: <20190614004450.20252-1-jgg@ziepe.ca>
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>
Tested-by: Philip Yang <Philip.Yang@amd.com>
---
mm/hmm.c | 28 +++++++++-------------------
1 file changed, 9 insertions(+), 19 deletions(-)
diff --git a/mm/hmm.c b/mm/hmm.c
index 26af511cbdd075..c0d43302fd6b2f 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -137,26 +137,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);
+ list_for_each_entry(mirror, &hmm->mirrors, list) {
+ /*
+ * Note: The driver is not allowed to trigger
+ * hmm_mirror_unregister() from this thread.
+ */
+ if (mirror->ops->release)
mirror->ops->release(mirror);
- down_write(&hmm->mirrors_sem);
- }
- mirror = list_first_entry_or_null(&hmm->mirrors,
- struct hmm_mirror, list);
}
- up_write(&hmm->mirrors_sem);
+ up_read(&hmm->mirrors_sem);
hmm_put(hmm);
}
@@ -286,7 +276,7 @@ void hmm_mirror_unregister(struct hmm_mirror *mirror)
struct hmm *hmm = mirror->hmm;
down_write(&hmm->mirrors_sem);
- list_del_init(&mirror->list);
+ list_del(&mirror->list);
up_write(&hmm->mirrors_sem);
hmm_put(hmm);
memset(&mirror->hmm, POISON_INUSE, sizeof(mirror->hmm));
--
2.21.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* [PATCH v3 hmm 12/12] mm/hmm: Fix error flows in hmm_invalidate_range_start
From: Jason Gunthorpe @ 2019-06-14 0:44 UTC (permalink / raw)
To: Jerome Glisse, Ralph Campbell, John Hubbard, Felix.Kuehling
Cc: Andrea Arcangeli, Philip Yang, linux-rdma, amd-gfx, linux-mm,
Jason Gunthorpe, dri-devel, Ben Skeggs
In-Reply-To: <20190614004450.20252-1-jgg@ziepe.ca>
From: Jason Gunthorpe <jgg@mellanox.com>
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>
Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>
Tested-by: Philip Yang <Philip.Yang@amd.com>
---
include/linux/hmm.h | 2 +-
mm/hmm.c | 77 +++++++++++++++++++++++++++------------------
2 files changed, 48 insertions(+), 31 deletions(-)
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 c0d43302fd6b2f..1172a4f0206963 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -67,7 +67,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;
@@ -124,18 +124,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) {
@@ -151,6 +152,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)
{
@@ -158,6 +176,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))
@@ -168,12 +187,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)
@@ -181,7 +195,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);
@@ -189,16 +203,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;
}
@@ -207,23 +231,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);
}
@@ -876,6 +891,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;
@@ -894,7 +910,7 @@ int hmm_range_register(struct hmm_range *range,
return -EFAULT;
/* Initialize range to track CPU page table updates. */
- mutex_lock(&hmm->lock);
+ spin_lock_irqsave(&hmm->ranges_lock, flags);
range->hmm = hmm;
kref_get(&hmm->kref);
@@ -906,7 +922,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;
}
@@ -922,10 +938,11 @@ EXPORT_SYMBOL(hmm_range_register);
void hmm_range_unregister(struct hmm_range *range)
{
struct hmm *hmm = range->hmm;
+ unsigned long flags;
- 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() */
mmput(hmm->mm);
--
2.21.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* Re: [PATCH RFC 00/10] RDMA/FS DAX truncate proposal
From: Dave Chinner @ 2019-06-14 2:09 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Jan Kara, linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, John Hubbard, Jeff Layton,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Matthew Wilcox,
linux-xfs-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
Jérôme Glisse, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
Theodore Ts'o, linux-ext4-u79uwXL29TY76Z2rM5mHXA,
Andrew Morton
In-Reply-To: <20190613234530.GK22901-uk2M96/98Pc@public.gmane.org>
On Thu, Jun 13, 2019 at 08:45:30PM -0300, Jason Gunthorpe wrote:
> On Thu, Jun 13, 2019 at 02:13:21PM -0700, Ira Weiny wrote:
> > On Thu, Jun 13, 2019 at 08:27:55AM -0700, Matthew Wilcox wrote:
> > > On Thu, Jun 13, 2019 at 10:25:55AM +1000, Dave Chinner wrote:
> > > > e.g. Process A has an exclusive layout lease on file F. It does an
> > > > IO to file F. The filesystem IO path checks that Process A owns the
> > > > lease on the file and so skips straight through layout breaking
> > > > because it owns the lease and is allowed to modify the layout. It
> > > > then takes the inode metadata locks to allocate new space and write
> > > > new data.
> > > >
> > > > Process B now tries to write to file F. The FS checks whether
> > > > Process B owns a layout lease on file F. It doesn't, so then it
> > > > tries to break the layout lease so the IO can proceed. The layout
> > > > breaking code sees that process A has an exclusive layout lease
> > > > granted, and so returns -ETXTBSY to process B - it is not allowed to
> > > > break the lease and so the IO fails with -ETXTBSY.
> > >
> > > This description doesn't match the behaviour that RDMA wants either.
> > > Even if Process A has a lease on the file, an IO from Process A which
> > > results in blocks being freed from the file is going to result in the
> > > RDMA device being able to write to blocks which are now freed (and
> > > potentially reallocated to another file).
> >
> > I don't understand why this would not work for RDMA? As long as the layout
> > does not change the page pins can remain in place.
>
> Because process A had a layout lease (and presumably a MR) and the
> layout was still modified in way that invalidates the RDMA MR.
The lease holder is allowed to modify the mapping it has a lease
over. That's necessary so lease holders can write data into
unallocated space in the file. The lease is there to prevent third
parties from modifying the layout without the lease holder being
informed and taking appropriate action to allow that 3rd party
modification to occur.
If the lease holder modifies the mapping in a way that causes it's
own internal state to screw up, then that's a bug in the lease
holder application.
Cheers,
Dave.
--
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org
^ permalink raw reply
* Re: [PATCH RFC 00/10] RDMA/FS DAX truncate proposal
From: Matthew Wilcox @ 2019-06-14 2:31 UTC (permalink / raw)
To: Dave Chinner
Cc: Jason Gunthorpe, Ira Weiny, Jan Kara, Dan Williams,
Theodore Ts'o, Jeff Layton, linux-xfs, Andrew Morton,
John Hubbard, Jérôme Glisse, linux-fsdevel,
linux-kernel, linux-nvdimm, linux-ext4, linux-mm, linux-rdma
In-Reply-To: <20190614020921.GM14363@dread.disaster.area>
On Fri, Jun 14, 2019 at 12:09:21PM +1000, Dave Chinner wrote:
> On Thu, Jun 13, 2019 at 08:45:30PM -0300, Jason Gunthorpe wrote:
> > On Thu, Jun 13, 2019 at 02:13:21PM -0700, Ira Weiny wrote:
> > > On Thu, Jun 13, 2019 at 08:27:55AM -0700, Matthew Wilcox wrote:
> > > > On Thu, Jun 13, 2019 at 10:25:55AM +1000, Dave Chinner wrote:
> > > > > e.g. Process A has an exclusive layout lease on file F. It does an
> > > > > IO to file F. The filesystem IO path checks that Process A owns the
> > > > > lease on the file and so skips straight through layout breaking
> > > > > because it owns the lease and is allowed to modify the layout. It
> > > > > then takes the inode metadata locks to allocate new space and write
> > > > > new data.
> > > > >
> > > > > Process B now tries to write to file F. The FS checks whether
> > > > > Process B owns a layout lease on file F. It doesn't, so then it
> > > > > tries to break the layout lease so the IO can proceed. The layout
> > > > > breaking code sees that process A has an exclusive layout lease
> > > > > granted, and so returns -ETXTBSY to process B - it is not allowed to
> > > > > break the lease and so the IO fails with -ETXTBSY.
> > > >
> > > > This description doesn't match the behaviour that RDMA wants either.
> > > > Even if Process A has a lease on the file, an IO from Process A which
> > > > results in blocks being freed from the file is going to result in the
> > > > RDMA device being able to write to blocks which are now freed (and
> > > > potentially reallocated to another file).
> > >
> > > I don't understand why this would not work for RDMA? As long as the layout
> > > does not change the page pins can remain in place.
> >
> > Because process A had a layout lease (and presumably a MR) and the
> > layout was still modified in way that invalidates the RDMA MR.
>
> The lease holder is allowed to modify the mapping it has a lease
> over. That's necessary so lease holders can write data into
> unallocated space in the file. The lease is there to prevent third
> parties from modifying the layout without the lease holder being
> informed and taking appropriate action to allow that 3rd party
> modification to occur.
>
> If the lease holder modifies the mapping in a way that causes it's
> own internal state to screw up, then that's a bug in the lease
> holder application.
Sounds like the lease semantics aren't the right ones for the longterm
GUP users then. The point of the longterm GUP is so the pages can be
written to, and if the filesystem is going to move the pages around when
they're written to, that just won't work.
^ permalink raw reply
* Re: [PATCH RFC 00/10] RDMA/FS DAX truncate proposal
From: Dave Chinner @ 2019-06-14 2:58 UTC (permalink / raw)
To: Ira Weiny
Cc: Matthew Wilcox, Jan Kara, Dan Williams, Theodore Ts'o,
Jeff Layton, 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: <20190613203404.GA30404@iweiny-DESK2.sc.intel.com>
On Thu, Jun 13, 2019 at 01:34:05PM -0700, Ira Weiny wrote:
> On Thu, Jun 13, 2019 at 10:25:55AM +1000, Dave Chinner wrote:
> > On Wed, Jun 12, 2019 at 05:37:53AM -0700, Matthew Wilcox wrote:
> > > On Sat, Jun 08, 2019 at 10:10:36AM +1000, Dave Chinner wrote:
> > > > On Fri, Jun 07, 2019 at 11:25:35AM -0700, Ira Weiny wrote:
> > > > > Are you suggesting that we have something like this from user space?
> > > > >
> > > > > fcntl(fd, F_SETLEASE, F_LAYOUT | F_UNBREAKABLE);
> > > >
> > > > Rather than "unbreakable", perhaps a clearer description of the
> > > > policy it entails is "exclusive"?
> > > >
> > > > i.e. what we are talking about here is an exclusive lease that
> > > > prevents other processes from changing the layout. i.e. the
> > > > mechanism used to guarantee a lease is exclusive is that the layout
> > > > becomes "unbreakable" at the filesystem level, but the policy we are
> > > > actually presenting to uses is "exclusive access"...
> > >
> > > That's rather different from the normal meaning of 'exclusive' in the
> > > context of locks, which is "only one user can have access to this at
> > > a time".
> >
> >
> > Layout leases are not locks, they are a user access policy object.
> > It is the process/fd which holds the lease and it's the process/fd
> > that is granted exclusive access. This is exactly the same semantic
> > as O_EXCL provides for granting exclusive access to a block device
> > via open(), yes?
> >
> > > As I understand it, this is rather more like a 'shared' or
> > > 'read' lock. The filesystem would be the one which wants an exclusive
> > > lock, so it can modify the mapping of logical to physical blocks.
> >
> > ISTM that you're conflating internal filesystem implementation with
> > application visible semantics. Yes, the filesystem uses internal
> > locks to serialise the modification of the things the lease manages
> > access too, but that has nothing to do with the access policy the
> > lease provides to users.
> >
> > e.g. Process A has an exclusive layout lease on file F. It does an
> > IO to file F. The filesystem IO path checks that Process A owns the
> > lease on the file and so skips straight through layout breaking
> > because it owns the lease and is allowed to modify the layout. It
> > then takes the inode metadata locks to allocate new space and write
> > new data.
> >
> > Process B now tries to write to file F. The FS checks whether
> > Process B owns a layout lease on file F. It doesn't, so then it
> > tries to break the layout lease so the IO can proceed. The layout
> > breaking code sees that process A has an exclusive layout lease
> > granted, and so returns -ETXTBSY to process B - it is not allowed to
> > break the lease and so the IO fails with -ETXTBSY.
> >
> > i.e. the exclusive layout lease prevents other processes from
> > performing operations that may need to modify the layout from
> > performing those operations. It does not "lock" the file/inode in
> > any way, it just changes how the layout lease breaking behaves.
>
> Question: Do we expect Process A to get notified that Process B was attempting
> to change the layout?
In which case?
In the non-exclusive case, yes, the lease gets
recalled and the application needs to play nice and release it's
references and drop the lease.
In the exclusive case, no. The application has said "I don't play
nice with others" and so we basically tell process B to get stuffed
and process A can continue onwards oblivious to the wreckage it
leaves behind....
> This changes the exclusivity semantics. While Process A has an exclusive lease
> it could release it if notified to allow process B temporary exclusivity.
And then it's not an exclusive lease - it's just a normal layout
lease. Process B -does not need a lease- to write to the file.
All the layout lease does is provide notification to applications
that rely on the layout of the file being under their control that
someone else is about to modify the layout. The lease holder that
"plays nice" then releases the layout and drops it's lease, allowing
process B to begin it's operation.
Process A then immediately takes a new layout lease, and remaps the
file layout via FIEMAP or by creating a new RDMA MR for the mmap
region. THose operations get serialised by the filesystem because
the operation being run by process B is run atomically w.r.t. the
original lease being broken. Hence the new mapping that process A
gets with it's new lease reflects whatever change was made by
process B.
IOWs, the "normal" layout lease recall behaviour provides "temporary
exclusivity" for third parties. If you are able to release leases
temporarily and regain them then there is no need for an exclusive
lease.
> Question 2: Do we expect other process' (say Process C) to also be able to map
> and pin the file? I believe users will need this and for layout purposes it is
> ok to do so. But this means that Process A does not have "exclusive" access to
> the lease.
This is an application architecture problem, not a layout lease or
filesystem problem. :)
i.e. if you have a single process controlling all the RDMA mappings,
then you can use exclusive leases. If you have multiple processes
that are uncoordinated and all require layout access to the same
file then you can't use exclusive layout leases in the application.
i.e. your application has to play nice with others.
Indeed, this is more than a application architecture problem - it's
actually a system wide architecture problem. e.g. the pNFS server
cannot use exclusive layout leases because it has to play nice with
anything else on the local filesystem that might require a layout
lease. An example of this woudl be an app that provides coherent
RDMA access to the same storage that pNFS is sharing (e.g. a
userspace CIFS server).
Hence I see that exclusive layout leases will end up being the
exception rather than the norm, because most applications will need
to play nice with other applications on the system that also
directly access the storage under the filesystem....
> So given Process C has also placed a layout lease on the file. Indicating
> that it does not want the layout to change.
That is *not what layout leases provide*.
Layout leases grant the owner the ability to map the layout and
directly access the underlying storage and to do it safely because
they will get a notification of 3rd party access that will
invalidate their mapping. Layout leases do not prevent anyone from
_changing_ the layout and, in fact, pNFS _requires_ the lease holder
to be able to modify the layout.
IOWs, the layout lease _as it stands now_ is a notification
mechanism that tells the lease owner when someone else is about to
modify the layout. It does not make the file layout immutable.
The "exclusive" aspect of layout we have been discussing is a
mechanism that prevents 3rd party modification of the layout by
denying the ability to break the layout. This "exclusive" aspect
does not make the layout immutable, either, it just means the
layout is only modifiable by the exclusive lease holder.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply
* Re: [PATCH RFC 00/10] RDMA/FS DAX truncate proposal
From: Dave Chinner @ 2019-06-14 3:07 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Theodore Ts'o, linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, John Hubbard, Jeff Layton,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-xfs-u79uwXL29TY76Z2rM5mHXA, Jason Gunthorpe,
Jérôme Glisse, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Jan Kara,
linux-ext4-u79uwXL29TY76Z2rM5mHXA, Andrew Morton
In-Reply-To: <20190614023107.GK32656-PfSpb0PWhxZc2C7mugBRk2EX/6BAtgUQ@public.gmane.org>
On Thu, Jun 13, 2019 at 07:31:07PM -0700, Matthew Wilcox wrote:
> On Fri, Jun 14, 2019 at 12:09:21PM +1000, Dave Chinner wrote:
> > If the lease holder modifies the mapping in a way that causes it's
> > own internal state to screw up, then that's a bug in the lease
> > holder application.
>
> Sounds like the lease semantics aren't the right ones for the longterm
> GUP users then. The point of the longterm GUP is so the pages can be
> written to, and if the filesystem is going to move the pages around when
> they're written to, that just won't work.
And now we go full circle back to the constraints we decided on long
ago because we can't rely on demand paging RDMA hardware any time
soon to do everything we need to transparently support long-term GUP
on file-backed mappings. i.e.:
RDMA to file backed mappings must first preallocate and
write zeros to the range of the file they are mapping so
that the filesystem block mapping is complete and static for
the life of the RDMA mapping that will pin it.
IOWs, the layout lease will tell the RDMA application that the
static setup it has already done to work correctly with a file
backed mapping may be about to be broken by a third party.....
-Dave.
--
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org
^ permalink raw reply
* Re: [PATCH RFC 00/10] RDMA/FS DAX truncate proposal
From: Dave Chinner @ 2019-06-14 3:42 UTC (permalink / raw)
To: Ira Weiny
Cc: Matthew Wilcox, Jan Kara, Dan Williams, Theodore Ts'o,
Jeff Layton, 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: <20190613203406.GB32404@iweiny-DESK2.sc.intel.com>
On Thu, Jun 13, 2019 at 01:34:06PM -0700, Ira Weiny wrote:
> On Thu, Jun 13, 2019 at 10:55:52AM +1000, Dave Chinner wrote:
> > On Wed, Jun 12, 2019 at 04:30:24PM -0700, Ira Weiny wrote:
> > > On Wed, Jun 12, 2019 at 05:37:53AM -0700, Matthew Wilcox wrote:
> > > > On Sat, Jun 08, 2019 at 10:10:36AM +1000, Dave Chinner wrote:
> > > > > On Fri, Jun 07, 2019 at 11:25:35AM -0700, Ira Weiny wrote:
> > > > > > Are you suggesting that we have something like this from user space?
> > > > > >
> > > > > > fcntl(fd, F_SETLEASE, F_LAYOUT | F_UNBREAKABLE);
> > > > >
> > > > > Rather than "unbreakable", perhaps a clearer description of the
> > > > > policy it entails is "exclusive"?
> > > > >
> > > > > i.e. what we are talking about here is an exclusive lease that
> > > > > prevents other processes from changing the layout. i.e. the
> > > > > mechanism used to guarantee a lease is exclusive is that the layout
> > > > > becomes "unbreakable" at the filesystem level, but the policy we are
> > > > > actually presenting to uses is "exclusive access"...
> > > >
> > > > That's rather different from the normal meaning of 'exclusive' in the
> > > > context of locks, which is "only one user can have access to this at
> > > > a time". As I understand it, this is rather more like a 'shared' or
> > > > 'read' lock. The filesystem would be the one which wants an exclusive
> > > > lock, so it can modify the mapping of logical to physical blocks.
> > > >
> > > > The complication being that by default the filesystem has an exclusive
> > > > lock on the mapping, and what we're trying to add is the ability for
> > > > readers to ask the filesystem to give up its exclusive lock.
> > >
> > > This is an interesting view...
> > >
> > > And after some more thought, exclusive does not seem like a good name for this
> > > because technically F_WRLCK _is_ an exclusive lease...
> > >
> > > In addition, the user does not need to take the "exclusive" write lease to be
> > > notified of (broken by) an unexpected truncate. A "read" lease is broken by
> > > truncate. (And "write" leases really don't do anything different WRT the
> > > interaction of the FS and the user app. Write leases control "exclusive"
> > > access between other file descriptors.)
> >
> > I've been assuming that there is only one type of layout lease -
> > there is no use case I've heard of for read/write layout leases, and
> > like you say there is zero difference in behaviour at the filesystem
> > level - they all have to be broken to allow a non-lease truncate to
> > proceed.
> >
> > IMO, taking a "read lease" to be able to modify and write to the
> > underlying mapping of a file makes absolutely no sense at all.
> > IOWs, we're talking exaclty about a revokable layout lease vs an
> > exclusive layout lease here, and so read/write really doesn't match
> > the policy or semantics we are trying to provide.
>
> I humbly disagree, at least depending on how you look at it... :-D
>
> The patches as they stand expect the user to take a "read" layout lease which
> indicates they are currently using "reading" the layout as is.
> They are not
> changing ("writing" to) the layout.
As I said in a another email in the thread, a layout lease does not
make the layout "read only". It just means the lease owner will be
notified when someone else is about to modify it. The lease owner
can modify the mapping themselves, and they will not get notified
about their own modifications.
> They then pin pages which locks parts of
> the layout and therefore they expect no "writers" to change the layout.
Except they can change the layout themselves. It's perfectly valid
to get a layout lease, write() from offset 0 to EOF and fsync() to
intiialise the file and allocate all the space in the file, then
mmap() it and hand to off to RMDA, all while holding the layout
lease.
> The "write" layout lease breaks the "read" layout lease indicating that the
> layout is being written to.
Layout leases do not work this way.
> In fact, this is what NFS does right now. The lease it puts on the file is of
> "read" type.
>
> nfs4layouts.c:
> static int
> nfsd4_layout_setlease(struct nfs4_layout_stateid *ls)
> {
> ...
> fl->fl_flags = FL_LAYOUT;
> fl->fl_type = F_RDLCK;
> ...
> }
Yes, the existing /implementation/ uses F_RDLCK, but that doesn't
mean the layout is "read only". Look at the pNFS mapping layout code
- the ->map_blocks export operation:
int (*map_blocks)(struct inode *inode, loff_t offset,
u64 len, struct iomap *iomap,
bool write, u32 *device_generation);
^^^^^^^^^^
Yup, it has a write variable that, when set, causes the filesystem
to _allocate_ blocks if the range to be written to falls over a hole
in the file. IOWs, a pNFS layout lease can modify the file layout -
you're conflating use of a "read lock" API to mean that what the
lease _manages_ is "read only". That is not correct.
Layouts are /always writeable/ by the lease owner(s), the question
here is what we do with third parties attempting to modify a layout
covered by an "exclusive" layout lease. Hence, I'll repeat:
> > we're talking exaclty about a revokable layout lease vs an
> > exclusive layout lease here, and so read/write really doesn't match
> > the policy or semantics we are trying to provide.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply
* Re: [PATCH v17 03/15] arm64: Introduce prctl() options to control the tagged user addresses ABI
From: Kees Cook @ 2019-06-14 5:13 UTC (permalink / raw)
To: Catalin Marinas
Cc: Dave Martin, Andrey Konovalov, linux-arm-kernel, linux-mm,
linux-kernel, amd-gfx, dri-devel, linux-rdma, linux-media, kvm,
linux-kselftest, Mark Rutland, Szabolcs Nagy, Will Deacon,
Kostya Serebryany, Khalid Aziz, Felix Kuehling, Vincenzo Frascino,
Jacob Bramley, Leon Romanovsky, Christoph Hellwig,
Jason Gunthorpe
In-Reply-To: <20190613152632.GT28951@C02TF0J2HF1T.local>
On Thu, Jun 13, 2019 at 04:26:32PM +0100, Catalin Marinas wrote:
> On Thu, Jun 13, 2019 at 12:02:35PM +0100, Dave P Martin wrote:
> > On Wed, Jun 12, 2019 at 01:43:20PM +0200, Andrey Konovalov wrote:
> > > +static int zero;
> > > +static int one = 1;
> >
> > !!!
> >
> > And these can't even be const without a cast. Yuk.
> >
> > (Not your fault though, but it would be nice to have a proc_dobool() to
> > avoid this.)
>
> I had the same reaction. Maybe for another patch sanitising this pattern
> across the kernel.
That's actually already happening (via -mm tree last I looked). tl;dr:
it ends up using a cast hidden in a macro. It's in linux-next already
along with a checkpatch.pl addition to yell about doing what's being
done here. ;)
https://lore.kernel.org/lkml/20190430180111.10688-1-mcroce@redhat.com/#r
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2] RDMA/cma: Make CM response timeout and # CM retries configurable
From: Håkon Bugge @ 2019-06-14 5:44 UTC (permalink / raw)
To: Doug Ledford
Cc: Jason Gunthorpe, Leon Romanovsky, Parav Pandit, Steve Wise,
OFED mailing list, linux-kernel
In-Reply-To: <6e586118ad154204ad2e2cf2c1391b916cb4ee54.camel@redhat.com>
> On 13 Jun 2019, at 22:25, Doug Ledford <dledford@redhat.com> wrote:
>
> On Thu, 2019-06-13 at 18:58 +0200, Håkon Bugge wrote:
>>> On 13 Jun 2019, at 16:25, Doug Ledford <dledford@redhat.com> wrote:
>>>
>>> On Tue, 2019-02-26 at 08:57 +0100, Håkon Bugge wrote:
>>>> During certain workloads, the default CM response timeout is too
>>>> short, leading to excessive retries. Hence, make it configurable
>>>> through sysctl. While at it, also make number of CM retries
>>>> configurable.
>>>>
>>>> The defaults are not changed.
>>>>
>>>> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
>>>> ---
>>>> v1 -> v2:
>>>> * Added unregister_net_sysctl_table() in cma_cleanup()
>>>> ---
>>>> drivers/infiniband/core/cma.c | 52
>>>> ++++++++++++++++++++++++++++++---
>>>> --
>>>> 1 file changed, 45 insertions(+), 7 deletions(-)
>>>
>>> This has been sitting on patchworks since forever. Presumably
>>> because
>>> Jason and I neither one felt like we really wanted it, but also
>>> couldn't justify flat refusing it.
>>
>> I thought the agreement was to use NL and iproute2. But I haven't had
>> the capacity.
>
> To be fair, the email thread was gone from my linux-rdma folder. So, I
> just had to review the entry in patchworks, and there was no captured
> discussion there. So, if the agreement was made, it must have been
> face to face some time and if I was involed, I had certainly forgotten
> by now. But I still needed to clean up patchworks, hence my email ;-).
This is the "agreement" I was referring too:
> On 4 Mar 2019, at 07:27, Parav Pandit <parav@mellanox.com> wrote:
>
>> []
>
> I think we should use rdma_nl_register(RDMA_NL_RDMA_CM, cma_cb_table) which was removed as part of ID stats removal.
> Because of below reasons.
> 1. rdma netlink command auto loads the module
> 2. we don't need to write any extra code to do register_net_sysctl () in each netns.
> Caller's skb's netns will read/write value of response_timeout in 'struct cma_pernet'.
> 3. last time sysctl added in ipv6 was in 2017 in net/ipv6/addrconf.c, however ipv4 was done in 2018.
>
> Currently rdma_cm/rdma_ucma has configfs, sysctl.
> We are adding netlink sys params to ib_core.
>
> We already have 3 clients and infra built using rdma_nl_register() netlink , so hooking up to netlink will provide unified way to set rdma params.
> Let's just use netlink for any new params unless it is not doable.
>
>>> Well, I've made up my mind, so
>>> unless Jason wants to argue the other side, I'm rejecting this
>>> patch.
>>> Here's why. The whole concept of a timeout is to help recovery in
>>> a
>>> situation that overloads one end of the connection. There is a
>>> relationship between the max queue backlog on the one host and the
>>> timeout on the other host.
>>
>> If you refer to the backlog parameter in rdma_listen(), I cannot see
>> it being used at all for IB.
>
> No, not exactly. I was more referring to heavy load causing an
> overflow in the mad packet receive processing. We have
> IB_MAD_QP_RECV_SIZE set to 512 by default, but it can be changed at
> module load time of the ib_core module and that represents the maximum
> number of backlogged mad packets we can have waiting to be processed
> before we just drop them on the floor. There can be other places to
> drop them too, but this is the one I was referring to.
That is another scenario than what I try to solve. What I see, is that the MAD packets are delayed, not lost. The delay is longer than the CMA timeout. Hence, the MAD packets are retried, adding more burden to the PF proxying and inducing even longer delays. And excessive CM retries are observed. See 2612d723aadc ("IB/mlx4: Increase the timeout for CM cache") where I have some quantification thereof.
Back to your scenario above, yes indeed, the queue sizes are module params. If the MADs are tossed, we will see rq_num_udsdprd incrementing on a CX-3.
But I do not understand how the dots are connected. Assume one client does rdma_listen(, backlog = 1000); Where are those 1000 REQs stored, assuming an "infinite slow processor"?
Thxs, Håkon
>
>> For CX-3, which is paravirtualized wrt. MAD packets, it is the proxy
>> UD receive queue length for the PF driver that can be construed as a
>> backlog. Remember that any MAD packet being sent from a VF or the PF
>> itself, is sent to a proxy UD QP in the PF. Those packets are then
>> multiplexed out on the real QP0/1. Incoming MAD packets are
>> demultiplexed and sent once more to the proxy QP in the VF.
>>
>>> Generally, in order for a request to get
>>> dropped and us to need to retransmit, the queue must already have a
>>> full backlog. So, how long does it take a heavily loaded system to
>>> process a full backlog? That, plus a fuzz for a margin of error,
>>> should be our timeout. We shouldn't be asking users to configure
>>> it.
>>
>> Customer configures #VMs and different workload may lead to way
>> different number of CM connections. The proxying of MAD packet
>> through the PF driver has a finite packet rate. With 64 VMs, 10.000
>> QPs on each, all going down due to a switch failing or similar, you
>> have 640.000 DREQs to be sent, and with the finite packet rate of MA
>> packets through the PF, this takes more than the current CM timeout.
>> And then you re-transmit and increase the burden of the PF proxying.
>>
>> So, we can change the default to cope with this. But, a MAD packet is
>> unreliable, we may have transient loss. In this case, we want a short
>> timeout.
>>
>>> However, if users change the default backlog queue on their
>>> systems,
>>> *then* it would make sense to have the users also change the
>>> timeout
>>> here, but I think guidance would be helpful.
>>>
>>> So, to revive this patch, what I'd like to see is some attempt to
>>> actually quantify a reasonable timeout for the default backlog
>>> depth,
>>> then the patch should actually change the default to that
>>> reasonable
>>> timeout, and then put in the ability to adjust the timeout with
>>> some
>>> sort of doc guidance on how to calculate a reasonable timeout based
>>> on
>>> configured backlog depth.
>>
>> I can agree to this :-)
>>
>>
>> Thxs, Håkon
>>
>>> --
>>> Doug Ledford <dledford@redhat.com>
>>> GPG KeyID: B826A3330E572FDD
>>> Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57
>>> 2FDD
>
> --
> Doug Ledford <dledford@redhat.com>
> GPG KeyID: B826A3330E572FDD
> Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57
> 2FDD
^ permalink raw reply
* [PATCH 1/2] ipoib: correcly show a VF hardware address
From: Denis Kirjanov @ 2019-06-14 13:32 UTC (permalink / raw)
To: davem, dledford; +Cc: netdev, linux-rdma, mkubecek, Denis Kirjanov
in the case of IPoIB with SRIOV enabled hardware
ip link show command incorrecly prints
0 instead of a VF hardware address.
Before:
11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
state UP mode DEFAULT group default qlen 256
link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
vf 0 MAC 00:00:00:00:00:00, spoof checking off, link-state disable,
trust off, query_rss off
...
After:
11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
state UP mode DEFAULT group default qlen 256
link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
vf 0 link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff, spoof
checking off, link-state disable, trust off, query_rss off
v1->v2: just copy an address without modifing ifla_vf_mac
v2->v3: update the changelog
Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
---
drivers/infiniband/ulp/ipoib/ipoib_main.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 9b5e11d3fb85..04ea7db08e87 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -1998,6 +1998,7 @@ static int ipoib_get_vf_config(struct net_device *dev, int vf,
return err;
ivf->vf = vf;
+ memcpy(ivf->mac, dev->dev_addr, dev->addr_len);
return 0;
}
--
2.12.3
^ permalink raw reply related
* [PATCH 2/2] ipoib: show VF broadcast address
From: Denis Kirjanov @ 2019-06-14 13:32 UTC (permalink / raw)
To: davem, dledford; +Cc: netdev, linux-rdma, mkubecek, Denis Kirjanov
In-Reply-To: <20190614133249.18308-1-dkirjanov@suse.com>
in IPoIB case we can't see a VF broadcast address for but
can see for PF
Before:
11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
state UP mode DEFAULT group default qlen 256
link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
vf 0 MAC 14:80:00:00:66:fe, spoof checking off, link-state disable,
trust off, query_rss off
...
After:
11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
state UP mode DEFAULT group default qlen 256
link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
vf 0 link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff, spoof
checking off, link-state disable, trust off, query_rss off
v1->v2: add the IFLA_VF_BROADCAST constant
v2->v3: put IFLA_VF_BROADCAST at the end
to avoid KABI breakage and set NLA_REJECT
dev_setlink
Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
---
include/uapi/linux/if_link.h | 5 +++++
net/core/rtnetlink.c | 5 +++++
2 files changed, 10 insertions(+)
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 5b225ff63b48..6f75bda2c2d7 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -694,6 +694,7 @@ enum {
IFLA_VF_IB_NODE_GUID, /* VF Infiniband node GUID */
IFLA_VF_IB_PORT_GUID, /* VF Infiniband port GUID */
IFLA_VF_VLAN_LIST, /* nested list of vlans, option for QinQ */
+ IFLA_VF_BROADCAST, /* VF broadcast */
__IFLA_VF_MAX,
};
@@ -704,6 +705,10 @@ struct ifla_vf_mac {
__u8 mac[32]; /* MAX_ADDR_LEN */
};
+struct ifla_vf_broadcast {
+ __u8 broadcast[32];
+};
+
struct ifla_vf_vlan {
__u32 vf;
__u32 vlan; /* 0 - 4095, 0 disables VLAN filter */
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index cec60583931f..8ac81630ab5c 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -908,6 +908,7 @@ static inline int rtnl_vfinfo_size(const struct net_device *dev,
size += num_vfs *
(nla_total_size(0) +
nla_total_size(sizeof(struct ifla_vf_mac)) +
+ nla_total_size(sizeof(struct ifla_vf_broadcast)) +
nla_total_size(sizeof(struct ifla_vf_vlan)) +
nla_total_size(0) + /* nest IFLA_VF_VLAN_LIST */
nla_total_size(MAX_VLAN_LIST_LEN *
@@ -1197,6 +1198,7 @@ static noinline_for_stack int rtnl_fill_vfinfo(struct sk_buff *skb,
struct ifla_vf_vlan vf_vlan;
struct ifla_vf_rate vf_rate;
struct ifla_vf_mac vf_mac;
+ struct ifla_vf_broadcast vf_broadcast;
struct ifla_vf_info ivi;
memset(&ivi, 0, sizeof(ivi));
@@ -1231,6 +1233,7 @@ static noinline_for_stack int rtnl_fill_vfinfo(struct sk_buff *skb,
vf_trust.vf = ivi.vf;
memcpy(vf_mac.mac, ivi.mac, sizeof(ivi.mac));
+ memcpy(vf_broadcast.broadcast, dev->broadcast, dev->addr_len);
vf_vlan.vlan = ivi.vlan;
vf_vlan.qos = ivi.qos;
vf_vlan_info.vlan = ivi.vlan;
@@ -1247,6 +1250,7 @@ static noinline_for_stack int rtnl_fill_vfinfo(struct sk_buff *skb,
if (!vf)
goto nla_put_vfinfo_failure;
if (nla_put(skb, IFLA_VF_MAC, sizeof(vf_mac), &vf_mac) ||
+ nla_put(skb, IFLA_VF_BROADCAST, sizeof(vf_broadcast), &vf_broadcast) ||
nla_put(skb, IFLA_VF_VLAN, sizeof(vf_vlan), &vf_vlan) ||
nla_put(skb, IFLA_VF_RATE, sizeof(vf_rate),
&vf_rate) ||
@@ -1753,6 +1757,7 @@ static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
static const struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = {
[IFLA_VF_MAC] = { .len = sizeof(struct ifla_vf_mac) },
+ [IFLA_VF_BROADCAST] = { .type = NLA_REJECT },
[IFLA_VF_VLAN] = { .len = sizeof(struct ifla_vf_vlan) },
[IFLA_VF_VLAN_LIST] = { .type = NLA_NESTED },
[IFLA_VF_TX_RATE] = { .len = sizeof(struct ifla_vf_tx_rate) },
--
2.12.3
^ permalink raw reply related
* [PATCH net-next v3 1/2] ipoib: correcly show a VF hardware address
From: Denis Kirjanov @ 2019-06-14 13:32 UTC (permalink / raw)
To: davem, dledford; +Cc: netdev, linux-rdma, mkubecek, Denis Kirjanov
In-Reply-To: <20190614133249.18308-1-dkirjanov@suse.com>
in the case of IPoIB with SRIOV enabled hardware
ip link show command incorrecly prints
0 instead of a VF hardware address.
Before:
11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
state UP mode DEFAULT group default qlen 256
link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
vf 0 MAC 00:00:00:00:00:00, spoof checking off, link-state disable,
trust off, query_rss off
...
After:
11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
state UP mode DEFAULT group default qlen 256
link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
vf 0 link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff, spoof
checking off, link-state disable, trust off, query_rss off
v1->v2: just copy an address without modifing ifla_vf_mac
v2->v3: update the changelog
Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
---
drivers/infiniband/ulp/ipoib/ipoib_main.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 9b5e11d3fb85..04ea7db08e87 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -1998,6 +1998,7 @@ static int ipoib_get_vf_config(struct net_device *dev, int vf,
return err;
ivf->vf = vf;
+ memcpy(ivf->mac, dev->dev_addr, dev->addr_len);
return 0;
}
--
2.12.3
^ permalink raw reply related
* [PATCH net-next v3 2/2] ipoib: show VF broadcast address
From: Denis Kirjanov @ 2019-06-14 13:32 UTC (permalink / raw)
To: davem, dledford; +Cc: netdev, linux-rdma, mkubecek, Denis Kirjanov
In-Reply-To: <20190614133249.18308-1-dkirjanov@suse.com>
in IPoIB case we can't see a VF broadcast address for but
can see for PF
Before:
11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
state UP mode DEFAULT group default qlen 256
link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
vf 0 MAC 14:80:00:00:66:fe, spoof checking off, link-state disable,
trust off, query_rss off
...
After:
11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
state UP mode DEFAULT group default qlen 256
link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
vf 0 link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff, spoof
checking off, link-state disable, trust off, query_rss off
v1->v2: add the IFLA_VF_BROADCAST constant
v2->v3: put IFLA_VF_BROADCAST at the end
to avoid KABI breakage and set NLA_REJECT
dev_setlink
Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
---
include/uapi/linux/if_link.h | 5 +++++
net/core/rtnetlink.c | 5 +++++
2 files changed, 10 insertions(+)
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 5b225ff63b48..6f75bda2c2d7 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -694,6 +694,7 @@ enum {
IFLA_VF_IB_NODE_GUID, /* VF Infiniband node GUID */
IFLA_VF_IB_PORT_GUID, /* VF Infiniband port GUID */
IFLA_VF_VLAN_LIST, /* nested list of vlans, option for QinQ */
+ IFLA_VF_BROADCAST, /* VF broadcast */
__IFLA_VF_MAX,
};
@@ -704,6 +705,10 @@ struct ifla_vf_mac {
__u8 mac[32]; /* MAX_ADDR_LEN */
};
+struct ifla_vf_broadcast {
+ __u8 broadcast[32];
+};
+
struct ifla_vf_vlan {
__u32 vf;
__u32 vlan; /* 0 - 4095, 0 disables VLAN filter */
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index cec60583931f..8ac81630ab5c 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -908,6 +908,7 @@ static inline int rtnl_vfinfo_size(const struct net_device *dev,
size += num_vfs *
(nla_total_size(0) +
nla_total_size(sizeof(struct ifla_vf_mac)) +
+ nla_total_size(sizeof(struct ifla_vf_broadcast)) +
nla_total_size(sizeof(struct ifla_vf_vlan)) +
nla_total_size(0) + /* nest IFLA_VF_VLAN_LIST */
nla_total_size(MAX_VLAN_LIST_LEN *
@@ -1197,6 +1198,7 @@ static noinline_for_stack int rtnl_fill_vfinfo(struct sk_buff *skb,
struct ifla_vf_vlan vf_vlan;
struct ifla_vf_rate vf_rate;
struct ifla_vf_mac vf_mac;
+ struct ifla_vf_broadcast vf_broadcast;
struct ifla_vf_info ivi;
memset(&ivi, 0, sizeof(ivi));
@@ -1231,6 +1233,7 @@ static noinline_for_stack int rtnl_fill_vfinfo(struct sk_buff *skb,
vf_trust.vf = ivi.vf;
memcpy(vf_mac.mac, ivi.mac, sizeof(ivi.mac));
+ memcpy(vf_broadcast.broadcast, dev->broadcast, dev->addr_len);
vf_vlan.vlan = ivi.vlan;
vf_vlan.qos = ivi.qos;
vf_vlan_info.vlan = ivi.vlan;
@@ -1247,6 +1250,7 @@ static noinline_for_stack int rtnl_fill_vfinfo(struct sk_buff *skb,
if (!vf)
goto nla_put_vfinfo_failure;
if (nla_put(skb, IFLA_VF_MAC, sizeof(vf_mac), &vf_mac) ||
+ nla_put(skb, IFLA_VF_BROADCAST, sizeof(vf_broadcast), &vf_broadcast) ||
nla_put(skb, IFLA_VF_VLAN, sizeof(vf_vlan), &vf_vlan) ||
nla_put(skb, IFLA_VF_RATE, sizeof(vf_rate),
&vf_rate) ||
@@ -1753,6 +1757,7 @@ static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
static const struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = {
[IFLA_VF_MAC] = { .len = sizeof(struct ifla_vf_mac) },
+ [IFLA_VF_BROADCAST] = { .type = NLA_REJECT },
[IFLA_VF_VLAN] = { .len = sizeof(struct ifla_vf_vlan) },
[IFLA_VF_VLAN_LIST] = { .type = NLA_NESTED },
[IFLA_VF_TX_RATE] = { .len = sizeof(struct ifla_vf_tx_rate) },
--
2.12.3
^ permalink raw reply related
* Re: [PATCH net-next v2 2/2] ipoib: show VF broadcast address
From: Denis Kirjanov @ 2019-06-14 13:37 UTC (permalink / raw)
To: Michal Kubecek; +Cc: davem, dledford, netdev, linux-rdma
In-Reply-To: <20190613163941.GK31797@unicorn.suse.cz>
On 6/13/19, Michal Kubecek <mkubecek@suse.cz> wrote:
> On Thu, Jun 13, 2019 at 04:20:03PM +0200, Denis Kirjanov wrote:
>> in IPoIB case we can't see a VF broadcast address for but
>> can see for PF
>>
>> Before:
>> 11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
>> state UP mode DEFAULT group default qlen 256
>> link/infiniband
>> 80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
>> 00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
>> vf 0 MAC 14:80:00:00:66:fe, spoof checking off, link-state disable,
>> trust off, query_rss off
>> ...
>>
>> After:
>> 11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
>> state UP mode DEFAULT group default qlen 256
>> link/infiniband
>> 80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
>> 00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
>> vf 0 link/infiniband
>> 80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
>> 00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff, spoof
>> checking off, link-state disable, trust off, query_rss off
>>
>> Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
>> ---
>> include/uapi/linux/if_link.h | 5 +++++
>> net/core/rtnetlink.c | 6 ++++++
>> 2 files changed, 11 insertions(+)
>>
>> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
>> index 5b225ff63b48..1f36dd3a45d6 100644
>> --- a/include/uapi/linux/if_link.h
>> +++ b/include/uapi/linux/if_link.h
>> @@ -681,6 +681,7 @@ enum {
>> enum {
>> IFLA_VF_UNSPEC,
>> IFLA_VF_MAC, /* Hardware queue specific attributes */
>> + IFLA_VF_BROADCAST,
>> IFLA_VF_VLAN, /* VLAN ID and QoS */
>> IFLA_VF_TX_RATE, /* Max TX Bandwidth Allocation */
>> IFLA_VF_SPOOFCHK, /* Spoof Checking on/off switch */
>
> Oops, I forgot to mention one important point when reviewing v1: the new
> attribute type must be added at the end (just before __IFLA_VF_MAX) so
> that you do not change value of existing IFLA_VF_* constants (this would
> break compatibility).
Right, I've also missed that that the change breaks KABI.
>
>> @@ -704,6 +705,10 @@ struct ifla_vf_mac {
>> __u8 mac[32]; /* MAX_ADDR_LEN */
>> };
>>
>> +struct ifla_vf_broadcast {
>> + __u8 broadcast[32];
>> +};
>> +
>> struct ifla_vf_vlan {
>> __u32 vf;
>> __u32 vlan; /* 0 - 4095, 0 disables VLAN filter */
>
> My first idea was that to question the need of a wrapping structure as
> we couldn't modify that structure in the future anyway so that there
> does not seem to be any gain against simply passing the address as a
> binary with attribute length equal to address length (like we do with
> IFLA_ADDRESS and IFLA_BROADCAST).
>
> But then I checked other IFLA_VF_* attributes and I'm confused. The
> structure seems to be
>
> IFLA_VF_INFO_LIST
> IFLA_VF_INFO
> IFLA_VF_MAC
> IFLA_VF_VLAN
> ...
> IFLA_VF_INFO
> IFLA_VF_MAC
> IFLA_VF_VLAN
> ...
> ...
>
> Each IFLA_VF_INFO corresponds to one virtual function but its number is
> not determined by an attribute within this nest. Instead, each of the
> neste IFLA_VF_* attributes is a structure containing "__u32 vf" and it's
> only matter of convention that within one IFLA_VF_INFO nest, all data
> belongs to the same VF, neither do_setlink() nor do_setvfinfo() check
> it.
>
> I guess you should either follow this weird pattern or introduce proper
> IFLA_VF_ID to be used for IFLA_VF_BROADCAST and all future IFLA_VF_*
> attributes. However, each new attribute makes IFLA_VF_INFO bigger and
> lowers the number of VFs that can be stored in an IFLA_VF_INFO_LIST nest
> without exceeding the hard limit of 65535 bytes so that we cannot afford
> to add too many.
I've just put it as other attrs for now.
>
>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> index cec60583931f..88304212f127 100644
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
> ...
>> @@ -1753,6 +1758,7 @@ static const struct nla_policy
>> ifla_info_policy[IFLA_INFO_MAX+1] = {
>>
>> static const struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = {
>> [IFLA_VF_MAC] = { .len = sizeof(struct ifla_vf_mac) },
>> + [IFLA_VF_BROADCAST] = {. len = sizeof(struct ifla_vf_broadcast) },
>> [IFLA_VF_VLAN] = { .len = sizeof(struct ifla_vf_vlan) },
>> [IFLA_VF_VLAN_LIST] = { .type = NLA_NESTED },
>> [IFLA_VF_TX_RATE] = { .len = sizeof(struct ifla_vf_tx_rate) },
>
> As you do not implement setting the broadcast address (is that possible
> at all?),
According to rfc4391 it's formed from the components like p_key,
q_key, mtu and other.
NLA_REJECT would be more appropriate so that the request isn't
> silently ignored.
Anyway, I've sent v3.
Thanks!
>
> Michal
>
^ 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