2006-02-04 16:34:05

by Roland Dreier

[permalink] [raw]
Subject: [git patch review 1/2] IB/mad: Handle DR SMPs with a LID routed part

Fix handling of directed route SMPs with a beginning or ending LID
routed part.

Signed-off-by: Ralph Campbell <[email protected]>
Signed-off-by: Hal Rosenstock <[email protected]>
Signed-off-by: Roland Dreier <[email protected]>

---

drivers/infiniband/core/mad.c | 10 +++++++++-
1 files changed, 9 insertions(+), 1 deletions(-)

8cf3f04f45694db0699f608c0e3fb550c607cc88
diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c
index d393b50..c82f47a 100644
--- a/drivers/infiniband/core/mad.c
+++ b/drivers/infiniband/core/mad.c
@@ -665,7 +665,15 @@ static int handle_outgoing_dr_smp(struct
struct ib_wc mad_wc;
struct ib_send_wr *send_wr = &mad_send_wr->send_wr;

- if (!smi_handle_dr_smp_send(smp, device->node_type, port_num)) {
+ /*
+ * Directed route handling starts if the initial LID routed part of
+ * a request or the ending LID routed part of a response is empty.
+ * If we are at the start of the LID routed part, don't update the
+ * hop_ptr or hop_cnt. See section 14.2.2, Vol 1 IB spec.
+ */
+ if ((ib_get_smp_direction(smp) ? smp->dr_dlid : smp->dr_slid) ==
+ IB_LID_PERMISSIVE &&
+ !smi_handle_dr_smp_send(smp, device->node_type, port_num)) {
ret = -EINVAL;
printk(KERN_ERR PFX "Invalid directed route\n");
goto out;
--
1.1.3


2006-02-04 16:34:04

by Roland Dreier

[permalink] [raw]
Subject: [git patch review 2/2] IB: Don't doublefree pages from scatterlist

On some architectures, mapping the scatterlist may coalesce entries:
if that coalesced list is then used for freeing the pages afterwards,
there's a danger that pages may be doubly freed (and others leaked).

Fix Infiniband's __ib_umem_release by freeing from a separate array
beyond the scatterlist: IB_UMEM_MAX_PAGE_CHUNK lowered to fit one page.

Signed-off-by: Hugh Dickins <[email protected]>
Signed-off-by: Roland Dreier <[email protected]>

---

drivers/infiniband/core/uverbs_mem.c | 22 ++++++++++++++++------
include/rdma/ib_verbs.h | 3 +--
2 files changed, 17 insertions(+), 8 deletions(-)

46fc99a4a1429f843e3b6df8ed1f571944bef4e2
diff --git a/drivers/infiniband/core/uverbs_mem.c b/drivers/infiniband/core/uverbs_mem.c
index 36a32c3..87a363e 100644
--- a/drivers/infiniband/core/uverbs_mem.c
+++ b/drivers/infiniband/core/uverbs_mem.c
@@ -49,15 +49,18 @@ struct ib_umem_account_work {
static void __ib_umem_release(struct ib_device *dev, struct ib_umem *umem, int dirty)
{
struct ib_umem_chunk *chunk, *tmp;
+ struct page **sg_pages;
int i;

list_for_each_entry_safe(chunk, tmp, &umem->chunk_list, list) {
dma_unmap_sg(dev->dma_device, chunk->page_list,
chunk->nents, DMA_BIDIRECTIONAL);
+ /* Scatterlist may have been coalesced: free saved pagelist */
+ sg_pages = (struct page **) (chunk->page_list + chunk->nents);
for (i = 0; i < chunk->nents; ++i) {
if (umem->writable && dirty)
- set_page_dirty_lock(chunk->page_list[i].page);
- put_page(chunk->page_list[i].page);
+ set_page_dirty_lock(sg_pages[i]);
+ put_page(sg_pages[i]);
}

kfree(chunk);
@@ -69,11 +72,13 @@ int ib_umem_get(struct ib_device *dev, s
{
struct page **page_list;
struct ib_umem_chunk *chunk;
+ struct page **sg_pages;
unsigned long locked;
unsigned long lock_limit;
unsigned long cur_base;
unsigned long npages;
int ret = 0;
+ int nents;
int off;
int i;

@@ -121,16 +126,21 @@ int ib_umem_get(struct ib_device *dev, s
off = 0;

while (ret) {
- chunk = kmalloc(sizeof *chunk + sizeof (struct scatterlist) *
- min_t(int, ret, IB_UMEM_MAX_PAGE_CHUNK),
+ nents = min_t(int, ret, IB_UMEM_MAX_PAGE_CHUNK);
+ chunk = kmalloc(sizeof *chunk +
+ sizeof (struct scatterlist) * nents +
+ sizeof (struct page *) * nents,
GFP_KERNEL);
if (!chunk) {
ret = -ENOMEM;
goto out;
}
+ /* Save pages to be freed in array beyond scatterlist */
+ sg_pages = (struct page **) (chunk->page_list + nents);

- chunk->nents = min_t(int, ret, IB_UMEM_MAX_PAGE_CHUNK);
+ chunk->nents = nents;
for (i = 0; i < chunk->nents; ++i) {
+ sg_pages[i] = page_list[i + off];
chunk->page_list[i].page = page_list[i + off];
chunk->page_list[i].offset = 0;
chunk->page_list[i].length = PAGE_SIZE;
@@ -142,7 +152,7 @@ int ib_umem_get(struct ib_device *dev, s
DMA_BIDIRECTIONAL);
if (chunk->nmap <= 0) {
for (i = 0; i < chunk->nents; ++i)
- put_page(chunk->page_list[i].page);
+ put_page(sg_pages[i]);
kfree(chunk);

ret = -ENOMEM;
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 22fc886..239c11d 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -696,8 +696,7 @@ struct ib_udata {

#define IB_UMEM_MAX_PAGE_CHUNK \
((PAGE_SIZE - offsetof(struct ib_umem_chunk, page_list)) / \
- ((void *) &((struct ib_umem_chunk *) 0)->page_list[1] - \
- (void *) &((struct ib_umem_chunk *) 0)->page_list[0]))
+ (sizeof (struct scatterlist) + sizeof (struct page *)))

struct ib_umem_object {
struct ib_uobject uobject;
--
1.1.3

2006-02-06 22:29:16

by Hugh Dickins

[permalink] [raw]
Subject: Re: [git patch review 2/2] IB: Don't doublefree pages from scatterlist

On Sat, 4 Feb 2006, Roland Dreier wrote:

> On some architectures, mapping the scatterlist may coalesce entries:
> if that coalesced list is then used for freeing the pages afterwards,
> there's a danger that pages may be doubly freed (and others leaked).
>
> Fix Infiniband's __ib_umem_release by freeing from a separate array
> beyond the scatterlist: IB_UMEM_MAX_PAGE_CHUNK lowered to fit one page.

It's now looking like this change won't be needed after all: Andi has
just posted a patch in the "ipr" thread which should stop x86_64 from
interfering with the scatterlist *page,offset,length fields, so what
IB and others were doing should then work safely (current thinking is
that x86_64 is the only architecture which coalesced in that way).

Hugh

2006-02-07 03:50:27

by Roland Dreier

[permalink] [raw]
Subject: Re: [git patch review 2/2] IB: Don't doublefree pages from scatterlist

Hugh> It's now looking like this change won't be needed after all:
Hugh> Andi has just posted a patch in the "ipr" thread which
Hugh> should stop x86_64 from interfering with the scatterlist
Hugh> *page,offset,length fields, so what IB and others were doing
Hugh> should then work safely (current thinking is that x86_64 is
Hugh> the only architecture which coalesced in that way).

OK, I'll drop this from my tree.

- R.