* [PATCH v4 rdma-next 1/3] qed*: Change dpi_addr to be denoted with __iomem
From: Michal Kalderon @ 2019-06-24 10:28 UTC (permalink / raw)
To: michal.kalderon, ariel.elior, jgg, dledford; +Cc: linux-rdma, davem, netdev
In-Reply-To: <20190624102809.8793-1-michal.kalderon@marvell.com>
Several casts were required around dpi_addr parameter in qed_rdma_if.h
This is an address on the doorbell bar and should therefore be marked
with __iomem.
Reported-by: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Michal Kalderon <michal.kalderon@marvell.com>
---
drivers/infiniband/hw/qedr/main.c | 2 +-
drivers/infiniband/hw/qedr/qedr.h | 2 +-
drivers/net/ethernet/qlogic/qed/qed_rdma.c | 5 ++---
include/linux/qed/qed_rdma_if.h | 2 +-
4 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/infiniband/hw/qedr/main.c b/drivers/infiniband/hw/qedr/main.c
index a0a7ba0a5af4..3db4b6ba5ad6 100644
--- a/drivers/infiniband/hw/qedr/main.c
+++ b/drivers/infiniband/hw/qedr/main.c
@@ -815,7 +815,7 @@ static int qedr_init_hw(struct qedr_dev *dev)
if (rc)
goto out;
- dev->db_addr = (void __iomem *)(uintptr_t)out_params.dpi_addr;
+ dev->db_addr = out_params.dpi_addr;
dev->db_phys_addr = out_params.dpi_phys_addr;
dev->db_size = out_params.dpi_size;
dev->dpi = out_params.dpi;
diff --git a/drivers/infiniband/hw/qedr/qedr.h b/drivers/infiniband/hw/qedr/qedr.h
index 6175d1e98717..8df56aba9d2c 100644
--- a/drivers/infiniband/hw/qedr/qedr.h
+++ b/drivers/infiniband/hw/qedr/qedr.h
@@ -227,7 +227,7 @@ struct qedr_ucontext {
struct ib_ucontext ibucontext;
struct qedr_dev *dev;
struct qedr_pd *pd;
- u64 dpi_addr;
+ void __iomem *dpi_addr;
u64 dpi_phys_addr;
u32 dpi_size;
u16 dpi;
diff --git a/drivers/net/ethernet/qlogic/qed/qed_rdma.c b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
index 7873d6dfd91f..fb3fe60a1a68 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_rdma.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
@@ -799,9 +799,8 @@ static int qed_rdma_add_user(void *rdma_cxt,
/* Calculate the corresponding DPI address */
dpi_start_offset = p_hwfn->dpi_start_offset;
- out_params->dpi_addr = (u64)((u8 __iomem *)p_hwfn->doorbells +
- dpi_start_offset +
- ((out_params->dpi) * p_hwfn->dpi_size));
+ out_params->dpi_addr = p_hwfn->doorbells + dpi_start_offset +
+ out_params->dpi * p_hwfn->dpi_size;
out_params->dpi_phys_addr = p_hwfn->cdev->db_phys_addr +
dpi_start_offset +
diff --git a/include/linux/qed/qed_rdma_if.h b/include/linux/qed/qed_rdma_if.h
index d15f8e4815e3..834166809a6c 100644
--- a/include/linux/qed/qed_rdma_if.h
+++ b/include/linux/qed/qed_rdma_if.h
@@ -225,7 +225,7 @@ struct qed_rdma_start_in_params {
struct qed_rdma_add_user_out_params {
u16 dpi;
- u64 dpi_addr;
+ void __iomem *dpi_addr;
u64 dpi_phys_addr;
u32 dpi_size;
u16 wid_count;
--
2.14.5
^ permalink raw reply related
* [PATCH v4 rdma-next 2/3] RDMA/qedr: Add doorbell overflow recovery support
From: Michal Kalderon @ 2019-06-24 10:28 UTC (permalink / raw)
To: michal.kalderon, ariel.elior, jgg, dledford; +Cc: linux-rdma, davem, netdev
In-Reply-To: <20190624102809.8793-1-michal.kalderon@marvell.com>
Use the doorbell recovery mechanism to register rdma related doorbells
that will be restored in case there is a doorbell overflow attention.
Signed-off-by: Ariel Elior <ariel.elior@marvell.com>
Signed-off-by: Michal Kalderon <michal.kalderon@marvell.com>
---
drivers/infiniband/hw/qedr/qedr.h | 13 +-
drivers/infiniband/hw/qedr/verbs.c | 351 ++++++++++++++++++++++++++++++-------
include/uapi/rdma/qedr-abi.h | 25 +++
3 files changed, 329 insertions(+), 60 deletions(-)
diff --git a/drivers/infiniband/hw/qedr/qedr.h b/drivers/infiniband/hw/qedr/qedr.h
index 8df56aba9d2c..006712ac1c88 100644
--- a/drivers/infiniband/hw/qedr/qedr.h
+++ b/drivers/infiniband/hw/qedr/qedr.h
@@ -231,7 +231,7 @@ struct qedr_ucontext {
u64 dpi_phys_addr;
u32 dpi_size;
u16 dpi;
-
+ bool db_rec;
struct list_head mm_head;
/* Lock to protect mm list */
@@ -263,6 +263,11 @@ struct qedr_userq {
struct qedr_pbl *pbl_tbl;
u64 buf_addr;
size_t buf_len;
+
+ /* doorbell recovery */
+ void __iomem *db_addr;
+ struct qedr_user_db_rec *db_rec_data;
+ u64 db_rec_phys;
};
struct qedr_cq {
@@ -298,11 +303,17 @@ struct qedr_pd {
struct qedr_ucontext *uctx;
};
+enum qedr_mm_type {
+ QEDR_MM_TYPE_DB_BAR,
+ QEDR_MM_TYPE_DB_REC
+};
+
struct qedr_mm {
struct {
u64 phy_addr;
unsigned long len;
} key;
+ enum qedr_mm_type type;
struct list_head entry;
};
diff --git a/drivers/infiniband/hw/qedr/verbs.c b/drivers/infiniband/hw/qedr/verbs.c
index 3fc7a4e901c3..cbf4cccd174d 100644
--- a/drivers/infiniband/hw/qedr/verbs.c
+++ b/drivers/infiniband/hw/qedr/verbs.c
@@ -257,7 +257,7 @@ int qedr_modify_port(struct ib_device *ibdev, u8 port, int mask,
}
static int qedr_add_mmap(struct qedr_ucontext *uctx, u64 phy_addr,
- unsigned long len)
+ unsigned long len, enum qedr_mm_type type)
{
struct qedr_mm *mm;
@@ -265,6 +265,7 @@ static int qedr_add_mmap(struct qedr_ucontext *uctx, u64 phy_addr,
if (!mm)
return -ENOMEM;
+ mm->type = type;
mm->key.phy_addr = phy_addr;
/* This function might be called with a length which is not a multiple
* of PAGE_SIZE, while the mapping is PAGE_SIZE grained and the kernel
@@ -281,24 +282,26 @@ static int qedr_add_mmap(struct qedr_ucontext *uctx, u64 phy_addr,
mutex_unlock(&uctx->mm_list_lock);
DP_DEBUG(uctx->dev, QEDR_MSG_MISC,
- "added (addr=0x%llx,len=0x%lx) for ctx=%p\n",
+ "added (addr=0x%llx,len=0x%lx,type=%s) for ctx=%p\n",
(unsigned long long)mm->key.phy_addr,
- (unsigned long)mm->key.len, uctx);
+ (unsigned long)mm->key.len,
+ (type == QEDR_MM_TYPE_DB_REC) ? "DB_REC" : "DB_BAR",
+ uctx);
return 0;
}
-static bool qedr_search_mmap(struct qedr_ucontext *uctx, u64 phy_addr,
- unsigned long len)
+static struct qedr_mm *qedr_remove_mmap(struct qedr_ucontext *uctx,
+ u64 phy_addr, unsigned long len)
{
+ struct qedr_mm *mm, *tmp;
bool found = false;
- struct qedr_mm *mm;
mutex_lock(&uctx->mm_list_lock);
- list_for_each_entry(mm, &uctx->mm_head, entry) {
+ list_for_each_entry_safe(mm, tmp, &uctx->mm_head, entry) {
if (len != mm->key.len || phy_addr != mm->key.phy_addr)
continue;
-
+ list_del_init(&mm->entry);
found = true;
break;
}
@@ -307,7 +310,10 @@ static bool qedr_search_mmap(struct qedr_ucontext *uctx, u64 phy_addr,
"searched for (addr=0x%llx,len=0x%lx) for ctx=%p, result=%d\n",
mm->key.phy_addr, mm->key.len, uctx, found);
- return found;
+ if (found)
+ return mm;
+
+ return NULL;
}
int qedr_alloc_ucontext(struct ib_ucontext *uctx, struct ib_udata *udata)
@@ -316,12 +322,24 @@ int qedr_alloc_ucontext(struct ib_ucontext *uctx, struct ib_udata *udata)
int rc;
struct qedr_ucontext *ctx = get_qedr_ucontext(uctx);
struct qedr_alloc_ucontext_resp uresp = {};
+ struct qedr_alloc_ucontext_req ureq = {};
struct qedr_dev *dev = get_qedr_dev(ibdev);
struct qed_rdma_add_user_out_params oparams;
if (!udata)
return -EFAULT;
+ if (udata->inlen) {
+ rc = ib_copy_from_udata(&ureq, udata,
+ min(sizeof(ureq), udata->inlen));
+ if (rc) {
+ DP_ERR(dev, "Problem copying data from user space\n");
+ return -EFAULT;
+ }
+
+ ctx->db_rec = !!(ureq.context_flags & QEDR_ALLOC_UCTX_DB_REC);
+ }
+
rc = dev->ops->rdma_add_user(dev->rdma_ctx, &oparams);
if (rc) {
DP_ERR(dev,
@@ -356,7 +374,8 @@ int qedr_alloc_ucontext(struct ib_ucontext *uctx, struct ib_udata *udata)
ctx->dev = dev;
- rc = qedr_add_mmap(ctx, ctx->dpi_phys_addr, ctx->dpi_size);
+ rc = qedr_add_mmap(ctx, ctx->dpi_phys_addr, ctx->dpi_size,
+ QEDR_MM_TYPE_DB_BAR);
if (rc)
return rc;
@@ -383,6 +402,43 @@ void qedr_dealloc_ucontext(struct ib_ucontext *ibctx)
}
}
+/* Map the doorbell bar */
+int qedr_mmap_db_bar(struct qedr_dev *dev, struct qedr_ucontext *ucontext,
+ struct vm_area_struct *vma, unsigned long dpi_start)
+{
+ unsigned long phys_addr = vma->vm_pgoff << PAGE_SHIFT;
+ unsigned long len = (vma->vm_end - vma->vm_start);
+
+ if (phys_addr < dpi_start ||
+ ((phys_addr + len) > (dpi_start + ucontext->dpi_size))) {
+ DP_ERR(dev,
+ "failed mmap, pages are outside of dpi; page address=0x%pK, dpi_start=0x%pK, dpi_size=0x%x\n",
+ (void *)phys_addr, (void *)dpi_start,
+ ucontext->dpi_size);
+ return -EINVAL;
+ }
+
+ if (vma->vm_flags & VM_READ) {
+ DP_ERR(dev, "failed mmap, cannot map doorbell bar for read\n");
+ return -EINVAL;
+ }
+
+ vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
+
+ return io_remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff, len,
+ vma->vm_page_prot);
+}
+
+/* Map the kernel doorbell recovery memory entry */
+int qedr_mmap_db_rec(struct vm_area_struct *vma)
+{
+ unsigned long len = vma->vm_end - vma->vm_start;
+
+ return remap_pfn_range(vma, vma->vm_start,
+ vma->vm_pgoff,
+ len, vma->vm_page_prot);
+}
+
int qedr_mmap(struct ib_ucontext *context, struct vm_area_struct *vma)
{
struct qedr_ucontext *ucontext = get_qedr_ucontext(context);
@@ -390,6 +446,8 @@ int qedr_mmap(struct ib_ucontext *context, struct vm_area_struct *vma)
unsigned long phys_addr = vma->vm_pgoff << PAGE_SHIFT;
unsigned long len = (vma->vm_end - vma->vm_start);
unsigned long dpi_start;
+ struct qedr_mm *mm;
+ int rc;
dpi_start = dev->db_phys_addr + (ucontext->dpi * ucontext->dpi_size);
@@ -405,29 +463,28 @@ int qedr_mmap(struct ib_ucontext *context, struct vm_area_struct *vma)
return -EINVAL;
}
- if (!qedr_search_mmap(ucontext, phys_addr, len)) {
- DP_ERR(dev, "failed mmap, vm_pgoff=0x%lx is not authorized\n",
+ mm = qedr_remove_mmap(ucontext, phys_addr, len);
+ if (!mm) {
+ DP_ERR(dev, "failed to remove mmap, vm_pgoff=0x%lx\n",
vma->vm_pgoff);
return -EINVAL;
}
- if (phys_addr < dpi_start ||
- ((phys_addr + len) > (dpi_start + ucontext->dpi_size))) {
- DP_ERR(dev,
- "failed mmap, pages are outside of dpi; page address=0x%pK, dpi_start=0x%pK, dpi_size=0x%x\n",
- (void *)phys_addr, (void *)dpi_start,
- ucontext->dpi_size);
- return -EINVAL;
+ switch (mm->type) {
+ case QEDR_MM_TYPE_DB_BAR:
+ rc = qedr_mmap_db_bar(dev, ucontext, vma, dpi_start);
+ break;
+ case QEDR_MM_TYPE_DB_REC:
+ rc = qedr_mmap_db_rec(vma);
+ break;
+ default:
+ rc = -EINVAL;
+ break;
}
- if (vma->vm_flags & VM_READ) {
- DP_ERR(dev, "failed mmap, cannot map doorbell bar for read\n");
- return -EINVAL;
- }
+ kfree(mm);
- vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
- return io_remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff, len,
- vma->vm_page_prot);
+ return rc;
}
int qedr_alloc_pd(struct ib_pd *ibpd, struct ib_udata *udata)
@@ -657,16 +714,48 @@ static void qedr_populate_pbls(struct qedr_dev *dev, struct ib_umem *umem,
}
}
+static int qedr_db_recovery_add(struct qedr_dev *dev,
+ void __iomem *db_addr,
+ void *db_data,
+ enum qed_db_rec_width db_width,
+ enum qed_db_rec_space db_space)
+{
+ if (!db_data) {
+ DP_DEBUG(dev, QEDR_MSG_INIT, "avoiding db rec since old lib\n");
+ return 0;
+ }
+
+ return dev->ops->common->db_recovery_add(dev->cdev, db_addr, db_data,
+ db_width, db_space);
+}
+
+static void qedr_db_recovery_del(struct qedr_dev *dev,
+ void __iomem *db_addr,
+ void *db_data)
+{
+ if (!db_data) {
+ DP_DEBUG(dev, QEDR_MSG_INIT, "avoiding db rec since old lib\n");
+ return;
+ }
+
+ /* Ignore return code as there is not much we can do about it. Error
+ * log will be printed inside.
+ */
+ dev->ops->common->db_recovery_del(dev->cdev, db_addr, db_data);
+}
+
static int qedr_copy_cq_uresp(struct qedr_dev *dev,
- struct qedr_cq *cq, struct ib_udata *udata)
+ struct qedr_cq *cq, struct ib_udata *udata,
+ u32 db_offset)
{
struct qedr_create_cq_uresp uresp;
int rc;
memset(&uresp, 0, sizeof(uresp));
- uresp.db_offset = DB_ADDR_SHIFT(DQ_PWM_OFFSET_UCM_RDMA_CQ_CONS_32BIT);
+ uresp.db_offset = db_offset;
uresp.icid = cq->icid;
+ uresp.db_rec_addr = cq->q.db_rec_phys;
rc = qedr_ib_copy_to_udata(udata, &uresp, sizeof(uresp));
if (rc)
@@ -694,10 +783,36 @@ static inline int qedr_align_cq_entries(int entries)
return aligned_size / QEDR_CQE_SIZE;
}
+static int qedr_init_user_db_rec(struct ib_udata *udata,
+ struct qedr_dev *dev, struct qedr_userq *q,
+ bool requires_db_rec)
+{
+ struct qedr_ucontext *uctx =
+ rdma_udata_to_drv_context(udata, struct qedr_ucontext,
+ ibucontext);
+
+ /* Aborting for non doorbell userqueue (SRQ) or non-supporting lib */
+ if (requires_db_rec == 0 || !uctx->db_rec)
+ return 0;
+
+ /* Allocate a page for doorbell recovery, add to mmap ) */
+ q->db_rec_data = (void *)get_zeroed_page(GFP_KERNEL);
+ if (!q->db_rec_data) {
+ DP_ERR(dev,
+ "get_free_page failed\n");
+ return -ENOMEM;
+ }
+
+ q->db_rec_phys = virt_to_phys(q->db_rec_data);
+ return qedr_add_mmap(uctx, q->db_rec_phys, PAGE_SIZE,
+ QEDR_MM_TYPE_DB_REC);
+}
+
static inline int qedr_init_user_queue(struct ib_udata *udata,
struct qedr_dev *dev,
struct qedr_userq *q, u64 buf_addr,
- size_t buf_len, int access, int dmasync,
+ size_t buf_len, bool requires_db_rec,
+ int access, int dmasync,
int alloc_and_init)
{
u32 fw_pages;
@@ -735,7 +850,8 @@ static inline int qedr_init_user_queue(struct ib_udata *udata,
}
}
- return 0;
+ /* mmap the user address used to store doorbell data for recovery */
+ return qedr_init_user_db_rec(udata, dev, q, requires_db_rec);
err0:
ib_umem_release(q->umem);
@@ -821,6 +937,7 @@ int qedr_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
int entries = attr->cqe;
struct qedr_cq *cq = get_qedr_cq(ibcq);
int chain_entries;
+ u32 db_offset;
int page_cnt;
u64 pbl_ptr;
u16 icid;
@@ -840,8 +957,12 @@ int qedr_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
chain_entries = qedr_align_cq_entries(entries);
chain_entries = min_t(int, chain_entries, QEDR_MAX_CQES);
+ /* calc db offset. user will add DPI base, kernel will add db addr */
+ db_offset = DB_ADDR_SHIFT(DQ_PWM_OFFSET_UCM_RDMA_CQ_CONS_32BIT);
+
if (udata) {
- if (ib_copy_from_udata(&ureq, udata, sizeof(ureq))) {
+ if (ib_copy_from_udata(&ureq, udata, min(sizeof(ureq),
+ udata->inlen))) {
DP_ERR(dev,
"create cq: problem copying data from user space\n");
goto err0;
@@ -856,8 +977,9 @@ int qedr_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
cq->cq_type = QEDR_CQ_TYPE_USER;
rc = qedr_init_user_queue(udata, dev, &cq->q, ureq.addr,
- ureq.len, IB_ACCESS_LOCAL_WRITE, 1,
- 1);
+ ureq.len, true,
+ IB_ACCESS_LOCAL_WRITE,
+ 1, 1);
if (rc)
goto err0;
@@ -865,6 +987,7 @@ int qedr_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
page_cnt = cq->q.pbl_info.num_pbes;
cq->ibcq.cqe = chain_entries;
+ cq->q.db_addr = ctx->dpi_addr + db_offset;
} else {
cq->cq_type = QEDR_CQ_TYPE_KERNEL;
@@ -895,14 +1018,21 @@ int qedr_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
spin_lock_init(&cq->cq_lock);
if (udata) {
- rc = qedr_copy_cq_uresp(dev, cq, udata);
+ rc = qedr_copy_cq_uresp(dev, cq, udata, db_offset);
if (rc)
goto err3;
+
+ rc = qedr_db_recovery_add(dev, cq->q.db_addr,
+ &cq->q.db_rec_data->db_data,
+ DB_REC_WIDTH_64B,
+ DB_REC_USER);
+ if (rc)
+ goto err3;
+
} else {
/* Generate doorbell address. */
- cq->db_addr = dev->db_addr +
- DB_ADDR_SHIFT(DQ_PWM_OFFSET_UCM_RDMA_CQ_CONS_32BIT);
cq->db.data.icid = cq->icid;
+ cq->db_addr = dev->db_addr + db_offset;
cq->db.data.params = DB_AGG_CMD_SET <<
RDMA_PWM_VAL32_DATA_AGG_CMD_SHIFT;
@@ -912,6 +1042,11 @@ int qedr_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
cq->latest_cqe = NULL;
consume_cqe(cq);
cq->cq_cons = qed_chain_get_cons_idx_u32(&cq->pbl);
+
+ rc = qedr_db_recovery_add(dev, cq->db_addr, &cq->db.data,
+ DB_REC_WIDTH_64B, DB_REC_KERNEL);
+ if (rc)
+ goto err3;
}
DP_DEBUG(dev, QEDR_MSG_CQ,
@@ -930,8 +1065,16 @@ int qedr_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
else
dev->ops->common->chain_free(dev->cdev, &cq->pbl);
err1:
- if (udata)
+ if (udata) {
ib_umem_release(cq->q.umem);
+ if (cq->q.db_rec_data) {
+ qedr_db_recovery_del(dev, cq->q.db_addr,
+ &cq->q.db_rec_data->db_data);
+ free_page((unsigned long)cq->q.db_rec_data);
+ }
+ } else {
+ qedr_db_recovery_del(dev, cq->db_addr, &cq->db.data);
+ }
err0:
return -EINVAL;
}
@@ -962,8 +1105,10 @@ void qedr_destroy_cq(struct ib_cq *ibcq, struct ib_udata *udata)
cq->destroyed = 1;
/* GSIs CQs are handled by driver, so they don't exist in the FW */
- if (cq->cq_type == QEDR_CQ_TYPE_GSI)
+ if (cq->cq_type == QEDR_CQ_TYPE_GSI) {
+ qedr_db_recovery_del(dev, cq->db_addr, &cq->db.data);
return;
+ }
iparams.icid = cq->icid;
dev->ops->rdma_destroy_cq(dev->rdma_ctx, &iparams, &oparams);
@@ -972,6 +1117,14 @@ void qedr_destroy_cq(struct ib_cq *ibcq, struct ib_udata *udata)
if (udata) {
qedr_free_pbl(dev, &cq->q.pbl_info, cq->q.pbl_tbl);
ib_umem_release(cq->q.umem);
+
+ if (cq->q.db_rec_data) {
+ qedr_db_recovery_del(dev, cq->q.db_addr,
+ &cq->q.db_rec_data->db_data);
+ free_page((unsigned long)cq->q.db_rec_data);
+ }
+ } else {
+ qedr_db_recovery_del(dev, cq->db_addr, &cq->db.data);
}
/* We don't want the IRQ handler to handle a non-existing CQ so we
@@ -1150,6 +1303,7 @@ static void qedr_copy_rq_uresp(struct qedr_dev *dev,
}
uresp->rq_icid = qp->icid;
+ uresp->rq_db_rec_addr = qp->urq.db_rec_phys;
}
static void qedr_copy_sq_uresp(struct qedr_dev *dev,
@@ -1163,22 +1317,24 @@ static void qedr_copy_sq_uresp(struct qedr_dev *dev,
uresp->sq_icid = qp->icid;
else
uresp->sq_icid = qp->icid + 1;
+
+ uresp->sq_db_rec_addr = qp->usq.db_rec_phys;
}
static int qedr_copy_qp_uresp(struct qedr_dev *dev,
- struct qedr_qp *qp, struct ib_udata *udata)
+ struct qedr_qp *qp, struct ib_udata *udata,
+ struct qedr_create_qp_uresp *uresp)
{
- struct qedr_create_qp_uresp uresp;
int rc;
- memset(&uresp, 0, sizeof(uresp));
- qedr_copy_sq_uresp(dev, &uresp, qp);
- qedr_copy_rq_uresp(dev, &uresp, qp);
+ memset(uresp, 0, sizeof(*uresp));
+ qedr_copy_sq_uresp(dev, uresp, qp);
+ qedr_copy_rq_uresp(dev, uresp, qp);
- uresp.atomic_supported = dev->atomic_cap != IB_ATOMIC_NONE;
- uresp.qp_id = qp->qp_id;
+ uresp->atomic_supported = dev->atomic_cap != IB_ATOMIC_NONE;
+ uresp->qp_id = qp->qp_id;
- rc = qedr_ib_copy_to_udata(udata, &uresp, sizeof(uresp));
+ rc = qedr_ib_copy_to_udata(udata, uresp, sizeof(*uresp));
if (rc)
DP_ERR(dev,
"create qp: failed a copy to user space with qp icid=0x%x.\n",
@@ -1222,16 +1378,35 @@ static void qedr_set_common_qp_params(struct qedr_dev *dev,
qp->sq.max_sges, qp->sq_cq->icid);
}
-static void qedr_set_roce_db_info(struct qedr_dev *dev, struct qedr_qp *qp)
+static int qedr_set_roce_db_info(struct qedr_dev *dev, struct qedr_qp *qp)
{
+ int rc;
+
qp->sq.db = dev->db_addr +
DB_ADDR_SHIFT(DQ_PWM_OFFSET_XCM_RDMA_SQ_PROD);
qp->sq.db_data.data.icid = qp->icid + 1;
+ rc = qedr_db_recovery_add(dev, qp->sq.db,
+ &qp->sq.db_data,
+ DB_REC_WIDTH_32B,
+ DB_REC_KERNEL);
+ if (rc)
+ return rc;
+
if (!qp->srq) {
qp->rq.db = dev->db_addr +
DB_ADDR_SHIFT(DQ_PWM_OFFSET_TCM_ROCE_RQ_PROD);
qp->rq.db_data.data.icid = qp->icid;
+
+ rc = qedr_db_recovery_add(dev, qp->rq.db,
+ &qp->rq.db_data,
+ DB_REC_WIDTH_32B,
+ DB_REC_KERNEL);
+ if (rc)
+ qedr_db_recovery_del(dev, qp->sq.db,
+ &qp->sq.db_data);
}
+
+ return rc;
}
static int qedr_check_srq_params(struct qedr_dev *dev,
@@ -1285,7 +1460,7 @@ static int qedr_init_srq_user_params(struct ib_udata *udata,
int rc;
rc = qedr_init_user_queue(udata, srq->dev, &srq->usrq, ureq->srq_addr,
- ureq->srq_len, access, dmasync, 1);
+ ureq->srq_len, false, access, dmasync, 1);
if (rc)
return rc;
@@ -1381,7 +1556,8 @@ int qedr_create_srq(struct ib_srq *ibsrq, struct ib_srq_init_attr *init_attr,
hw_srq->max_sges = init_attr->attr.max_sge;
if (udata) {
- if (ib_copy_from_udata(&ureq, udata, sizeof(ureq))) {
+ if (ib_copy_from_udata(&ureq, udata, min(sizeof(ureq),
+ udata->inlen))) {
DP_ERR(dev,
"create srq: problem copying data from user space\n");
goto err0;
@@ -1579,6 +1755,18 @@ static void qedr_cleanup_user(struct qedr_dev *dev, struct qedr_qp *qp)
if (qp->urq.umem)
ib_umem_release(qp->urq.umem);
qp->urq.umem = NULL;
+
+ if (qp->usq.db_rec_data) {
+ qedr_db_recovery_del(dev, qp->usq.db_addr,
+ &qp->usq.db_rec_data->db_data);
+ free_page((unsigned long)qp->usq.db_rec_data);
+ }
+
+ if (qp->urq.db_rec_data) {
+ qedr_db_recovery_del(dev, qp->urq.db_addr,
+ &qp->urq.db_rec_data->db_data);
+ free_page((unsigned long)qp->urq.db_rec_data);
+ }
}
static int qedr_create_user_qp(struct qedr_dev *dev,
@@ -1590,12 +1778,14 @@ static int qedr_create_user_qp(struct qedr_dev *dev,
struct qed_rdma_create_qp_in_params in_params;
struct qed_rdma_create_qp_out_params out_params;
struct qedr_pd *pd = get_qedr_pd(ibpd);
+ struct qedr_create_qp_uresp uresp;
+ struct qedr_ucontext *ctx = NULL;
struct qedr_create_qp_ureq ureq;
int alloc_and_init = rdma_protocol_roce(&dev->ibdev, 1);
int rc = -EINVAL;
memset(&ureq, 0, sizeof(ureq));
- rc = ib_copy_from_udata(&ureq, udata, sizeof(ureq));
+ rc = ib_copy_from_udata(&ureq, udata, min(sizeof(ureq), udata->inlen));
if (rc) {
DP_ERR(dev, "Problem copying data from user space\n");
return rc;
@@ -1603,14 +1793,16 @@ static int qedr_create_user_qp(struct qedr_dev *dev,
/* SQ - read access only (0), dma sync not required (0) */
rc = qedr_init_user_queue(udata, dev, &qp->usq, ureq.sq_addr,
- ureq.sq_len, 0, 0, alloc_and_init);
+ ureq.sq_len, true, 0, 0,
+ alloc_and_init);
if (rc)
return rc;
if (!qp->srq) {
/* RQ - read access only (0), dma sync not required (0) */
rc = qedr_init_user_queue(udata, dev, &qp->urq, ureq.rq_addr,
- ureq.rq_len, 0, 0, alloc_and_init);
+ ureq.rq_len, true,
+ 0, 0, alloc_and_init);
if (rc)
return rc;
}
@@ -1640,13 +1832,31 @@ static int qedr_create_user_qp(struct qedr_dev *dev,
qp->qp_id = out_params.qp_id;
qp->icid = out_params.icid;
- rc = qedr_copy_qp_uresp(dev, qp, udata);
+ rc = qedr_copy_qp_uresp(dev, qp, udata, &uresp);
if (rc)
goto err;
+ /* db offset was calculated in copy_qp_uresp, now set in the user q */
+ ctx = pd->uctx;
+ qp->usq.db_addr = ctx->dpi_addr + uresp.sq_db_offset;
+ qp->urq.db_addr = ctx->dpi_addr + uresp.rq_db_offset;
+
+ rc = qedr_db_recovery_add(dev, qp->usq.db_addr,
+ &qp->usq.db_rec_data->db_data,
+ DB_REC_WIDTH_32B,
+ DB_REC_USER);
+ if (rc)
+ goto err;
+
+ rc = qedr_db_recovery_add(dev, qp->urq.db_addr,
+ &qp->urq.db_rec_data->db_data,
+ DB_REC_WIDTH_32B,
+ DB_REC_USER);
+ if (rc)
+ goto err;
qedr_qp_user_print(dev, qp);
- return 0;
+ return rc;
err:
rc = dev->ops->rdma_destroy_qp(dev->rdma_ctx, qp->qed_qp);
if (rc)
@@ -1657,12 +1867,21 @@ static int qedr_create_user_qp(struct qedr_dev *dev,
return rc;
}
-static void qedr_set_iwarp_db_info(struct qedr_dev *dev, struct qedr_qp *qp)
+static int qedr_set_iwarp_db_info(struct qedr_dev *dev, struct qedr_qp *qp)
{
+ int rc;
+
qp->sq.db = dev->db_addr +
DB_ADDR_SHIFT(DQ_PWM_OFFSET_XCM_RDMA_SQ_PROD);
qp->sq.db_data.data.icid = qp->icid;
+ rc = qedr_db_recovery_add(dev, qp->sq.db,
+ &qp->sq.db_data,
+ DB_REC_WIDTH_32B,
+ DB_REC_KERNEL);
+ if (rc)
+ return rc;
+
qp->rq.db = dev->db_addr +
DB_ADDR_SHIFT(DQ_PWM_OFFSET_TCM_IWARP_RQ_PROD);
qp->rq.db_data.data.icid = qp->icid;
@@ -1670,6 +1889,13 @@ static void qedr_set_iwarp_db_info(struct qedr_dev *dev, struct qedr_qp *qp)
DB_ADDR_SHIFT(DQ_PWM_OFFSET_TCM_FLAGS);
qp->rq.iwarp_db2_data.data.icid = qp->icid;
qp->rq.iwarp_db2_data.data.value = DQ_TCM_IWARP_POST_RQ_CF_CMD;
+
+ rc = qedr_db_recovery_add(dev, qp->rq.db,
+ &qp->rq.db_data,
+ DB_REC_WIDTH_32B,
+ DB_REC_KERNEL);
+
+ return rc;
}
static int
@@ -1717,8 +1943,7 @@ qedr_roce_create_kernel_qp(struct qedr_dev *dev,
qp->qp_id = out_params.qp_id;
qp->icid = out_params.icid;
- qedr_set_roce_db_info(dev, qp);
- return rc;
+ return qedr_set_roce_db_info(dev, qp);
}
static int
@@ -1776,8 +2001,7 @@ qedr_iwarp_create_kernel_qp(struct qedr_dev *dev,
qp->qp_id = out_params.qp_id;
qp->icid = out_params.icid;
- qedr_set_iwarp_db_info(dev, qp);
- return rc;
+ return qedr_set_iwarp_db_info(dev, qp);
err:
dev->ops->rdma_destroy_qp(dev->rdma_ctx, qp->qed_qp);
@@ -1792,6 +2016,15 @@ static void qedr_cleanup_kernel(struct qedr_dev *dev, struct qedr_qp *qp)
dev->ops->common->chain_free(dev->cdev, &qp->rq.pbl);
kfree(qp->rqe_wr_id);
+
+ /* GSI qp is not registered to db mechanism so no need to delete */
+ if (qp->qp_type == IB_QPT_GSI)
+ return;
+
+ qedr_db_recovery_del(dev, qp->sq.db, &qp->sq.db_data);
+
+ if (!qp->srq)
+ qedr_db_recovery_del(dev, qp->rq.db, &qp->rq.db_data);
}
static int qedr_create_kernel_qp(struct qedr_dev *dev,
diff --git a/include/uapi/rdma/qedr-abi.h b/include/uapi/rdma/qedr-abi.h
index 7a10b3a325fa..aa06dfa1a031 100644
--- a/include/uapi/rdma/qedr-abi.h
+++ b/include/uapi/rdma/qedr-abi.h
@@ -38,6 +38,15 @@
#define QEDR_ABI_VERSION (8)
/* user kernel communication data structures. */
+enum qedr_alloc_ucontext_flags {
+ QEDR_ALLOC_UCTX_RESERVED = 1 << 0,
+ QEDR_ALLOC_UCTX_DB_REC = 1 << 1
+};
+
+struct qedr_alloc_ucontext_req {
+ __u32 context_flags;
+ __u32 reserved;
+};
struct qedr_alloc_ucontext_resp {
__aligned_u64 db_pa;
@@ -74,6 +83,7 @@ struct qedr_create_cq_uresp {
__u32 db_offset;
__u16 icid;
__u16 reserved;
+ __u64 db_rec_addr;
};
struct qedr_create_qp_ureq {
@@ -109,6 +119,13 @@ struct qedr_create_qp_uresp {
__u32 rq_db2_offset;
__u32 reserved;
+
+ /* address of SQ doorbell recovery user entry */
+ __u64 sq_db_rec_addr;
+
+ /* address of RQ doorbell recovery user entry */
+ __u64 rq_db_rec_addr;
+
};
struct qedr_create_srq_ureq {
@@ -128,4 +145,12 @@ struct qedr_create_srq_uresp {
__u32 reserved1;
};
+/* doorbell recovery entry allocated and populated by userspace doorbelling
+ * entities and mapped to kernel. Kernel uses this to register doorbell
+ * information with doorbell drop recovery mechanism.
+ */
+struct qedr_user_db_rec {
+ __aligned_u64 db_data; /* doorbell data */
+};
+
#endif /* __QEDR_USER_H__ */
--
2.14.5
^ permalink raw reply related
* [PATCH v4 rdma-next 3/3] RDMA/qedr: Add iWARP doorbell recovery support
From: Michal Kalderon @ 2019-06-24 10:28 UTC (permalink / raw)
To: michal.kalderon, ariel.elior, jgg, dledford; +Cc: linux-rdma, davem, netdev
In-Reply-To: <20190624102809.8793-1-michal.kalderon@marvell.com>
This patch adds the iWARP specific doorbells to the doorbell
recovery mechanism
Signed-off-by: Ariel Elior <ariel.elior@marvell.com>
Signed-off-by: Michal Kalderon <michal.kalderon@marvell.com>
---
drivers/infiniband/hw/qedr/qedr.h | 12 +++++++-----
drivers/infiniband/hw/qedr/verbs.c | 37 ++++++++++++++++++++++++++++++++++++-
2 files changed, 43 insertions(+), 6 deletions(-)
diff --git a/drivers/infiniband/hw/qedr/qedr.h b/drivers/infiniband/hw/qedr/qedr.h
index 006712ac1c88..6c5524d6b04e 100644
--- a/drivers/infiniband/hw/qedr/qedr.h
+++ b/drivers/infiniband/hw/qedr/qedr.h
@@ -238,6 +238,11 @@ struct qedr_ucontext {
struct mutex mm_list_lock;
};
+union db_prod32 {
+ struct rdma_pwm_val16_data data;
+ u32 raw;
+};
+
union db_prod64 {
struct rdma_pwm_val32_data data;
u64 raw;
@@ -268,6 +273,8 @@ struct qedr_userq {
void __iomem *db_addr;
struct qedr_user_db_rec *db_rec_data;
u64 db_rec_phys;
+ void __iomem *db_rec_db2_addr;
+ union db_prod32 db_rec_db2_data;
};
struct qedr_cq {
@@ -317,11 +324,6 @@ struct qedr_mm {
struct list_head entry;
};
-union db_prod32 {
- struct rdma_pwm_val16_data data;
- u32 raw;
-};
-
struct qedr_qp_hwq_info {
/* WQE Elements */
struct qed_chain pbl;
diff --git a/drivers/infiniband/hw/qedr/verbs.c b/drivers/infiniband/hw/qedr/verbs.c
index cbf4cccd174d..c15196ce9662 100644
--- a/drivers/infiniband/hw/qedr/verbs.c
+++ b/drivers/infiniband/hw/qedr/verbs.c
@@ -1767,6 +1767,10 @@ static void qedr_cleanup_user(struct qedr_dev *dev, struct qedr_qp *qp)
&qp->urq.db_rec_data->db_data);
free_page((unsigned long)qp->urq.db_rec_data);
}
+
+ if (rdma_protocol_iwarp(&dev->ibdev, 1))
+ qedr_db_recovery_del(dev, qp->urq.db_rec_db2_addr,
+ &qp->urq.db_rec_db2_data);
}
static int qedr_create_user_qp(struct qedr_dev *dev,
@@ -1841,6 +1845,17 @@ static int qedr_create_user_qp(struct qedr_dev *dev,
qp->usq.db_addr = ctx->dpi_addr + uresp.sq_db_offset;
qp->urq.db_addr = ctx->dpi_addr + uresp.rq_db_offset;
+ if (rdma_protocol_iwarp(&dev->ibdev, 1)) {
+ qp->urq.db_rec_db2_addr = ctx->dpi_addr + uresp.rq_db2_offset;
+
+ /* calculate the db_rec_db2 data since it is constant so no
+ * need to reflect from user
+ */
+ qp->urq.db_rec_db2_data.data.icid = cpu_to_le16(qp->icid);
+ qp->urq.db_rec_db2_data.data.value =
+ cpu_to_le16(DQ_TCM_IWARP_POST_RQ_CF_CMD);
+ }
+
rc = qedr_db_recovery_add(dev, qp->usq.db_addr,
&qp->usq.db_rec_data->db_data,
DB_REC_WIDTH_32B,
@@ -1854,6 +1869,15 @@ static int qedr_create_user_qp(struct qedr_dev *dev,
DB_REC_USER);
if (rc)
goto err;
+
+ if (rdma_protocol_iwarp(&dev->ibdev, 1)) {
+ rc = qedr_db_recovery_add(dev, qp->urq.db_rec_db2_addr,
+ &qp->urq.db_rec_db2_data,
+ DB_REC_WIDTH_32B,
+ DB_REC_USER);
+ if (rc)
+ goto err;
+ }
qedr_qp_user_print(dev, qp);
return rc;
@@ -1894,7 +1918,13 @@ static int qedr_set_iwarp_db_info(struct qedr_dev *dev, struct qedr_qp *qp)
&qp->rq.db_data,
DB_REC_WIDTH_32B,
DB_REC_KERNEL);
+ if (rc)
+ return rc;
+ rc = qedr_db_recovery_add(dev, qp->rq.iwarp_db2,
+ &qp->rq.iwarp_db2_data,
+ DB_REC_WIDTH_32B,
+ DB_REC_KERNEL);
return rc;
}
@@ -2023,8 +2053,13 @@ static void qedr_cleanup_kernel(struct qedr_dev *dev, struct qedr_qp *qp)
qedr_db_recovery_del(dev, qp->sq.db, &qp->sq.db_data);
- if (!qp->srq)
+ if (!qp->srq) {
qedr_db_recovery_del(dev, qp->rq.db, &qp->rq.db_data);
+
+ if (rdma_protocol_iwarp(&dev->ibdev, 1))
+ qedr_db_recovery_del(dev, qp->rq.iwarp_db2,
+ &qp->rq.iwarp_db2_data);
+ }
}
static int qedr_create_kernel_qp(struct qedr_dev *dev,
--
2.14.5
^ permalink raw reply related
* RE: [EXT] Re: [PATCH v3 rdma-next 0/3] RDMA/qedr: Use the doorbell overflow recovery mechanism for RDMA
From: Michal Kalderon @ 2019-06-24 10:36 UTC (permalink / raw)
To: Jason Gunthorpe, Doug Ledford
Cc: Ariel Elior, linux-rdma@vger.kernel.org, davem@davemloft.net,
netdev@vger.kernel.org
In-Reply-To: <20190621195818.GY19891@ziepe.ca>
> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Friday, June 21, 2019 10:58 PM
>
> External Email
>
> ----------------------------------------------------------------------
> On Fri, Jun 21, 2019 at 07:49:39PM +0000, Michal Kalderon wrote:
> > > From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> > > owner@vger.kernel.org> On Behalf Of Doug Ledford
> > >
> > > On Thu, 2019-06-13 at 11:38 +0300, Michal Kalderon wrote:
> > > > This patch series used the doorbell overflow recovery mechanism
> > > > introduced in commit 36907cd5cd72 ("qed: Add doorbell overflow
> > > > recovery mechanism") for rdma ( RoCE and iWARP )
> > > >
> > > > rdma-core pull request #493
> > > >
> > > > Changes from V2:
> > > > - Don't use long-lived kmap. Instead use user-trigger mmap for the
> > > > doorbell recovery entries.
> > > > - Modify dpi_addr to be denoted with __iomem and avoid redundant
> > > > casts
> > > >
> > > > Changes from V1:
> > > > - call kmap to map virtual address into kernel space
> > > > - modify db_rec_delete to be void
> > > > - remove some cpu_to_le16 that were added to previous patch which
> are
> > > > correct but not related to the overflow recovery mechanism. Will be
> > > > submitted as part of a different patch
> > > >
> > > >
> > > > Michal Kalderon (3):
> > > > qed*: Change dpi_addr to be denoted with __iomem
> > > > RDMA/qedr: Add doorbell overflow recovery support
> > > > RDMA/qedr: Add iWARP doorbell recovery support
> > > >
> > > > drivers/infiniband/hw/qedr/main.c | 2 +-
> > > > drivers/infiniband/hw/qedr/qedr.h | 27 +-
> > > > drivers/infiniband/hw/qedr/verbs.c | 387
> > > > ++++++++++++++++++++++++-----
> > > > drivers/net/ethernet/qlogic/qed/qed_rdma.c | 6 +-
> > > > include/linux/qed/qed_rdma_if.h | 2 +-
> > > > include/uapi/rdma/qedr-abi.h | 25 ++
> > > > 6 files changed, 378 insertions(+), 71 deletions(-)
> > > >
> > >
> > > Hi Michal,
> > >
> > > In patch 2 and 3 both, you still have quite a few casts to (u8 __iomem *).
> > > Why not just define the struct elements as u8 __iomem * instead of
> > > void __iomem * and avoid all the casts?
> > >
> > Hi Doug,
> >
> > Thanks for the review. The remaining casts are due to pointer
> > arithmetic and not variable assignments as before. Removing the cast
> > entirely will require quite a lot of changes in qed and in rdma-core which I
> would be happy to avoid at this time.
>
> In linux pointer math on a void * acts the same as a u8 so you should never
> need to cast a void * to a u8 just to do math?
>
Ok, thanks. Sent v4.
Michal
> Jason
^ permalink raw reply
* Re: [PATCH rdma-next v1 08/12] IB/mlx5: Introduce MLX5_IB_OBJECT_DEVX_ASYNC_EVENT_FD
From: Jason Gunthorpe @ 2019-06-24 11:51 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Yishai Hadas,
Saeed Mahameed, linux-netdev
In-Reply-To: <20190618171540.11729-9-leon@kernel.org>
On Tue, Jun 18, 2019 at 08:15:36PM +0300, Leon Romanovsky wrote:
> From: Yishai Hadas <yishaih@mellanox.com>
>
> Introduce MLX5_IB_OBJECT_DEVX_ASYNC_EVENT_FD and its initial
> implementation.
>
> This object is from type class FD and will be used to read DEVX
> async events.
>
> Signed-off-by: Yishai Hadas <yishaih@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> drivers/infiniband/hw/mlx5/devx.c | 112 ++++++++++++++++++++--
> include/uapi/rdma/mlx5_user_ioctl_cmds.h | 10 ++
> include/uapi/rdma/mlx5_user_ioctl_verbs.h | 4 +
> 3 files changed, 116 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/infiniband/hw/mlx5/devx.c b/drivers/infiniband/hw/mlx5/devx.c
> index 80b42d069328..1815ce0f8daf 100644
> +++ b/drivers/infiniband/hw/mlx5/devx.c
> @@ -33,6 +33,24 @@ struct devx_async_data {
> struct mlx5_ib_uapi_devx_async_cmd_hdr hdr;
> };
>
> +struct devx_async_event_queue {
It seems to be a mistake to try and re-use the async_event_queue for
both cmd and event, as they use it very differently and don't even
store the same things in the event_list. I think it is bettter to just
inline this into devx_async_event_file (and inline the old struct in
the cmd file
> + spinlock_t lock;
> + wait_queue_head_t poll_wait;
> + struct list_head event_list;
> + atomic_t bytes_in_use;
> + u8 is_destroyed:1;
> + u32 flags;
> +};
All the flags testing is ugly, why not just add another bitfield?
> +
> +struct devx_async_event_file {
> + struct ib_uobject uobj;
> + struct list_head subscribed_events_list; /* Head of events that are
> + * subscribed to this FD
> + */
> + struct devx_async_event_queue ev_queue;
> + struct mlx5_ib_dev *dev;
> +};
> +
Crazy indenting
> diff --git a/include/uapi/rdma/mlx5_user_ioctl_verbs.h b/include/uapi/rdma/mlx5_user_ioctl_verbs.h
> index a8f34c237458..57beea4589e4 100644
> +++ b/include/uapi/rdma/mlx5_user_ioctl_verbs.h
> @@ -63,5 +63,9 @@ enum mlx5_ib_uapi_dm_type {
> MLX5_IB_UAPI_DM_TYPE_HEADER_MODIFY_SW_ICM,
> };
>
> +enum mlx5_ib_uapi_devx_create_event_channel_flags {
> + MLX5_IB_UAPI_DEVX_CREATE_EVENT_CHANNEL_FLAGS_OMIT_EV_DATA = 1
> << 0,
Maybe this name is too long
Jason
^ permalink raw reply
* Re: [PATCH rdma-next v1 09/12] IB/mlx5: Register DEVX with mlx5_core to get async events
From: Jason Gunthorpe @ 2019-06-24 11:52 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Yishai Hadas,
Saeed Mahameed, linux-netdev
In-Reply-To: <20190618171540.11729-10-leon@kernel.org>
On Tue, Jun 18, 2019 at 08:15:37PM +0300, Leon Romanovsky wrote:
> void __mlx5_ib_remove(struct mlx5_ib_dev *dev,
> diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> index 9cf23ae6324e..556af34b788b 100644
> +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> @@ -944,6 +944,13 @@ struct mlx5_ib_pf_eq {
> mempool_t *pool;
> };
>
> +struct mlx5_devx_event_table {
> + struct mlx5_nb devx_nb;
> + /* serialize updating the event_xa */
> + struct mutex event_xa_lock;
> + struct xarray event_xa;
> +};
> +
> struct mlx5_ib_dev {
> struct ib_device ib_dev;
> struct mlx5_core_dev *mdev;
> @@ -994,6 +1001,7 @@ struct mlx5_ib_dev {
> struct mlx5_srq_table srq_table;
> struct mlx5_async_ctx async_ctx;
> int free_port;
> + struct mlx5_devx_event_table devx_event_table;
I really question if adding all these structs really does anything for
readability..
Jason
^ permalink raw reply
* Re: [PATCH rdma-next v1 10/12] IB/mlx5: Enable subscription for device events over DEVX
From: Jason Gunthorpe @ 2019-06-24 11:57 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Yishai Hadas,
Saeed Mahameed, linux-netdev
In-Reply-To: <20190618171540.11729-11-leon@kernel.org>
On Tue, Jun 18, 2019 at 08:15:38PM +0300, Leon Romanovsky wrote:
> From: Yishai Hadas <yishaih@mellanox.com>
>
> Enable subscription for device events over DEVX.
>
> Each subscription is added to the two level XA data structure according
> to its event number and the DEVX object information in case was given
> with the given target fd.
>
> Those events will be reported over the given fd once will occur.
> Downstream patches will mange the dispatching to any subscription.
>
> Signed-off-by: Yishai Hadas <yishaih@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> drivers/infiniband/hw/mlx5/devx.c | 564 ++++++++++++++++++++++-
> include/uapi/rdma/mlx5_user_ioctl_cmds.h | 9 +
> 2 files changed, 566 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/infiniband/hw/mlx5/devx.c b/drivers/infiniband/hw/mlx5/devx.c
> index e9b9ba5a3e9a..304b13e7a265 100644
> +++ b/drivers/infiniband/hw/mlx5/devx.c
> @@ -14,6 +14,7 @@
> #include <linux/mlx5/driver.h>
> #include <linux/mlx5/fs.h>
> #include "mlx5_ib.h"
> +#include <linux/xarray.h>
>
> #define UVERBS_MODULE_NAME mlx5_ib
> #include <rdma/uverbs_named_ioctl.h>
> @@ -33,6 +34,37 @@ struct devx_async_data {
> struct mlx5_ib_uapi_devx_async_cmd_hdr hdr;
> };
>
> +/* first level XA value data structure */
> +struct devx_event {
> + struct xarray object_ids; /* second XA level, Key = object id */
> + struct list_head unaffiliated_list;
> +};
> +
> +/* second level XA value data structure */
> +struct devx_obj_event {
> + struct rcu_head rcu;
> + struct list_head obj_sub_list;
> +};
> +
> +struct devx_event_subscription {
> + struct list_head file_list; /* headed in private_data->
> + * subscribed_events_list
> + */
> + struct list_head xa_list; /* headed in devx_event->unaffiliated_list or
> + * devx_obj_event->obj_sub_list
> + */
> + struct list_head obj_list; /* headed in devx_object */
> +
> + u32 xa_key_level1;
> + u32 xa_key_level2;
> + struct rcu_head rcu;
> + u64 cookie;
> + bool is_obj_related;
> + struct ib_uobject *fd_uobj;
> + void *object; /* May need direct access upon hot unplug */
This should be a 'struct file *' and have a better name.
And I'm unclear why we need to store both the ib_uobject and the
struct file for the same thing? And why are we storing the uobj here
instead of the struct devx_async_event_file *?
Since uobj->object == flip && filp->private_data == uobj, I have a
hard time to understand why we need both things, it seems to me that
if we get the fget on the filp then we can rely on the
filp->private_data to get back to the devx_async_event_file.
> + struct eventfd_ctx *eventfd;
> +};
> +
> /*
> * As the obj_id in the firmware is not globally unique the object type
> * must be considered upon checking for a valid object id.
> @@ -1143,14 +1275,53 @@ static void devx_cleanup_mkey(struct devx_obj *obj)
> write_unlock_irqrestore(&table->lock, flags);
> }
>
> +static void devx_cleanup_subscription(struct mlx5_ib_dev *dev,
> + struct devx_event_subscription *sub)
> +{
> + list_del_rcu(&sub->file_list);
> + list_del_rcu(&sub->xa_list);
> +
> + if (sub->is_obj_related) {
is_obj_related looks like it is just list_empty(obj_list)??
Success oriented flow
> @@ -1523,6 +1700,350 @@ static int UVERBS_HANDLER(MLX5_IB_METHOD_DEVX_OBJ_ASYNC_QUERY)(
> return err;
> }
>
> +static void
> +subscribe_event_xa_dealloc(struct mlx5_devx_event_table *devx_event_table,
> + u32 key_level1,
> + u32 key_level2,
> + struct devx_obj_event *alloc_obj_event)
> +{
> + struct devx_event *event;
> +
> + /* Level 1 is valid for future use - no need to free */
> + if (!alloc_obj_event)
> + return;
> +
> + event = xa_load(&devx_event_table->event_xa, key_level1);
> + WARN_ON(!event);
> +
> + xa_erase(&event->object_ids, key_level2);
Shoulnd't this only erase if the value stored is NULL?
> + kfree(alloc_obj_event);
> +}
> +
> +static int
> +subscribe_event_xa_alloc(struct mlx5_devx_event_table *devx_event_table,
> + u32 key_level1,
> + bool is_level2,
> + u32 key_level2,
> + struct devx_obj_event **alloc_obj_event)
> +{
> + struct devx_obj_event *obj_event;
> + struct devx_event *event;
> + bool new_entry_level1 = false;
> + int err;
> +
> + event = xa_load(&devx_event_table->event_xa, key_level1);
> + if (!event) {
> + event = kzalloc(sizeof(*event), GFP_KERNEL);
> + if (!event)
> + return -ENOMEM;
> +
> + new_entry_level1 = true;
> + INIT_LIST_HEAD(&event->unaffiliated_list);
> + xa_init(&event->object_ids);
> +
> + err = xa_insert(&devx_event_table->event_xa,
> + key_level1,
> + event,
> + GFP_KERNEL);
> + if (err)
> + goto end;
> + }
> +
> + if (!is_level2)
> + return 0;
> +
> + obj_event = xa_load(&event->object_ids, key_level2);
> + if (!obj_event) {
> + err = xa_reserve(&event->object_ids, key_level2, GFP_KERNEL);
> + if (err)
> + goto err_level1;
> +
> + obj_event = kzalloc(sizeof(*obj_event), GFP_KERNEL);
> + if (!obj_event) {
> + err = -ENOMEM;
> + goto err_level2;
> + }
> +
> + INIT_LIST_HEAD(&obj_event->obj_sub_list);
> + *alloc_obj_event = obj_event;
This is goofy, just store the empty obj_event in the xa instead of
using xa_reserve, and when you go to do the error unwind just delete
any level2' devx_obj_event' that has a list_empty(obj_sub_list), get
rid of the wonky alloc_obj_event stuff.
The best configuration would be to use devx_cleanup_subscription to
undo the partially ready subscription.
> + }
> +
> + return 0;
> +
> +err_level2:
> + xa_erase(&event->object_ids, key_level2);
> +
> +err_level1:
> + if (new_entry_level1)
> + xa_erase(&devx_event_table->event_xa, key_level1);
> +end:
> + if (new_entry_level1)
> + kfree(event);
Can't do this, once the level1 is put in the tree it could be referenced by
the irqs. At least it needs a kfree_rcu, most likely it is simpler to
just leave it.
> +#define MAX_NUM_EVENTS 16
> +static int UVERBS_HANDLER(MLX5_IB_METHOD_DEVX_SUBSCRIBE_EVENT)(
> + struct uverbs_attr_bundle *attrs)
> +{
> + struct ib_uobject *devx_uobj = uverbs_attr_get_uobject(
> + attrs,
> + MLX5_IB_ATTR_DEVX_SUBSCRIBE_EVENT_OBJ_HANDLE);
> + struct mlx5_ib_ucontext *c = rdma_udata_to_drv_context(
> + &attrs->driver_udata, struct mlx5_ib_ucontext, ibucontext);
> + struct mlx5_ib_dev *dev = to_mdev(c->ibucontext.device);
> + struct ib_uobject *fd_uobj;
> + struct devx_obj *obj = NULL;
> + struct devx_async_event_file *ev_file;
> + struct mlx5_devx_event_table *devx_event_table = &dev->devx_event_table;
> + u16 *event_type_num_list;
> + struct devx_event_subscription **event_sub_arr;
> + struct devx_obj_event **event_obj_array_alloc;
> + int redirect_fd;
> + bool use_eventfd = false;
> + int num_events;
> + int num_alloc_xa_entries = 0;
> + u16 obj_type = 0;
> + u64 cookie = 0;
> + u32 obj_id = 0;
> + int err;
> + int i;
> +
> + if (!c->devx_uid)
> + return -EINVAL;
> +
> + if (!IS_ERR(devx_uobj)) {
> + obj = (struct devx_obj *)devx_uobj->object;
> + if (obj)
> + obj_id = get_dec_obj_id(obj->obj_id);
> + }
> +
> + fd_uobj = uverbs_attr_get_uobject(attrs,
> + MLX5_IB_ATTR_DEVX_SUBSCRIBE_EVENT_FD_HANDLE);
> + if (IS_ERR(fd_uobj))
> + return PTR_ERR(fd_uobj);
> +
> + ev_file = container_of(fd_uobj, struct devx_async_event_file,
> + uobj);
> +
> + if (uverbs_attr_is_valid(attrs,
> + MLX5_IB_ATTR_DEVX_SUBSCRIBE_EVENT_FD_NUM)) {
> + err = uverbs_copy_from(&redirect_fd, attrs,
> + MLX5_IB_ATTR_DEVX_SUBSCRIBE_EVENT_FD_NUM);
> + if (err)
> + return err;
> +
> + use_eventfd = true;
> + }
> +
> + if (uverbs_attr_is_valid(attrs,
> + MLX5_IB_ATTR_DEVX_SUBSCRIBE_EVENT_COOKIE)) {
> + if (use_eventfd)
> + return -EINVAL;
> +
> + err = uverbs_copy_from(&cookie, attrs,
> + MLX5_IB_ATTR_DEVX_SUBSCRIBE_EVENT_COOKIE);
> + if (err)
> + return err;
> + }
> +
> + num_events = uverbs_attr_ptr_get_array_size(
> + attrs, MLX5_IB_ATTR_DEVX_SUBSCRIBE_EVENT_TYPE_NUM_LIST,
> + sizeof(u16));
> +
> + if (num_events < 0)
> + return num_events;
> +
> + if (num_events > MAX_NUM_EVENTS)
> + return -EINVAL;
> +
> + event_type_num_list = uverbs_attr_get_alloced_ptr(attrs,
> + MLX5_IB_ATTR_DEVX_SUBSCRIBE_EVENT_TYPE_NUM_LIST);
> +
> + if (!is_valid_events(dev->mdev, num_events, event_type_num_list, obj))
> + return -EINVAL;
> +
> + event_sub_arr = uverbs_zalloc(attrs,
> + MAX_NUM_EVENTS * sizeof(struct devx_event_subscription *));
> + event_obj_array_alloc = uverbs_zalloc(attrs,
> + MAX_NUM_EVENTS * sizeof(struct devx_obj_event *));
There are so many list_heads in the devx_event_subscription, why not
use just one of them to store the allocated events instead of this
temp array? ie event_list looks good for this purpose.
> +
> + if (!event_sub_arr || !event_obj_array_alloc)
> + return -ENOMEM;
> +
> + /* Protect from concurrent subscriptions to same XA entries to allow
> + * both to succeed
> + */
> + mutex_lock(&devx_event_table->event_xa_lock);
> + for (i = 0; i < num_events; i++) {
> + u32 key_level1;
> +
> + if (obj)
> + obj_type = get_dec_obj_type(obj,
> + event_type_num_list[i]);
> + key_level1 = event_type_num_list[i] | obj_type << 16;
> +
> + err = subscribe_event_xa_alloc(devx_event_table,
> + key_level1,
> + obj ? true : false,
> + obj_id,
> + &event_obj_array_alloc[i]);
Usless ?:
> + if (err)
> + goto err;
> +
> + num_alloc_xa_entries++;
> + event_sub_arr[i] = kzalloc(sizeof(*event_sub_arr[i]),
> + GFP_KERNEL);
> + if (!event_sub_arr[i])
> + goto err;
> +
> + if (use_eventfd) {
> + event_sub_arr[i]->eventfd =
> + eventfd_ctx_fdget(redirect_fd);
> +
> + if (IS_ERR(event_sub_arr[i]->eventfd)) {
> + err = PTR_ERR(event_sub_arr[i]->eventfd);
> + event_sub_arr[i]->eventfd = NULL;
> + goto err;
> + }
> + }
> +
> + event_sub_arr[i]->cookie = cookie;
> + event_sub_arr[i]->fd_uobj = fd_uobj;
> + event_sub_arr[i]->object = fd_uobj->object;
> + /* May be needed upon cleanup the devx object/subscription */
> + event_sub_arr[i]->xa_key_level1 = key_level1;
> + event_sub_arr[i]->xa_key_level2 = obj_id;
> + event_sub_arr[i]->is_obj_related = obj ? true : false;
Unneeded ?:
Jason
^ permalink raw reply
* Re: [PATCH rdma-next v1 11/12] IB/mlx5: Implement DEVX dispatching event
From: Jason Gunthorpe @ 2019-06-24 12:03 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Yishai Hadas,
Saeed Mahameed, linux-netdev
In-Reply-To: <20190618171540.11729-12-leon@kernel.org>
On Tue, Jun 18, 2019 at 08:15:39PM +0300, Leon Romanovsky wrote:
> From: Yishai Hadas <yishaih@mellanox.com>
>
> Implement DEVX dispatching event by looking up for the applicable
> subscriptions for the reported event and using their target fd to
> signal/set the event.
>
> Signed-off-by: Yishai Hadas <yishaih@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> drivers/infiniband/hw/mlx5/devx.c | 362 +++++++++++++++++++++-
> include/uapi/rdma/mlx5_user_ioctl_verbs.h | 5 +
> 2 files changed, 357 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/infiniband/hw/mlx5/devx.c b/drivers/infiniband/hw/mlx5/devx.c
> index 304b13e7a265..49fdce95d6d9 100644
> +++ b/drivers/infiniband/hw/mlx5/devx.c
> @@ -34,6 +34,11 @@ struct devx_async_data {
> struct mlx5_ib_uapi_devx_async_cmd_hdr hdr;
> };
>
> +struct devx_async_event_data {
> + struct list_head list; /* headed in ev_queue->event_list */
> + struct mlx5_ib_uapi_devx_async_event_hdr hdr;
> +};
> +
> /* first level XA value data structure */
> struct devx_event {
> struct xarray object_ids; /* second XA level, Key = object id */
> @@ -54,7 +59,9 @@ struct devx_event_subscription {
> * devx_obj_event->obj_sub_list
> */
> struct list_head obj_list; /* headed in devx_object */
> + struct list_head event_list; /* headed in ev_queue->event_list */
>
> + u8 is_cleaned:1;
There is a loose bool 'is_obj_related' that should be combined with
this bool bitfield as well.
> static void devx_cleanup_subscription(struct mlx5_ib_dev *dev,
> - struct devx_event_subscription *sub)
> + struct devx_event_subscription *sub,
> + bool file_close)
> {
> - list_del_rcu(&sub->file_list);
> + if (sub->is_cleaned)
> + goto end;
> +
> + sub->is_cleaned = 1;
> list_del_rcu(&sub->xa_list);
>
> if (sub->is_obj_related) {
> @@ -1303,10 +1355,15 @@ static void devx_cleanup_subscription(struct mlx5_ib_dev *dev,
> }
> }
>
> - if (sub->eventfd)
> - eventfd_ctx_put(sub->eventfd);
> +end:
> + if (file_close) {
> + if (sub->eventfd)
> + eventfd_ctx_put(sub->eventfd);
>
> - kfree_rcu(sub, rcu);
> + list_del_rcu(&sub->file_list);
> + /* subscription may not be used by the read API any more */
> + kfree_rcu(sub, rcu);
> + }
Dis like this confusing file_close stuff, just put this in the single place
that calls this with the true bool
> +static int deliver_event(struct devx_event_subscription *event_sub,
> + const void *data)
> +{
> + struct ib_uobject *fd_uobj = event_sub->fd_uobj;
> + struct devx_async_event_file *ev_file;
> + struct devx_async_event_queue *ev_queue;
> + struct devx_async_event_data *event_data;
> + unsigned long flags;
> + bool omit_data;
> +
> + ev_file = container_of(fd_uobj, struct devx_async_event_file,
> + uobj);
> + ev_queue = &ev_file->ev_queue;
> + omit_data = ev_queue->flags &
> + MLX5_IB_UAPI_DEVX_CREATE_EVENT_CHANNEL_FLAGS_OMIT_EV_DATA;
> +
> + if (omit_data) {
> + spin_lock_irqsave(&ev_queue->lock, flags);
> + if (!list_empty(&event_sub->event_list)) {
> + spin_unlock_irqrestore(&ev_queue->lock, flags);
> + return 0;
> + }
> +
> + list_add_tail(&event_sub->event_list, &ev_queue->event_list);
> + spin_unlock_irqrestore(&ev_queue->lock, flags);
> + wake_up_interruptible(&ev_queue->poll_wait);
> + return 0;
> + }
> +
> + event_data = kzalloc(sizeof(*event_data) +
> + (omit_data ? 0 : sizeof(struct mlx5_eqe)),
> + GFP_ATOMIC);
omit_data is always false here
> + if (!event_data) {
> + spin_lock_irqsave(&ev_queue->lock, flags);
> + ev_queue->is_overflow_err = 1;
> + spin_unlock_irqrestore(&ev_queue->lock, flags);
> + return -ENOMEM;
> + }
> +
> + event_data->hdr.cookie = event_sub->cookie;
> + memcpy(event_data->hdr.out_data, data, sizeof(struct mlx5_eqe));
> +
> + spin_lock_irqsave(&ev_queue->lock, flags);
> + list_add_tail(&event_data->list, &ev_queue->event_list);
> + spin_unlock_irqrestore(&ev_queue->lock, flags);
> + wake_up_interruptible(&ev_queue->poll_wait);
> +
> + return 0;
> +}
> +
> +static void dispatch_event_fd(struct list_head *fd_list,
> + const void *data)
> +{
> + struct devx_event_subscription *item;
> +
> + list_for_each_entry_rcu(item, fd_list, xa_list) {
> + if (!get_file_rcu((struct file *)item->object))
> + continue;
> +
> + if (item->eventfd) {
> + eventfd_signal(item->eventfd, 1);
> + fput(item->object);
> + continue;
> + }
> +
> + deliver_event(item, data);
> + fput(item->object);
> + }
> +}
> +
> static int devx_event_notifier(struct notifier_block *nb,
> unsigned long event_type, void *data)
> {
> - return NOTIFY_DONE;
> + struct mlx5_devx_event_table *table;
> + struct mlx5_ib_dev *dev;
> + struct devx_event *event;
> + struct devx_obj_event *obj_event;
> + u16 obj_type = 0;
> + bool is_unaffiliated;
> + u32 obj_id;
> +
> + /* Explicit filtering to kernel events which may occur frequently */
> + if (event_type == MLX5_EVENT_TYPE_CMD ||
> + event_type == MLX5_EVENT_TYPE_PAGE_REQUEST)
> + return NOTIFY_OK;
> +
> + table = container_of(nb, struct mlx5_devx_event_table, devx_nb.nb);
> + dev = container_of(table, struct mlx5_ib_dev, devx_event_table);
> + is_unaffiliated = is_unaffiliated_event(dev->mdev, event_type);
> +
> + if (!is_unaffiliated)
> + obj_type = get_event_obj_type(event_type, data);
> + event = xa_load(&table->event_xa, event_type | (obj_type << 16));
> + if (!event)
> + return NOTIFY_DONE;
event should be in the rcu as well
> + if (is_unaffiliated) {
> + dispatch_event_fd(&event->unaffiliated_list, data);
> + return NOTIFY_OK;
> + }
> +
> + obj_id = devx_get_obj_id_from_event(event_type, data);
> + rcu_read_lock();
> + obj_event = xa_load(&event->object_ids, obj_id);
> + if (!obj_event) {
> + rcu_read_unlock();
> + return NOTIFY_DONE;
> + }
> +
> + dispatch_event_fd(&obj_event->obj_sub_list, data);
> +
> + rcu_read_unlock();
> + return NOTIFY_OK;
> }
>
> void mlx5_ib_devx_init_event_table(struct mlx5_ib_dev *dev)
> @@ -2221,7 +2444,7 @@ void mlx5_ib_devx_cleanup_event_table(struct mlx5_ib_dev *dev)
> event = entry;
> list_for_each_entry_safe(sub, tmp, &event->unaffiliated_list,
> xa_list)
> - devx_cleanup_subscription(dev, sub);
> + devx_cleanup_subscription(dev, sub, false);
> kfree(entry);
> }
> mutex_unlock(&dev->devx_event_table.event_xa_lock);
> @@ -2329,18 +2552,126 @@ static const struct file_operations devx_async_cmd_event_fops = {
> static ssize_t devx_async_event_read(struct file *filp, char __user *buf,
> size_t count, loff_t *pos)
> {
> - return -EINVAL;
> + struct devx_async_event_file *ev_file = filp->private_data;
> + struct devx_async_event_queue *ev_queue = &ev_file->ev_queue;
> + struct devx_event_subscription *event_sub;
> + struct devx_async_event_data *uninitialized_var(event);
> + int ret = 0;
> + size_t eventsz;
> + bool omit_data;
> + void *event_data;
> +
> + omit_data = ev_queue->flags &
> + MLX5_IB_UAPI_DEVX_CREATE_EVENT_CHANNEL_FLAGS_OMIT_EV_DATA;
> +
> + spin_lock_irq(&ev_queue->lock);
> +
> + if (ev_queue->is_overflow_err) {
> + ev_queue->is_overflow_err = 0;
> + spin_unlock_irq(&ev_queue->lock);
> + return -EOVERFLOW;
> + }
> +
> + while (list_empty(&ev_queue->event_list)) {
> + spin_unlock_irq(&ev_queue->lock);
> +
> + if (filp->f_flags & O_NONBLOCK)
> + return -EAGAIN;
> +
> + if (wait_event_interruptible(ev_queue->poll_wait,
> + (!list_empty(&ev_queue->event_list) ||
> + ev_queue->is_destroyed))) {
> + return -ERESTARTSYS;
> + }
> +
> + if (list_empty(&ev_queue->event_list) &&
> + ev_queue->is_destroyed)
> + return -EIO;
All these tests should be under the lock.
Why don't we return EIO as soon as is-destroyed happens? What is the
point of flushing out the accumulated events?
> +
> + spin_lock_irq(&ev_queue->lock);
> + }
> +
> + if (omit_data) {
> + event_sub = list_first_entry(&ev_queue->event_list,
> + struct devx_event_subscription,
> + event_list);
> + eventsz = sizeof(event_sub->cookie);
> + event_data = &event_sub->cookie;
> + } else {
> + event = list_first_entry(&ev_queue->event_list,
> + struct devx_async_event_data, list);
> + eventsz = sizeof(struct mlx5_eqe) +
> + sizeof(struct mlx5_ib_uapi_devx_async_event_hdr);
> + event_data = &event->hdr;
> + }
> +
> + if (eventsz > count) {
> + spin_unlock_irq(&ev_queue->lock);
> + return -ENOSPC;
This is probably the wrong errno
> + }
> +
> + if (omit_data)
> + list_del_init(&event_sub->event_list);
> + else
> + list_del(&event->list);
> +
> + spin_unlock_irq(&ev_queue->lock);
> +
> + if (copy_to_user(buf, event_data, eventsz))
> + ret = -EFAULT;
> + else
> + ret = eventsz;
This is really poorly ordered, EFAULT will cause the event to be lost. :(
Maybe the event should be re-added on error? Tricky.
> + if (!omit_data)
> + kfree(event);
> + return ret;
> }
>
> static __poll_t devx_async_event_poll(struct file *filp,
> struct poll_table_struct *wait)
> {
> - return 0;
> + struct devx_async_event_file *ev_file = filp->private_data;
> + struct devx_async_event_queue *ev_queue = &ev_file->ev_queue;
> + __poll_t pollflags = 0;
> +
> + poll_wait(filp, &ev_queue->poll_wait, wait);
> +
> + spin_lock_irq(&ev_queue->lock);
> + if (ev_queue->is_destroyed)
> + pollflags = EPOLLIN | EPOLLRDNORM | EPOLLRDHUP;
> + else if (!list_empty(&ev_queue->event_list))
> + pollflags = EPOLLIN | EPOLLRDNORM;
> + spin_unlock_irq(&ev_queue->lock);
> +
> + return pollflags;
> }
>
> static int devx_async_event_close(struct inode *inode, struct file *filp)
> {
> + struct ib_uobject *uobj = filp->private_data;
> + struct devx_async_event_file *ev_file =
> + container_of(uobj, struct devx_async_event_file, uobj);
> + struct devx_event_subscription *event_sub, *event_sub_tmp;
> + struct devx_async_event_data *entry, *tmp;
> +
> + mutex_lock(&ev_file->dev->devx_event_table.event_xa_lock);
> + /* delete the subscriptions which are related to this FD */
> + list_for_each_entry_safe(event_sub, event_sub_tmp,
> + &ev_file->subscribed_events_list, file_list)
> + devx_cleanup_subscription(ev_file->dev, event_sub, true);
> + mutex_unlock(&ev_file->dev->devx_event_table.event_xa_lock);
> +
> + /* free the pending events allocation */
> + if (!(ev_file->ev_queue.flags &
> + MLX5_IB_UAPI_DEVX_CREATE_EVENT_CHANNEL_FLAGS_OMIT_EV_DATA)) {
> + spin_lock_irq(&ev_file->ev_queue.lock);
> + list_for_each_entry_safe(entry, tmp,
> + &ev_file->ev_queue.event_list, list)
> + kfree(entry); /* read can't come any nore */
spelling
> + spin_unlock_irq(&ev_file->ev_queue.lock);
> + }
> uverbs_close_fd(filp);
> + put_device(&ev_file->dev->ib_dev.dev);
> return 0;
> }
>
> @@ -2374,6 +2705,17 @@ static int devx_hot_unplug_async_cmd_event_file(struct ib_uobject *uobj,
> static int devx_hot_unplug_async_event_file(struct ib_uobject *uobj,
> enum rdma_remove_reason why)
> {
> + struct devx_async_event_file *ev_file =
> + container_of(uobj, struct devx_async_event_file,
> + uobj);
> + struct devx_async_event_queue *ev_queue = &ev_file->ev_queue;
> +
> + spin_lock_irq(&ev_queue->lock);
> + ev_queue->is_destroyed = 1;
> + spin_unlock_irq(&ev_queue->lock);
> +
> + if (why == RDMA_REMOVE_DRIVER_REMOVE)
> + wake_up_interruptible(&ev_queue->poll_wait);
Why isn't this wakeup always done?
Jason
^ permalink raw reply
* Re: [PATCH rdma-next v1 12/12] IB/mlx5: Add DEVX support for CQ events
From: Jason Gunthorpe @ 2019-06-24 12:04 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Yishai Hadas,
Saeed Mahameed, linux-netdev
In-Reply-To: <20190618171540.11729-13-leon@kernel.org>
On Tue, Jun 18, 2019 at 08:15:40PM +0300, Leon Romanovsky wrote:
> From: Yishai Hadas <yishaih@mellanox.com>
>
> Add DEVX support for CQ events by creating and destroying the CQ via
> mlx5_core and set an handler to manage its completions.
>
> Signed-off-by: Yishai Hadas <yishaih@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> drivers/infiniband/hw/mlx5/devx.c | 40 +++++++++++++++++++++++++++++++
> 1 file changed, 40 insertions(+)
>
> diff --git a/drivers/infiniband/hw/mlx5/devx.c b/drivers/infiniband/hw/mlx5/devx.c
> index 49fdce95d6d9..91ccd58ebc05 100644
> +++ b/drivers/infiniband/hw/mlx5/devx.c
> @@ -19,9 +19,12 @@
> #define UVERBS_MODULE_NAME mlx5_ib
> #include <rdma/uverbs_named_ioctl.h>
>
> +static void dispatch_event_fd(struct list_head *fd_list, const void *data);
> +
> enum devx_obj_flags {
> DEVX_OBJ_FLAGS_INDIRECT_MKEY = 1 << 0,
> DEVX_OBJ_FLAGS_DCT = 1 << 1,
> + DEVX_OBJ_FLAGS_CQ = 1 << 2,
> };
>
> struct devx_async_data {
> @@ -94,6 +97,7 @@ struct devx_async_event_file {
> #define MLX5_MAX_DESTROY_INBOX_SIZE_DW MLX5_ST_SZ_DW(delete_fte_in)
> struct devx_obj {
> struct mlx5_core_dev *mdev;
> + struct mlx5_ib_dev *ib_dev;
This seems strange, why would we need to store the core_dev and the ib_dev
in a struct when ibdev->mdev == core_dev?
Jason
^ permalink raw reply
* [PATCH][next] RDMA/hns: fix spelling mistake "attatch" -> "attach"
From: Colin King @ 2019-06-24 12:16 UTC (permalink / raw)
To: Lijun Ou, Wei Hu, Doug Ledford, Jason Gunthorpe, linux-rdma
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
There is a spelling mistake in an dev_err message. Fix it.
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/infiniband/hw/hns/hns_roce_qp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/infiniband/hw/hns/hns_roce_qp.c b/drivers/infiniband/hw/hns/hns_roce_qp.c
index 99ec5d43b99b..13a1916b0cdc 100644
--- a/drivers/infiniband/hw/hns/hns_roce_qp.c
+++ b/drivers/infiniband/hw/hns/hns_roce_qp.c
@@ -858,7 +858,7 @@ static int hns_roce_create_qp_common(struct hns_roce_dev *hr_dev,
ret = hns_roce_mtr_attach(hr_dev, &hr_qp->mtr, buf_list,
hr_qp->regions, hr_qp->region_cnt);
if (ret) {
- dev_err(dev, "mtr attatch error for create qp\n");
+ dev_err(dev, "mtr attach error for create qp\n");
goto err_mtr;
}
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v4 2/3][v4.9.y] coredump: fix race condition between mmget_not_zero()/get_task_mm() and core dumping
From: Jason Gunthorpe @ 2019-06-24 13:12 UTC (permalink / raw)
To: Ajay Kaher
Cc: aarcange@redhat.com, jannh@google.com, oleg@redhat.com,
peterx@redhat.com, rppt@linux.ibm.com, mhocko@suse.com,
jglisse@redhat.com, akpm@linux-foundation.org,
mike.kravetz@oracle.com, viro@zeniv.linux.org.uk,
riandrews@android.com, arve@android.com, Yishai Hadas,
dledford@redhat.com, sean.hefty@intel.com,
hal.rosenstock@gmail.com, Matan Barak, Leon
In-Reply-To: <1561410186-3919-2-git-send-email-akaher@vmware.com>
On Tue, Jun 25, 2019 at 02:33:04AM +0530, Ajay Kaher wrote:
> This patch is the extension of following upstream commit to fix
> the race condition between get_task_mm() and core dumping
> for IB->mlx4 and IB->mlx5 drivers:
>
> commit 04f5866e41fb ("coredump: fix race condition between
> mmget_not_zero()/get_task_mm() and core dumping")'
>
> Thanks to Jason for pointing this.
>
> Signed-off-by: Ajay Kaher <akaher@vmware.com>
> ---
> drivers/infiniband/hw/mlx4/main.c | 4 +++-
> drivers/infiniband/hw/mlx5/main.c | 3 +++
> 2 files changed, 6 insertions(+), 1 deletion(-)
Looks OK
Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>
Thanks
Jason
^ permalink raw reply
* Re: [PATCH rdma-next v1 08/12] IB/mlx5: Introduce MLX5_IB_OBJECT_DEVX_ASYNC_EVENT_FD
From: Yishai Hadas @ 2019-06-24 13:25 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Yishai Hadas,
Saeed Mahameed, linux-netdev
In-Reply-To: <20190624115059.GA5479@mellanox.com>
On 6/24/2019 2:51 PM, Jason Gunthorpe wrote:
> On Tue, Jun 18, 2019 at 08:15:36PM +0300, Leon Romanovsky wrote:
>> From: Yishai Hadas <yishaih@mellanox.com>
>>
>> Introduce MLX5_IB_OBJECT_DEVX_ASYNC_EVENT_FD and its initial
>> implementation.
>>
>> This object is from type class FD and will be used to read DEVX
>> async events.
>>
>> Signed-off-by: Yishai Hadas <yishaih@mellanox.com>
>> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
>> drivers/infiniband/hw/mlx5/devx.c | 112 ++++++++++++++++++++--
>> include/uapi/rdma/mlx5_user_ioctl_cmds.h | 10 ++
>> include/uapi/rdma/mlx5_user_ioctl_verbs.h | 4 +
>> 3 files changed, 116 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/infiniband/hw/mlx5/devx.c b/drivers/infiniband/hw/mlx5/devx.c
>> index 80b42d069328..1815ce0f8daf 100644
>> +++ b/drivers/infiniband/hw/mlx5/devx.c
>> @@ -33,6 +33,24 @@ struct devx_async_data {
>> struct mlx5_ib_uapi_devx_async_cmd_hdr hdr;
>> };
>>
>> +struct devx_async_event_queue {
>
> It seems to be a mistake to try and re-use the async_event_queue for
> both cmd and event, as they use it very differently and don't even
> store the same things in the event_list. I think it is bettter to just
> inline this into devx_async_event_file (and inline the old struct in
> the cmd file
>
How about having another struct with all the event's queue fields
together ? this has the benefit of having all those related fields in
one place and leave the cmd as is.
Alternatively,
We can inline the event stuff under devx_async_event_file and leave the
cmd for now under a struct as it's not directly related to this series.
What do you think ?
>> + spinlock_t lock;
>> + wait_queue_head_t poll_wait;
>> + struct list_head event_list;
>> + atomic_t bytes_in_use;
>> + u8 is_destroyed:1;
>> + u32 flags;
>> +};
>
> All the flags testing is ugly, why not just add another bitfield?
The flags are coming from user space and have their different name
space, I prefer to not mix with kernel ones. (i.e. is_destroyed).
Makes sense ?
>
>> +
>> +struct devx_async_event_file {
>> + struct ib_uobject uobj;
>> + struct list_head subscribed_events_list; /* Head of events that are
>> + * subscribed to this FD
>> + */
>> + struct devx_async_event_queue ev_queue;
>> + struct mlx5_ib_dev *dev;
>> +};
>> +
>
> Crazy indenting
>
OK, will handle.
>> diff --git a/include/uapi/rdma/mlx5_user_ioctl_verbs.h b/include/uapi/rdma/mlx5_user_ioctl_verbs.h
>> index a8f34c237458..57beea4589e4 100644
>> +++ b/include/uapi/rdma/mlx5_user_ioctl_verbs.h
>> @@ -63,5 +63,9 @@ enum mlx5_ib_uapi_dm_type {
>> MLX5_IB_UAPI_DM_TYPE_HEADER_MODIFY_SW_ICM,
>> };
>>
>> +enum mlx5_ib_uapi_devx_create_event_channel_flags {
>> + MLX5_IB_UAPI_DEVX_CREATE_EVENT_CHANNEL_FLAGS_OMIT_EV_DATA = 1
>> << 0,
>
> Maybe this name is too long
Quite long but follows the name scheme having the UAPI prefix.
Any shorter suggestion ?
^ permalink raw reply
* Re: [PATCH rdma-next v1 09/12] IB/mlx5: Register DEVX with mlx5_core to get async events
From: Yishai Hadas @ 2019-06-24 13:36 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Leon Romanovsky, Doug Ledford, Leon Romanovsky, RDMA mailing list,
Yishai Hadas, Saeed Mahameed, linux-netdev
In-Reply-To: <20190624115206.GB5479@mellanox.com>
On 6/24/2019 2:52 PM, Jason Gunthorpe wrote:
> On Tue, Jun 18, 2019 at 08:15:37PM +0300, Leon Romanovsky wrote:
>> void __mlx5_ib_remove(struct mlx5_ib_dev *dev,
>> diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
>> index 9cf23ae6324e..556af34b788b 100644
>> +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
>> @@ -944,6 +944,13 @@ struct mlx5_ib_pf_eq {
>> mempool_t *pool;
>> };
>>
>> +struct mlx5_devx_event_table {
>> + struct mlx5_nb devx_nb;
>> + /* serialize updating the event_xa */
>> + struct mutex event_xa_lock;
>> + struct xarray event_xa;
>> +};
>> +
>> struct mlx5_ib_dev {
>> struct ib_device ib_dev;
>> struct mlx5_core_dev *mdev;
>> @@ -994,6 +1001,7 @@ struct mlx5_ib_dev {
>> struct mlx5_srq_table srq_table;
>> struct mlx5_async_ctx async_ctx;
>> int free_port;
>> + struct mlx5_devx_event_table devx_event_table;
>
> I really question if adding all these structs really does anything for
> readability..
>
I would prefer this option to add only one structure (i.e.
mlx5_devx_event_table) on ib_dev, it will hold internally the other
related stuff.
^ permalink raw reply
* Re: [PATCH] RDMA/core: Fix race when resolving IP address
From: Dag Moxnes @ 2019-06-24 13:40 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: dledford, leon, linux-rdma, linux-kernel, Parav Pandit
In-Reply-To: <20190621145604.GS19891@ziepe.ca>
Hi Jason,
Thanks for the review.
On 6/21/19 4:56 PM, Jason Gunthorpe wrote:
> On Fri, Jun 21, 2019 at 04:09:16PM +0200, Dag Moxnes wrote:
>> Use neighbour lock when copying MAC address from neighbour data struct
>> in dst_fetch_ha.
>>
>> When not using the lock, it is possible for the function to race with
>> neigh_update, causing it to copy an invalid MAC address.
>>
>> It is possible to provoke this error by calling rdma_resolve_addr in a
>> tight loop, while deleting the corresponding ARP entry in another tight
>> loop.
>>
>> Signed-off-by: Dag Moxnes <dag.moxnes@oracle.com>
>> Change-Id: I3c5f982b304457f0a83ea7def2fac70315ed38b4
>> drivers/infiniband/core/addr.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c
>> index 2f7d141598..e4945fd1bb 100644
>> +++ b/drivers/infiniband/core/addr.c
>> @@ -333,12 +333,16 @@ static int dst_fetch_ha(const struct dst_entry *dst,
>> if (!n)
>> return -ENODATA;
>>
>> + read_lock_bh(&n->lock)
Miising semicolon at end of statement. Sorry about that.
>> if (!(n->nud_state & NUD_VALID)) {
>> - neigh_event_send(n, NULL);
>> ret = -ENODATA;
>> } else {
>> memcpy(dev_addr->dst_dev_addr, n->ha, MAX_ADDR_LEN);
>> }
>> + read_unlock_bh(&n->lock);
>> +
>> + if (ret)
>> + neigh_event_send(n, NULL);
>>
>> neigh_release(n);
> Can we write this with less spaghetti please, maybe:
>
> static int dst_fetch_ha(const struct dst_entry *dst,
> struct rdma_dev_addr *dev_addr,
> const void *daddr)
> {
> struct neighbour *n;
> int ret = 0;
>
> n = dst_neigh_lookup(dst, daddr);
> if (!n)
> return -ENODATA;
>
> read_lock_bh(&n->lock);
> if (!(n->nud_state & NUD_VALID)) {
> read_unlock_bh(&n->lock);
> goto out_send;
> }
> memcpy(dev_addr->dst_dev_addr, n->ha, MAX_ADDR_LEN);
> read_unlock_bh(&n->lock);
>
> goto out_release;
>
> out_send:
> neigh_event_send(n, NULL);
> ret = -ENODATA;
> out_release:
> neigh_release(n);
>
> return ret;
> }
Personally I find it more readable when the unlock is done in one place,
but sure, I can rewrite it the way you suggest if the reviewers agree that
that way is preferable.
Regards,
-Dag
> Also, Parav should look at it.
>
> Thanks,
> Jason
^ permalink raw reply
* Re: [RFC PATCH 00/28] Removing struct page from P2PDMA
From: Jason Gunthorpe @ 2019-06-24 13:46 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Dan Williams, Logan Gunthorpe, Linux Kernel Mailing List,
linux-block, linux-nvme, linux-pci, linux-rdma, Jens Axboe,
Bjorn Helgaas, Sagi Grimberg, Keith Busch, Stephen Bates
In-Reply-To: <20190624073126.GB3954@lst.de>
On Mon, Jun 24, 2019 at 09:31:26AM +0200, Christoph Hellwig wrote:
> On Thu, Jun 20, 2019 at 04:33:53PM -0300, Jason Gunthorpe wrote:
> > > My primary concern with this is that ascribes a level of generality
> > > that just isn't there for peer-to-peer dma operations. "Peer"
> > > addresses are not "DMA" addresses, and the rules about what can and
> > > can't do peer-DMA are not generically known to the block layer.
> >
> > ?? The P2P infrastructure produces a DMA bus address for the
> > initiating device that is is absolutely a DMA address. There is some
> > intermediate CPU centric representation, but after mapping it is the
> > same as any other DMA bus address.
> >
> > The map function can tell if the device pair combination can do p2p or
> > not.
>
> At the PCIe level there is no such thing as a DMA address, it all
> is bus address with MMIO and DMA in the same address space (without
> that P2P would have not chance of actually working obviously). But
> that bus address space is different per "bus" (which would be an
> root port in PCIe), and we need to be careful about that.
Sure, that is how dma_addr_t is supposed to work - it is always a
device specific value that can be used only by the device that it was
created for, and different devices could have different dma_addr_t
values for the same memory.
So when Logan goes and puts dma_addr_t into the block stack he must
also invert things so that the DMA map happens at the start of the
process to create the right dma_addr_t early.
I'm not totally clear if this series did that inversion, if it didn't
then it should not be using the dma_addr_t label at all, or refering
to anything as a 'dma address' as it is just confusing.
BTW, it is not just offset right? It is possible that the IOMMU can
generate unique dma_addr_t values for each device?? Simple offset is
just something we saw in certain embedded cases, IIRC.
Jason
^ permalink raw reply
* Re: [RFC PATCH 00/28] Removing struct page from P2PDMA
From: Christoph Hellwig @ 2019-06-24 13:50 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Christoph Hellwig, Dan Williams, Logan Gunthorpe,
Linux Kernel Mailing List, linux-block, linux-nvme, linux-pci,
linux-rdma, Jens Axboe, Bjorn Helgaas, Sagi Grimberg, Keith Busch,
Stephen Bates
In-Reply-To: <20190624134641.GA8268@ziepe.ca>
On Mon, Jun 24, 2019 at 10:46:41AM -0300, Jason Gunthorpe wrote:
> BTW, it is not just offset right? It is possible that the IOMMU can
> generate unique dma_addr_t values for each device?? Simple offset is
> just something we saw in certain embedded cases, IIRC.
Yes, it could. If we are trying to do P2P between two devices on
different root ports and with the IOMMU enabled we'll generate
a new bus address for the BAR on the other side dynamically everytime
we map.
^ permalink raw reply
* Re: [RFC PATCH 00/28] Removing struct page from P2PDMA
From: Jason Gunthorpe @ 2019-06-24 13:55 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Dan Williams, Logan Gunthorpe, Linux Kernel Mailing List,
linux-block, linux-nvme, linux-pci, linux-rdma, Jens Axboe,
Bjorn Helgaas, Sagi Grimberg, Keith Busch, Stephen Bates
In-Reply-To: <20190624135024.GA11248@lst.de>
On Mon, Jun 24, 2019 at 03:50:24PM +0200, Christoph Hellwig wrote:
> On Mon, Jun 24, 2019 at 10:46:41AM -0300, Jason Gunthorpe wrote:
> > BTW, it is not just offset right? It is possible that the IOMMU can
> > generate unique dma_addr_t values for each device?? Simple offset is
> > just something we saw in certain embedded cases, IIRC.
>
> Yes, it could. If we are trying to do P2P between two devices on
> different root ports and with the IOMMU enabled we'll generate
> a new bus address for the BAR on the other side dynamically everytime
> we map.
Even with the same root port if ACS is turned on could behave like this.
It is only a very narrow case where you can take shortcuts with
dma_addr_t, and I don't think shortcuts like are are appropriate for
the mainline kernel..
Jason
^ permalink raw reply
* Re: [PATCH v17 04/15] mm, arm64: untag user pointers passed to memory syscalls
From: Andrey Konovalov @ 2019-06-24 14:22 UTC (permalink / raw)
To: Khalid Aziz
Cc: Linux ARM, Linux Memory Management List, LKML, amd-gfx, dri-devel,
linux-rdma, linux-media, kvm, open list:KERNEL SELFTEST FRAMEWORK,
Catalin Marinas, Vincenzo Frascino, Will Deacon, Mark Rutland,
Andrew Morton, Greg Kroah-Hartman, Kees Cook, Yishai Hadas,
Felix Kuehling, Alexander Deucher
In-Reply-To: <c4bdd767-eb3f-6668-0f49-4aaf4bc7689d@oracle.com>
On Wed, Jun 19, 2019 at 6:46 PM Khalid Aziz <khalid.aziz@oracle.com> wrote:
>
> On 6/19/19 9:55 AM, Khalid Aziz wrote:
> > On 6/12/19 5:43 AM, Andrey Konovalov wrote:
> >> This patch is a part of a series that extends arm64 kernel ABI to allow to
> >> pass tagged user pointers (with the top byte set to something else other
> >> than 0x00) as syscall arguments.
> >>
> >> This patch allows tagged pointers to be passed to the following memory
> >> syscalls: get_mempolicy, madvise, mbind, mincore, mlock, mlock2, mprotect,
> >> mremap, msync, munlock, move_pages.
> >>
> >> The mmap and mremap syscalls do not currently accept tagged addresses.
> >> Architectures may interpret the tag as a background colour for the
> >> corresponding vma.
> >>
> >> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> >> Reviewed-by: Kees Cook <keescook@chromium.org>
> >> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> >> ---
> >
> > Reviewed-by: Khalid Aziz <khalid.aziz@oracle.com>
> >
> >
>
> I would also recommend updating commit log for all the patches in this
> series that are changing files under mm/ as opposed to arch/arm64 to not
> reference arm64 kernel ABI since the change applies to every
> architecture. So something along the lines of "This patch is part of a
> series that extends kernel ABI to allow......."
Sure, will do in v18, thanks!
>
> --
> Khalid
>
>
> >> mm/madvise.c | 2 ++
> >> mm/mempolicy.c | 3 +++
> >> mm/migrate.c | 2 +-
> >> mm/mincore.c | 2 ++
> >> mm/mlock.c | 4 ++++
> >> mm/mprotect.c | 2 ++
> >> mm/mremap.c | 7 +++++++
> >> mm/msync.c | 2 ++
> >> 8 files changed, 23 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/mm/madvise.c b/mm/madvise.c
> >> index 628022e674a7..39b82f8a698f 100644
> >> --- a/mm/madvise.c
> >> +++ b/mm/madvise.c
> >> @@ -810,6 +810,8 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> >> size_t len;
> >> struct blk_plug plug;
> >>
> >> + start = untagged_addr(start);
> >> +
> >> if (!madvise_behavior_valid(behavior))
> >> return error;
> >>
> >> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> >> index 01600d80ae01..78e0a88b2680 100644
> >> --- a/mm/mempolicy.c
> >> +++ b/mm/mempolicy.c
> >> @@ -1360,6 +1360,7 @@ static long kernel_mbind(unsigned long start, unsigned long len,
> >> int err;
> >> unsigned short mode_flags;
> >>
> >> + start = untagged_addr(start);
> >> mode_flags = mode & MPOL_MODE_FLAGS;
> >> mode &= ~MPOL_MODE_FLAGS;
> >> if (mode >= MPOL_MAX)
> >> @@ -1517,6 +1518,8 @@ static int kernel_get_mempolicy(int __user *policy,
> >> int uninitialized_var(pval);
> >> nodemask_t nodes;
> >>
> >> + addr = untagged_addr(addr);
> >> +
> >> if (nmask != NULL && maxnode < nr_node_ids)
> >> return -EINVAL;
> >>
> >> diff --git a/mm/migrate.c b/mm/migrate.c
> >> index f2ecc2855a12..d22c45cf36b2 100644
> >> --- a/mm/migrate.c
> >> +++ b/mm/migrate.c
> >> @@ -1616,7 +1616,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
> >> goto out_flush;
> >> if (get_user(node, nodes + i))
> >> goto out_flush;
> >> - addr = (unsigned long)p;
> >> + addr = (unsigned long)untagged_addr(p);
> >>
> >> err = -ENODEV;
> >> if (node < 0 || node >= MAX_NUMNODES)
> >> diff --git a/mm/mincore.c b/mm/mincore.c
> >> index c3f058bd0faf..64c322ed845c 100644
> >> --- a/mm/mincore.c
> >> +++ b/mm/mincore.c
> >> @@ -249,6 +249,8 @@ SYSCALL_DEFINE3(mincore, unsigned long, start, size_t, len,
> >> unsigned long pages;
> >> unsigned char *tmp;
> >>
> >> + start = untagged_addr(start);
> >> +
> >> /* Check the start address: needs to be page-aligned.. */
> >> if (start & ~PAGE_MASK)
> >> return -EINVAL;fixup_user_fault
> >> diff --git a/mm/mlock.c b/mm/mlock.c
> >> index 080f3b36415b..e82609eaa428 100644
> >> --- a/mm/mlock.c
> >> +++ b/mm/mlock.c
> >> @@ -674,6 +674,8 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla
> >> unsigned long lock_limit;
> >> int error = -ENOMEM;
> >>
> >> + start = untagged_addr(start);
> >> +
> >> if (!can_do_mlock())
> >> return -EPERM;
> >>
> >> @@ -735,6 +737,8 @@ SYSCALL_DEFINE2(munlock, unsigned long, start, size_t, len)
> >> {
> >> int ret;
> >>
> >> + start = untagged_addr(start);
> >> +
> >> len = PAGE_ALIGN(len + (offset_in_page(start)));
> >> start &= PAGE_MASK;
> >>
> >> diff --git a/mm/mprotect.c b/mm/mprotect.c
> >> index bf38dfbbb4b4..19f981b733bc 100644
> >> --- a/mm/mprotect.c
> >> +++ b/mm/mprotect.c
> >> @@ -465,6 +465,8 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
> >> const bool rier = (current->personality & READ_IMPLIES_EXEC) &&
> >> (prot & PROT_READ);
> >>
> >> + start = untagged_addr(start);
> >> +
> >> prot &= ~(PROT_GROWSDOWN|PROT_GROWSUP);
> >> if (grows == (PROT_GROWSDOWN|PROT_GROWSUP)) /* can't be both */
> >> return -EINVAL;
> >> diff --git a/mm/mremap.c b/mm/mremap.c
> >> index fc241d23cd97..64c9a3b8be0a 100644
> >> --- a/mm/mremap.c
> >> +++ b/mm/mremap.c
> >> @@ -606,6 +606,13 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
> >> LIST_HEAD(uf_unmap_early);
> >> LIST_HEAD(uf_unmap);
> >>
> >> + /*
> >> + * Architectures may interpret the tag passed to mmap as a background
> >> + * colour for the corresponding vma. For mremap we don't allow tagged
> >> + * new_addr to preserve similar behaviour to mmap.
> >> + */
> >> + addr = untagged_addr(addr);
> >> +
> >> if (flags & ~(MREMAP_FIXED | MREMAP_MAYMOVE))
> >> return ret;
> >>
> >> diff --git a/mm/msync.c b/mm/msync.c
> >> index ef30a429623a..c3bd3e75f687 100644
> >> --- a/mm/msync.c
> >> +++ b/mm/msync.c
> >> @@ -37,6 +37,8 @@ SYSCALL_DEFINE3(msync, unsigned long, start, size_t, len, int, flags)
> >> int unmapped_error = 0;
> >> int error = -EINVAL;
> >>
> >> + start = untagged_addr(start);
> >> +
> >> if (flags & ~(MS_ASYNC | MS_INVALIDATE | MS_SYNC))
> >> goto out;
> >> if (offset_in_page(start))
> >>
> >
> >
>
>
^ permalink raw reply
* Re: [PATCH rdma-next v1 08/12] IB/mlx5: Introduce MLX5_IB_OBJECT_DEVX_ASYNC_EVENT_FD
From: Jason Gunthorpe @ 2019-06-24 14:30 UTC (permalink / raw)
To: Yishai Hadas
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Yishai Hadas,
Saeed Mahameed, linux-netdev
In-Reply-To: <baae74b9-94ff-9f5a-0992-c1eec5049306@dev.mellanox.co.il>
On Mon, Jun 24, 2019 at 04:25:37PM +0300, Yishai Hadas wrote:
> On 6/24/2019 2:51 PM, Jason Gunthorpe wrote:
> > On Tue, Jun 18, 2019 at 08:15:36PM +0300, Leon Romanovsky wrote:
> > > From: Yishai Hadas <yishaih@mellanox.com>
> > >
> > > Introduce MLX5_IB_OBJECT_DEVX_ASYNC_EVENT_FD and its initial
> > > implementation.
> > >
> > > This object is from type class FD and will be used to read DEVX
> > > async events.
> > >
> > > Signed-off-by: Yishai Hadas <yishaih@mellanox.com>
> > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > > drivers/infiniband/hw/mlx5/devx.c | 112 ++++++++++++++++++++--
> > > include/uapi/rdma/mlx5_user_ioctl_cmds.h | 10 ++
> > > include/uapi/rdma/mlx5_user_ioctl_verbs.h | 4 +
> > > 3 files changed, 116 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/hw/mlx5/devx.c b/drivers/infiniband/hw/mlx5/devx.c
> > > index 80b42d069328..1815ce0f8daf 100644
> > > +++ b/drivers/infiniband/hw/mlx5/devx.c
> > > @@ -33,6 +33,24 @@ struct devx_async_data {
> > > struct mlx5_ib_uapi_devx_async_cmd_hdr hdr;
> > > };
> > > +struct devx_async_event_queue {
> >
> > It seems to be a mistake to try and re-use the async_event_queue for
> > both cmd and event, as they use it very differently and don't even
> > store the same things in the event_list. I think it is bettter to just
> > inline this into devx_async_event_file (and inline the old struct in
> > the cmd file
> >
>
> How about having another struct with all the event's queue fields together ?
> this has the benefit of having all those related fields in one place and
> leave the cmd as is.
>
> Alternatively,
> We can inline the event stuff under devx_async_event_file and leave the cmd
> for now under a struct as it's not directly related to this series.
I would probbaly do this
> > > + spinlock_t lock;
> > > + wait_queue_head_t poll_wait;
> > > + struct list_head event_list;
> > > + atomic_t bytes_in_use;
> > > + u8 is_destroyed:1;
> > > + u32 flags;
> > > +};
> >
> > All the flags testing is ugly, why not just add another bitfield?
>
> The flags are coming from user space and have their different name space, I
> prefer to not mix with kernel ones. (i.e. is_destroyed).
> Makes sense ?
No, better to add a bitfield than store the raw flags and another
bitfield.
> > > diff --git a/include/uapi/rdma/mlx5_user_ioctl_verbs.h b/include/uapi/rdma/mlx5_user_ioctl_verbs.h
> > > index a8f34c237458..57beea4589e4 100644
> > > +++ b/include/uapi/rdma/mlx5_user_ioctl_verbs.h
> > > @@ -63,5 +63,9 @@ enum mlx5_ib_uapi_dm_type {
> > > MLX5_IB_UAPI_DM_TYPE_HEADER_MODIFY_SW_ICM,
> > > };
> > > +enum mlx5_ib_uapi_devx_create_event_channel_flags {
> > > + MLX5_IB_UAPI_DEVX_CREATE_EVENT_CHANNEL_FLAGS_OMIT_EV_DATA = 1
> > > << 0,
> >
> > Maybe this name is too long
>
> Quite long but follows the name scheme having the UAPI prefix.
> Any shorter suggestion ?
>
I think you should shorten it
Jason
^ permalink raw reply
* Re: [PATCH rdma-next v1 09/12] IB/mlx5: Register DEVX with mlx5_core to get async events
From: Jason Gunthorpe @ 2019-06-24 14:30 UTC (permalink / raw)
To: Yishai Hadas
Cc: Leon Romanovsky, Doug Ledford, Leon Romanovsky, RDMA mailing list,
Yishai Hadas, Saeed Mahameed, linux-netdev
In-Reply-To: <3bc6780f-5c3e-b121-e4ea-f7b8f00cbd13@dev.mellanox.co.il>
On Mon, Jun 24, 2019 at 04:36:44PM +0300, Yishai Hadas wrote:
> On 6/24/2019 2:52 PM, Jason Gunthorpe wrote:
> > On Tue, Jun 18, 2019 at 08:15:37PM +0300, Leon Romanovsky wrote:
> > > void __mlx5_ib_remove(struct mlx5_ib_dev *dev,
> > > diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> > > index 9cf23ae6324e..556af34b788b 100644
> > > +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> > > @@ -944,6 +944,13 @@ struct mlx5_ib_pf_eq {
> > > mempool_t *pool;
> > > };
> > > +struct mlx5_devx_event_table {
> > > + struct mlx5_nb devx_nb;
> > > + /* serialize updating the event_xa */
> > > + struct mutex event_xa_lock;
> > > + struct xarray event_xa;
> > > +};
> > > +
> > > struct mlx5_ib_dev {
> > > struct ib_device ib_dev;
> > > struct mlx5_core_dev *mdev;
> > > @@ -994,6 +1001,7 @@ struct mlx5_ib_dev {
> > > struct mlx5_srq_table srq_table;
> > > struct mlx5_async_ctx async_ctx;
> > > int free_port;
> > > + struct mlx5_devx_event_table devx_event_table;
> >
> > I really question if adding all these structs really does anything for
> > readability..
> >
>
> I would prefer this option to add only one structure (i.e.
> mlx5_devx_event_table) on ib_dev, it will hold internally the other related
> stuff.
It seems confounding but generally is the style in this struct :\
Jason
^ permalink raw reply
* [PATCH v18 00/15] arm64: untag user pointers passed to the kernel
From: Andrey Konovalov @ 2019-06-24 14:32 UTC (permalink / raw)
To: linux-arm-kernel, linux-mm, linux-kernel, amd-gfx, dri-devel,
linux-rdma, linux-media, kvm, linux-kselftest
Cc: Catalin Marinas, Vincenzo Frascino, Will Deacon, Mark Rutland,
Andrew Morton, Greg Kroah-Hartman, Kees Cook, Yishai Hadas,
Felix Kuehling, Alexander Deucher, Christian Koenig,
Mauro Carvalho Chehab, Jens Wiklander, Alex Williamson,
Leon Romanovsky, Luc Van Oostenryck, Dave Martin, Khalid Aziz,
enh <enh@
=== Overview
arm64 has a feature called Top Byte Ignore, which allows to embed pointer
tags into the top byte of each pointer. Userspace programs (such as
HWASan, a memory debugging tool [1]) might use this feature and pass
tagged user pointers to the kernel through syscalls or other interfaces.
Right now the kernel is already able to handle user faults with tagged
pointers, due to these patches:
1. 81cddd65 ("arm64: traps: fix userspace cache maintenance emulation on a
tagged pointer")
2. 7dcd9dd8 ("arm64: hw_breakpoint: fix watchpoint matching for tagged
pointers")
3. 276e9327 ("arm64: entry: improve data abort handling of tagged
pointers")
This patchset extends tagged pointer support to syscall arguments.
As per the proposed ABI change [3], tagged pointers are only allowed to be
passed to syscalls when they point to memory ranges obtained by anonymous
mmap() or sbrk() (see the patchset [3] for more details).
For non-memory syscalls this is done by untaging user pointers when the
kernel performs pointer checking to find out whether the pointer comes
from userspace (most notably in access_ok). The untagging is done only
when the pointer is being checked, the tag is preserved as the pointer
makes its way through the kernel and stays tagged when the kernel
dereferences the pointer when perfoming user memory accesses.
The mmap and mremap (only new_addr) syscalls do not currently accept
tagged addresses. Architectures may interpret the tag as a background
colour for the corresponding vma.
Other memory syscalls (mprotect, etc.) don't do user memory accesses but
rather deal with memory ranges, and untagged pointers are better suited to
describe memory ranges internally. Thus for memory syscalls we untag
pointers completely when they enter the kernel.
=== Other approaches
One of the alternative approaches to untagging that was considered is to
completely strip the pointer tag as the pointer enters the kernel with
some kind of a syscall wrapper, but that won't work with the countless
number of different ioctl calls. With this approach we would need a custom
wrapper for each ioctl variation, which doesn't seem practical.
An alternative approach to untagging pointers in memory syscalls prologues
is to inspead allow tagged pointers to be passed to find_vma() (and other
vma related functions) and untag them there. Unfortunately, a lot of
find_vma() callers then compare or subtract the returned vma start and end
fields against the pointer that was being searched. Thus this approach
would still require changing all find_vma() callers.
=== Testing
The following testing approaches has been taken to find potential issues
with user pointer untagging:
1. Static testing (with sparse [2] and separately with a custom static
analyzer based on Clang) to track casts of __user pointers to integer
types to find places where untagging needs to be done.
2. Static testing with grep to find parts of the kernel that call
find_vma() (and other similar functions) or directly compare against
vm_start/vm_end fields of vma.
3. Static testing with grep to find parts of the kernel that compare
user pointers with TASK_SIZE or other similar consts and macros.
4. Dynamic testing: adding BUG_ON(has_tag(addr)) to find_vma() and running
a modified syzkaller version that passes tagged pointers to the kernel.
Based on the results of the testing the requried patches have been added
to the patchset.
=== Notes
This patchset is meant to be merged together with "arm64 relaxed ABI" [3].
This patchset is a prerequisite for ARM's memory tagging hardware feature
support [4].
This patchset has been merged into the Pixel 2 & 3 kernel trees and is
now being used to enable testing of Pixel phones with HWASan.
Thanks!
[1] http://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html
[2] https://github.com/lucvoo/sparse-dev/commit/5f960cb10f56ec2017c128ef9d16060e0145f292
[3] https://lkml.org/lkml/2019/6/12/745
[4] https://community.arm.com/processors/b/blog/posts/arm-a-profile-architecture-2018-developments-armv85a
=== History
Changes in v18:
- Reverted the selftest back to not using the LD_PRELOAD approach.
- Added prctl(PR_SET_TAGGED_ADDR_CTRL) call to the selftest.
- Reworded the patch descriptions to make them less oriented on arm64
only.
- Catalin's patch: "I added a Kconfig option and dropped the prctl args
zero check. There is some minor clean-up as well".
Changes in v17:
- The "uaccess: add noop untagged_addr definition" patch is dropped, as it
was merged into upstream named as "uaccess: add noop untagged_addr
definition".
- Merged "mm, arm64: untag user pointers in do_pages_move" into
"mm, arm64: untag user pointers passed to memory syscalls".
- Added "arm64: Introduce prctl() options to control the tagged user
addresses ABI" patch from Catalin.
- Add tags_lib.so to tools/testing/selftests/arm64/.gitignore.
- Added a comment clarifying untagged in mremap.
- Moved untagging back into mlx4_get_umem_mr() for the IB patch.
Changes in v16:
- Moved untagging for memory syscalls from arm64 wrappers back to generic
code.
- Dropped untagging for the following memory syscalls: brk, mmap, munmap;
mremap (only dropped for new_address); mmap_pgoff (not used on arm64);
remap_file_pages (deprecated); shmat, shmdt (work on shared memory).
- Changed kselftest to LD_PRELOAD a shared library that overrides malloc
to return tagged pointers.
- Rebased onto 5.2-rc3.
Changes in v15:
- Removed unnecessary untagging from radeon_ttm_tt_set_userptr().
- Removed unnecessary untagging from amdgpu_ttm_tt_set_userptr().
- Moved untagging to validate_range() in userfaultfd code.
- Moved untagging to ib_uverbs_(re)reg_mr() from mlx4_get_umem_mr().
- Rebased onto 5.1.
Changes in v14:
- Moved untagging for most memory syscalls to an arm64 specific
implementation, instead of doing that in the common code.
- Dropped "net, arm64: untag user pointers in tcp_zerocopy_receive", since
the provided user pointers don't come from an anonymous map and thus are
not covered by this ABI relaxation.
- Dropped "kernel, arm64: untag user pointers in prctl_set_mm*".
- Moved untagging from __check_mem_type() to tee_shm_register().
- Updated untagging for the amdgpu and radeon drivers to cover the MMU
notifier, as suggested by Felix.
- Since this ABI relaxation doesn't actually allow tagged instruction
pointers, dropped the following patches:
- Dropped "tracing, arm64: untag user pointers in seq_print_user_ip".
- Dropped "uprobes, arm64: untag user pointers in find_active_uprobe".
- Dropped "bpf, arm64: untag user pointers in stack_map_get_build_id_offset".
- Rebased onto 5.1-rc7 (37624b58).
Changes in v13:
- Simplified untagging in tcp_zerocopy_receive().
- Looked at find_vma() callers in drivers/, which allowed to identify a
few other places where untagging is needed.
- Added patch "mm, arm64: untag user pointers in get_vaddr_frames".
- Added patch "drm/amdgpu, arm64: untag user pointers in
amdgpu_ttm_tt_get_user_pages".
- Added patch "drm/radeon, arm64: untag user pointers in
radeon_ttm_tt_pin_userptr".
- Added patch "IB/mlx4, arm64: untag user pointers in mlx4_get_umem_mr".
- Added patch "media/v4l2-core, arm64: untag user pointers in
videobuf_dma_contig_user_get".
- Added patch "tee/optee, arm64: untag user pointers in check_mem_type".
- Added patch "vfio/type1, arm64: untag user pointers".
Changes in v12:
- Changed untagging in tcp_zerocopy_receive() to also untag zc->address.
- Fixed untagging in prctl_set_mm* to only untag pointers for vma lookups
and validity checks, but leave them as is for actual user space accesses.
- Updated the link to the v2 of the "arm64 relaxed ABI" patchset [3].
- Dropped the documentation patch, as the "arm64 relaxed ABI" patchset [3]
handles that.
Changes in v11:
- Added "uprobes, arm64: untag user pointers in find_active_uprobe" patch.
- Added "bpf, arm64: untag user pointers in stack_map_get_build_id_offset"
patch.
- Fixed "tracing, arm64: untag user pointers in seq_print_user_ip" to
correctly perform subtration with a tagged addr.
- Moved untagged_addr() from SYSCALL_DEFINE3(mprotect) and
SYSCALL_DEFINE4(pkey_mprotect) to do_mprotect_pkey().
- Moved untagged_addr() definition for other arches from
include/linux/memory.h to include/linux/mm.h.
- Changed untagging in strn*_user() to perform userspace accesses through
tagged pointers.
- Updated the documentation to mention that passing tagged pointers to
memory syscalls is allowed.
- Updated the test to use malloc'ed memory instead of stack memory.
Changes in v10:
- Added "mm, arm64: untag user pointers passed to memory syscalls" back.
- New patch "fs, arm64: untag user pointers in fs/userfaultfd.c".
- New patch "net, arm64: untag user pointers in tcp_zerocopy_receive".
- New patch "kernel, arm64: untag user pointers in prctl_set_mm*".
- New patch "tracing, arm64: untag user pointers in seq_print_user_ip".
Changes in v9:
- Rebased onto 4.20-rc6.
- Used u64 instead of __u64 in type casts in the untagged_addr macro for
arm64.
- Added braces around (addr) in the untagged_addr macro for other arches.
Changes in v8:
- Rebased onto 65102238 (4.20-rc1).
- Added a note to the cover letter on why syscall wrappers/shims that untag
user pointers won't work.
- Added a note to the cover letter that this patchset has been merged into
the Pixel 2 kernel tree.
- Documentation fixes, in particular added a list of syscalls that don't
support tagged user pointers.
Changes in v7:
- Rebased onto 17b57b18 (4.19-rc6).
- Dropped the "arm64: untag user address in __do_user_fault" patch, since
the existing patches already handle user faults properly.
- Dropped the "usb, arm64: untag user addresses in devio" patch, since the
passed pointer must come from a vma and therefore be untagged.
- Dropped the "arm64: annotate user pointers casts detected by sparse"
patch (see the discussion to the replies of the v6 of this patchset).
- Added more context to the cover letter.
- Updated Documentation/arm64/tagged-pointers.txt.
Changes in v6:
- Added annotations for user pointer casts found by sparse.
- Rebased onto 050cdc6c (4.19-rc1+).
Changes in v5:
- Added 3 new patches that add untagging to places found with static
analysis.
- Rebased onto 44c929e1 (4.18-rc8).
Changes in v4:
- Added a selftest for checking that passing tagged pointers to the
kernel succeeds.
- Rebased onto 81e97f013 (4.18-rc1+).
Changes in v3:
- Rebased onto e5c51f30 (4.17-rc6+).
- Added linux-arch@ to the list of recipients.
Changes in v2:
- Rebased onto 2d618bdf (4.17-rc3+).
- Removed excessive untagging in gup.c.
- Removed untagging pointers returned from __uaccess_mask_ptr.
Changes in v1:
- Rebased onto 4.17-rc1.
Changes in RFC v2:
- Added "#ifndef untagged_addr..." fallback in linux/uaccess.h instead of
defining it for each arch individually.
- Updated Documentation/arm64/tagged-pointers.txt.
- Dropped "mm, arm64: untag user addresses in memory syscalls".
- Rebased onto 3eb2ce82 (4.16-rc7).
Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
Andrey Konovalov (14):
arm64: untag user pointers in access_ok and __uaccess_mask_ptr
lib: untag user pointers in strn*_user
mm: untag user pointers passed to memory syscalls
mm: untag user pointers in mm/gup.c
mm: untag user pointers in get_vaddr_frames
fs/namespace: untag user pointers in copy_mount_options
userfaultfd: untag user pointers
drm/amdgpu: untag user pointers
drm/radeon: untag user pointers in radeon_gem_userptr_ioctl
IB/mlx4: untag user pointers in mlx4_get_umem_mr
media/v4l2-core: untag user pointers in videobuf_dma_contig_user_get
tee/shm: untag user pointers in tee_shm_register
vfio/type1: untag user pointers in vaddr_get_pfn
selftests, arm64: add a selftest for passing tagged pointers to kernel
Catalin Marinas (1):
arm64: Introduce prctl() options to control the tagged user addresses
ABI
arch/arm64/Kconfig | 9 +++
arch/arm64/include/asm/processor.h | 8 +++
arch/arm64/include/asm/thread_info.h | 1 +
arch/arm64/include/asm/uaccess.h | 12 +++-
arch/arm64/kernel/process.c | 72 +++++++++++++++++++
.../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 2 +
drivers/gpu/drm/radeon/radeon_gem.c | 2 +
drivers/infiniband/hw/mlx4/mr.c | 7 +-
drivers/media/v4l2-core/videobuf-dma-contig.c | 9 +--
drivers/tee/tee_shm.c | 1 +
drivers/vfio/vfio_iommu_type1.c | 2 +
fs/namespace.c | 2 +-
fs/userfaultfd.c | 22 +++---
include/uapi/linux/prctl.h | 5 ++
kernel/sys.c | 12 ++++
lib/strncpy_from_user.c | 3 +-
lib/strnlen_user.c | 3 +-
mm/frame_vector.c | 2 +
mm/gup.c | 4 ++
mm/madvise.c | 2 +
mm/mempolicy.c | 3 +
mm/migrate.c | 2 +-
mm/mincore.c | 2 +
mm/mlock.c | 4 ++
mm/mprotect.c | 2 +
mm/mremap.c | 7 ++
mm/msync.c | 2 +
tools/testing/selftests/arm64/.gitignore | 1 +
tools/testing/selftests/arm64/Makefile | 11 +++
.../testing/selftests/arm64/run_tags_test.sh | 12 ++++
tools/testing/selftests/arm64/tags_test.c | 29 ++++++++
32 files changed, 232 insertions(+), 25 deletions(-)
create mode 100644 tools/testing/selftests/arm64/.gitignore
create mode 100644 tools/testing/selftests/arm64/Makefile
create mode 100755 tools/testing/selftests/arm64/run_tags_test.sh
create mode 100644 tools/testing/selftests/arm64/tags_test.c
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply
* [PATCH v18 01/15] arm64: untag user pointers in access_ok and __uaccess_mask_ptr
From: Andrey Konovalov @ 2019-06-24 14:32 UTC (permalink / raw)
To: linux-arm-kernel, linux-mm, linux-kernel, amd-gfx, dri-devel,
linux-rdma, linux-media, kvm, linux-kselftest
Cc: Catalin Marinas, Vincenzo Frascino, Will Deacon, Mark Rutland,
Andrew Morton, Greg Kroah-Hartman, Kees Cook, Yishai Hadas,
Felix Kuehling, Alexander Deucher, Christian Koenig,
Mauro Carvalho Chehab, Jens Wiklander, Alex Williamson,
Leon Romanovsky, Luc Van Oostenryck, Dave Martin, Khalid Aziz,
enh <enh@
In-Reply-To: <cover.1561386715.git.andreyknvl@google.com>
This patch is a part of a series that extends kernel ABI to allow to pass
tagged user pointers (with the top byte set to something else other than
0x00) as syscall arguments.
copy_from_user (and a few other similar functions) are used to copy data
from user memory into the kernel memory or vice versa. Since a user can
provided a tagged pointer to one of the syscalls that use copy_from_user,
we need to correctly handle such pointers.
Do this by untagging user pointers in access_ok and in __uaccess_mask_ptr,
before performing access validity checks.
Note, that this patch only temporarily untags the pointers to perform the
checks, but then passes them as is into the kernel internals.
Reviewed-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
arch/arm64/include/asm/uaccess.h | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index 5a1c32260c1f..a138e3b4f717 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -62,6 +62,8 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si
{
unsigned long ret, limit = current_thread_info()->addr_limit;
+ addr = untagged_addr(addr);
+
__chk_user_ptr(addr);
asm volatile(
// A + B <= C + 1 for all A,B,C, in four easy steps:
@@ -215,7 +217,8 @@ static inline void uaccess_enable_not_uao(void)
/*
* Sanitise a uaccess pointer such that it becomes NULL if above the
- * current addr_limit.
+ * current addr_limit. In case the pointer is tagged (has the top byte set),
+ * untag the pointer before checking.
*/
#define uaccess_mask_ptr(ptr) (__typeof__(ptr))__uaccess_mask_ptr(ptr)
static inline void __user *__uaccess_mask_ptr(const void __user *ptr)
@@ -223,10 +226,11 @@ static inline void __user *__uaccess_mask_ptr(const void __user *ptr)
void __user *safe_ptr;
asm volatile(
- " bics xzr, %1, %2\n"
+ " bics xzr, %3, %2\n"
" csel %0, %1, xzr, eq\n"
: "=&r" (safe_ptr)
- : "r" (ptr), "r" (current_thread_info()->addr_limit)
+ : "r" (ptr), "r" (current_thread_info()->addr_limit),
+ "r" (untagged_addr(ptr))
: "cc");
csdb();
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH v18 02/15] arm64: Introduce prctl() options to control the tagged user addresses ABI
From: Andrey Konovalov @ 2019-06-24 14:32 UTC (permalink / raw)
To: linux-arm-kernel, linux-mm, linux-kernel, amd-gfx, dri-devel,
linux-rdma, linux-media, kvm, linux-kselftest
Cc: Catalin Marinas, Vincenzo Frascino, Will Deacon, Mark Rutland,
Andrew Morton, Greg Kroah-Hartman, Kees Cook, Yishai Hadas,
Felix Kuehling, Alexander Deucher, Christian Koenig,
Mauro Carvalho Chehab, Jens Wiklander, Alex Williamson,
Leon Romanovsky, Luc Van Oostenryck, Dave Martin, Khalid Aziz,
enh <enh@
In-Reply-To: <cover.1561386715.git.andreyknvl@google.com>
From: Catalin Marinas <catalin.marinas@arm.com>
It is not desirable to relax the ABI to allow tagged user addresses into
the kernel indiscriminately. This patch introduces a prctl() interface
for enabling or disabling the tagged ABI with a global sysctl control
for preventing applications from enabling the relaxed ABI (meant for
testing user-space prctl() return error checking without reconfiguring
the kernel). The ABI properties are inherited by threads of the same
application and fork()'ed children but cleared on execve(). A Kconfig
option allows the overall disabling of the relaxed ABI.
The PR_SET_TAGGED_ADDR_CTRL will be expanded in the future to handle
MTE-specific settings like imprecise vs precise exceptions.
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
arch/arm64/Kconfig | 9 ++++
arch/arm64/include/asm/processor.h | 8 ++++
arch/arm64/include/asm/thread_info.h | 1 +
arch/arm64/include/asm/uaccess.h | 4 +-
arch/arm64/kernel/process.c | 72 ++++++++++++++++++++++++++++
include/uapi/linux/prctl.h | 5 ++
kernel/sys.c | 12 +++++
7 files changed, 110 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 697ea0510729..55fbaf20af2d 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1107,6 +1107,15 @@ config ARM64_SW_TTBR0_PAN
zeroed area and reserved ASID. The user access routines
restore the valid TTBR0_EL1 temporarily.
+config ARM64_TAGGED_ADDR_ABI
+ bool "Enable the tagged user addresses syscall ABI"
+ default y
+ help
+ When this option is enabled, user applications can opt in to a
+ relaxed ABI via prctl() allowing tagged addresses to be passed
+ to system calls as pointer arguments. For details, see
+ Documentation/arm64/tagged-address-abi.txt.
+
menuconfig COMPAT
bool "Kernel support for 32-bit EL0"
depends on ARM64_4K_PAGES || EXPERT
diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
index fd5b1a4efc70..ee86070a28d4 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -296,6 +296,14 @@ extern void __init minsigstksz_setup(void);
/* PR_PAC_RESET_KEYS prctl */
#define PAC_RESET_KEYS(tsk, arg) ptrauth_prctl_reset_keys(tsk, arg)
+#ifdef CONFIG_ARM64_TAGGED_ADDR_ABI
+/* PR_{SET,GET}_TAGGED_ADDR_CTRL prctl */
+long set_tagged_addr_ctrl(unsigned long arg);
+long get_tagged_addr_ctrl(void);
+#define SET_TAGGED_ADDR_CTRL(arg) set_tagged_addr_ctrl(arg)
+#define GET_TAGGED_ADDR_CTRL() get_tagged_addr_ctrl()
+#endif
+
/*
* For CONFIG_GCC_PLUGIN_STACKLEAK
*
diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
index 2372e97db29c..4f81c4f15404 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -88,6 +88,7 @@ void arch_release_task_struct(struct task_struct *tsk);
#define TIF_SVE 23 /* Scalable Vector Extension in use */
#define TIF_SVE_VL_INHERIT 24 /* Inherit sve_vl_onexec across exec */
#define TIF_SSBD 25 /* Wants SSB mitigation */
+#define TIF_TAGGED_ADDR 26 /* Allow tagged user addresses */
#define _TIF_SIGPENDING (1 << TIF_SIGPENDING)
#define _TIF_NEED_RESCHED (1 << TIF_NEED_RESCHED)
diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index a138e3b4f717..097d6bfac0b7 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -62,7 +62,9 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si
{
unsigned long ret, limit = current_thread_info()->addr_limit;
- addr = untagged_addr(addr);
+ if (IS_ENABLED(CONFIG_ARM64_TAGGED_ADDR_ABI) &&
+ test_thread_flag(TIF_TAGGED_ADDR))
+ addr = untagged_addr(addr);
__chk_user_ptr(addr);
asm volatile(
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 9856395ccdb7..60e70158a4a1 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -19,6 +19,7 @@
#include <linux/kernel.h>
#include <linux/mm.h>
#include <linux/stddef.h>
+#include <linux/sysctl.h>
#include <linux/unistd.h>
#include <linux/user.h>
#include <linux/delay.h>
@@ -307,11 +308,18 @@ static void tls_thread_flush(void)
}
}
+static void flush_tagged_addr_state(void)
+{
+ if (IS_ENABLED(CONFIG_ARM64_TAGGED_ADDR_ABI))
+ clear_thread_flag(TIF_TAGGED_ADDR);
+}
+
void flush_thread(void)
{
fpsimd_flush_thread();
tls_thread_flush();
flush_ptrace_hw_breakpoint(current);
+ flush_tagged_addr_state();
}
void release_thread(struct task_struct *dead_task)
@@ -541,3 +549,67 @@ void arch_setup_new_exec(void)
ptrauth_thread_init_user(current);
}
+
+#ifdef CONFIG_ARM64_TAGGED_ADDR_ABI
+/*
+ * Control the relaxed ABI allowing tagged user addresses into the kernel.
+ */
+static unsigned int tagged_addr_prctl_allowed = 1;
+
+long set_tagged_addr_ctrl(unsigned long arg)
+{
+ if (!tagged_addr_prctl_allowed)
+ return -EINVAL;
+ if (is_compat_task())
+ return -EINVAL;
+ if (arg & ~PR_TAGGED_ADDR_ENABLE)
+ return -EINVAL;
+
+ update_thread_flag(TIF_TAGGED_ADDR, arg & PR_TAGGED_ADDR_ENABLE);
+
+ return 0;
+}
+
+long get_tagged_addr_ctrl(void)
+{
+ if (!tagged_addr_prctl_allowed)
+ return -EINVAL;
+ if (is_compat_task())
+ return -EINVAL;
+
+ if (test_thread_flag(TIF_TAGGED_ADDR))
+ return PR_TAGGED_ADDR_ENABLE;
+
+ return 0;
+}
+
+/*
+ * Global sysctl to disable the tagged user addresses support. This control
+ * only prevents the tagged address ABI enabling via prctl() and does not
+ * disable it for tasks that already opted in to the relaxed ABI.
+ */
+static int zero;
+static int one = 1;
+
+static struct ctl_table tagged_addr_sysctl_table[] = {
+ {
+ .procname = "tagged_addr",
+ .mode = 0644,
+ .data = &tagged_addr_prctl_allowed,
+ .maxlen = sizeof(int),
+ .proc_handler = proc_dointvec_minmax,
+ .extra1 = &zero,
+ .extra2 = &one,
+ },
+ { }
+};
+
+static int __init tagged_addr_init(void)
+{
+ if (!register_sysctl("abi", tagged_addr_sysctl_table))
+ return -EINVAL;
+ return 0;
+}
+
+core_initcall(tagged_addr_init);
+#endif /* CONFIG_ARM64_TAGGED_ADDR_ABI */
diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
index 094bb03b9cc2..2e927b3e9d6c 100644
--- a/include/uapi/linux/prctl.h
+++ b/include/uapi/linux/prctl.h
@@ -229,4 +229,9 @@ struct prctl_mm_map {
# define PR_PAC_APDBKEY (1UL << 3)
# define PR_PAC_APGAKEY (1UL << 4)
+/* Tagged user address controls for arm64 */
+#define PR_SET_TAGGED_ADDR_CTRL 55
+#define PR_GET_TAGGED_ADDR_CTRL 56
+# define PR_TAGGED_ADDR_ENABLE (1UL << 0)
+
#endif /* _LINUX_PRCTL_H */
diff --git a/kernel/sys.c b/kernel/sys.c
index 2969304c29fe..c6c4d5358bd3 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -124,6 +124,12 @@
#ifndef PAC_RESET_KEYS
# define PAC_RESET_KEYS(a, b) (-EINVAL)
#endif
+#ifndef SET_TAGGED_ADDR_CTRL
+# define SET_TAGGED_ADDR_CTRL(a) (-EINVAL)
+#endif
+#ifndef GET_TAGGED_ADDR_CTRL
+# define GET_TAGGED_ADDR_CTRL() (-EINVAL)
+#endif
/*
* this is where the system-wide overflow UID and GID are defined, for
@@ -2492,6 +2498,12 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
return -EINVAL;
error = PAC_RESET_KEYS(me, arg2);
break;
+ case PR_SET_TAGGED_ADDR_CTRL:
+ error = SET_TAGGED_ADDR_CTRL(arg2);
+ break;
+ case PR_GET_TAGGED_ADDR_CTRL:
+ error = GET_TAGGED_ADDR_CTRL();
+ break;
default:
error = -EINVAL;
break;
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH v18 03/15] lib: untag user pointers in strn*_user
From: Andrey Konovalov @ 2019-06-24 14:32 UTC (permalink / raw)
To: linux-arm-kernel, linux-mm, linux-kernel, amd-gfx, dri-devel,
linux-rdma, linux-media, kvm, linux-kselftest
Cc: Mark Rutland, Szabolcs Nagy, Catalin Marinas, Will Deacon,
Kostya Serebryany, Khalid Aziz, Felix Kuehling, Vincenzo Frascino,
Jacob Bramley, Leon Romanovsky, Christoph Hellwig,
Jason Gunthorpe, Dave Martin, Evgeniy Stepanov, Kevin Brodsky,
Kees Cook, Ruben Ayrapetyan, Andrey Konovalov,
Ramana Radhakrishnan, Alex Williamson, Mauro Carvalho Chehab,
Dmitry Vyukov, Greg
In-Reply-To: <cover.1561386715.git.andreyknvl@google.com>
This patch is a part of a series that extends kernel ABI to allow to pass
tagged user pointers (with the top byte set to something else other than
0x00) as syscall arguments.
strncpy_from_user and strnlen_user accept user addresses as arguments, and
do not go through the same path as copy_from_user and others, so here we
need to handle the case of tagged user addresses separately.
Untag user pointers passed to these functions.
Note, that this patch only temporarily untags the pointers to perform
validity checks, but then uses them as is to perform user memory accesses.
Reviewed-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
Reviewed-by: Khalid Aziz <khalid.aziz@oracle.com>
Acked-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
lib/strncpy_from_user.c | 3 ++-
lib/strnlen_user.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
index 023ba9f3b99f..dccb95af6003 100644
--- a/lib/strncpy_from_user.c
+++ b/lib/strncpy_from_user.c
@@ -6,6 +6,7 @@
#include <linux/uaccess.h>
#include <linux/kernel.h>
#include <linux/errno.h>
+#include <linux/mm.h>
#include <asm/byteorder.h>
#include <asm/word-at-a-time.h>
@@ -108,7 +109,7 @@ long strncpy_from_user(char *dst, const char __user *src, long count)
return 0;
max_addr = user_addr_max();
- src_addr = (unsigned long)src;
+ src_addr = (unsigned long)untagged_addr(src);
if (likely(src_addr < max_addr)) {
unsigned long max = max_addr - src_addr;
long retval;
diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c
index 7f2db3fe311f..28ff554a1be8 100644
--- a/lib/strnlen_user.c
+++ b/lib/strnlen_user.c
@@ -2,6 +2,7 @@
#include <linux/kernel.h>
#include <linux/export.h>
#include <linux/uaccess.h>
+#include <linux/mm.h>
#include <asm/word-at-a-time.h>
@@ -109,7 +110,7 @@ long strnlen_user(const char __user *str, long count)
return 0;
max_addr = user_addr_max();
- src_addr = (unsigned long)str;
+ src_addr = (unsigned long)untagged_addr(str);
if (likely(src_addr < max_addr)) {
unsigned long max = max_addr - src_addr;
long retval;
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH v18 04/15] mm: untag user pointers passed to memory syscalls
From: Andrey Konovalov @ 2019-06-24 14:32 UTC (permalink / raw)
To: linux-arm-kernel, linux-mm, linux-kernel, amd-gfx, dri-devel,
linux-rdma, linux-media, kvm, linux-kselftest
Cc: Catalin Marinas, Vincenzo Frascino, Will Deacon, Mark Rutland,
Andrew Morton, Greg Kroah-Hartman, Kees Cook, Yishai Hadas,
Felix Kuehling, Alexander Deucher, Christian Koenig,
Mauro Carvalho Chehab, Jens Wiklander, Alex Williamson,
Leon Romanovsky, Luc Van Oostenryck, Dave Martin, Khalid Aziz,
enh <enh@
In-Reply-To: <cover.1561386715.git.andreyknvl@google.com>
This patch is a part of a series that extends kernel ABI to allow to pass
tagged user pointers (with the top byte set to something else other than
0x00) as syscall arguments.
This patch allows tagged pointers to be passed to the following memory
syscalls: get_mempolicy, madvise, mbind, mincore, mlock, mlock2, mprotect,
mremap, msync, munlock, move_pages.
The mmap and mremap syscalls do not currently accept tagged addresses.
Architectures may interpret the tag as a background colour for the
corresponding vma.
Reviewed-by: Khalid Aziz <khalid.aziz@oracle.com>
Reviewed-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
mm/madvise.c | 2 ++
mm/mempolicy.c | 3 +++
mm/migrate.c | 2 +-
mm/mincore.c | 2 ++
mm/mlock.c | 4 ++++
mm/mprotect.c | 2 ++
mm/mremap.c | 7 +++++++
mm/msync.c | 2 ++
8 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/mm/madvise.c b/mm/madvise.c
index 628022e674a7..39b82f8a698f 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -810,6 +810,8 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
size_t len;
struct blk_plug plug;
+ start = untagged_addr(start);
+
if (!madvise_behavior_valid(behavior))
return error;
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 01600d80ae01..78e0a88b2680 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1360,6 +1360,7 @@ static long kernel_mbind(unsigned long start, unsigned long len,
int err;
unsigned short mode_flags;
+ start = untagged_addr(start);
mode_flags = mode & MPOL_MODE_FLAGS;
mode &= ~MPOL_MODE_FLAGS;
if (mode >= MPOL_MAX)
@@ -1517,6 +1518,8 @@ static int kernel_get_mempolicy(int __user *policy,
int uninitialized_var(pval);
nodemask_t nodes;
+ addr = untagged_addr(addr);
+
if (nmask != NULL && maxnode < nr_node_ids)
return -EINVAL;
diff --git a/mm/migrate.c b/mm/migrate.c
index f2ecc2855a12..d22c45cf36b2 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1616,7 +1616,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
goto out_flush;
if (get_user(node, nodes + i))
goto out_flush;
- addr = (unsigned long)p;
+ addr = (unsigned long)untagged_addr(p);
err = -ENODEV;
if (node < 0 || node >= MAX_NUMNODES)
diff --git a/mm/mincore.c b/mm/mincore.c
index c3f058bd0faf..64c322ed845c 100644
--- a/mm/mincore.c
+++ b/mm/mincore.c
@@ -249,6 +249,8 @@ SYSCALL_DEFINE3(mincore, unsigned long, start, size_t, len,
unsigned long pages;
unsigned char *tmp;
+ start = untagged_addr(start);
+
/* Check the start address: needs to be page-aligned.. */
if (start & ~PAGE_MASK)
return -EINVAL;
diff --git a/mm/mlock.c b/mm/mlock.c
index a90099da4fb4..a72c1eeded77 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -674,6 +674,8 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla
unsigned long lock_limit;
int error = -ENOMEM;
+ start = untagged_addr(start);
+
if (!can_do_mlock())
return -EPERM;
@@ -735,6 +737,8 @@ SYSCALL_DEFINE2(munlock, unsigned long, start, size_t, len)
{
int ret;
+ start = untagged_addr(start);
+
len = PAGE_ALIGN(len + (offset_in_page(start)));
start &= PAGE_MASK;
diff --git a/mm/mprotect.c b/mm/mprotect.c
index bf38dfbbb4b4..19f981b733bc 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -465,6 +465,8 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
const bool rier = (current->personality & READ_IMPLIES_EXEC) &&
(prot & PROT_READ);
+ start = untagged_addr(start);
+
prot &= ~(PROT_GROWSDOWN|PROT_GROWSUP);
if (grows == (PROT_GROWSDOWN|PROT_GROWSUP)) /* can't be both */
return -EINVAL;
diff --git a/mm/mremap.c b/mm/mremap.c
index fc241d23cd97..64c9a3b8be0a 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -606,6 +606,13 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
LIST_HEAD(uf_unmap_early);
LIST_HEAD(uf_unmap);
+ /*
+ * Architectures may interpret the tag passed to mmap as a background
+ * colour for the corresponding vma. For mremap we don't allow tagged
+ * new_addr to preserve similar behaviour to mmap.
+ */
+ addr = untagged_addr(addr);
+
if (flags & ~(MREMAP_FIXED | MREMAP_MAYMOVE))
return ret;
diff --git a/mm/msync.c b/mm/msync.c
index ef30a429623a..c3bd3e75f687 100644
--- a/mm/msync.c
+++ b/mm/msync.c
@@ -37,6 +37,8 @@ SYSCALL_DEFINE3(msync, unsigned long, start, size_t, len, int, flags)
int unmapped_error = 0;
int error = -EINVAL;
+ start = untagged_addr(start);
+
if (flags & ~(MS_ASYNC | MS_INVALIDATE | MS_SYNC))
goto out;
if (offset_in_page(start))
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox