linux-kernel - Re: [PATCH 3/3 v6] IB: expand ib_umem

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <adar6dgm8yi.fsf@cisco.com>
Date:	Tue, 08 Apr 2008 12:40:21 -0700
From:	Roland Dreier <rdreier@...co.com>
To:	akepner@....com
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Tony Luck <tony.luck@...el.com>,
	Jesse Barnes <jbarnes@...tuousgeek.org>,
	Jes Sorensen <jes@....com>,
	Randy Dunlap <randy.dunlap@...cle.com>,
	James Bottomley <James.Bottomley@...senPartnership.com>,
	David Miller <davem@...emloft.net>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Grant Grundler <grundler@...isc-linux.org>,
	Michael Ellerman <michael@...erman.id.au>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3 v6] IB: expand ib_umem_get() prototype

All of this looks good to me, and I'm happy we are finally fixing this
up.  However, now that it is finally ready to merge, I started thinking
about the libmthca changes required to handle this, and I realized that
we can actually avoid this:

 > --- a/drivers/infiniband/hw/mthca/mthca_user.h
 > +++ b/drivers/infiniband/hw/mthca/mthca_user.h
 > @@ -41,7 +41,7 @@
 >   * Increment this value if any changes that break userspace ABI
 >   * compatibility are made.
 >   */
 > -#define MTHCA_UVERBS_ABI_VERSION	1
 > +#define MTHCA_UVERBS_ABI_VERSION	2

Instead the mthca kernel driver can look at the size of the data passed
in from userspace, and fall back to the old behavior if it's talking to
old userspace.

I think this is a net win.  The current patch gives the following:

 old kernel, old userspace: current behavior (subtle failure on Altix)
 new kernel, old userspace: refuses to run (on all systems)
 old kernel, new userspace: current behavior (broken on Altix),
 new kernel, new userspace: works everywhere

with my proposed change, the only difference is:

 new kernel, old userspace: current behavior (subtle failure on Altix),
                            plus print a warning telling people to
                            update userspace.

and given that the vast majority of the systems where mthca is used are
*not* Altix systems, it seems more friendly to leave them working across
a kernel update.

Andrew: if Arthur agrees with this, please roll the patch below into the
current IB patch you have as part of the dma attrs series.

Thanks,
  Roland

diff --git a/drivers/infiniband/hw/mthca/mthca_provider.c b/drivers/infiniband/hw/mthca/mthca_provider.c
index ca5bab0..d2e1429 100644
--- a/drivers/infiniband/hw/mthca/mthca_provider.c
+++ b/drivers/infiniband/hw/mthca/mthca_provider.c
@@ -367,6 +367,8 @@ static struct ib_ucontext *mthca_alloc_ucontext(struct ib_device *ibdev,
 		return ERR_PTR(-EFAULT);
 	}
 
+	context->reg_mr_warned = 0;
+
 	return &context->ibucontext;
 }
 
@@ -1013,7 +1015,15 @@ static struct ib_mr *mthca_reg_user_mr(struct ib_pd *pd, u64 start, u64 length,
 	int err = 0;
 	int write_mtt_size;
 
-	if (ib_copy_from_udata(&ucmd, udata, sizeof ucmd))
+	if (udata->inlen < sizeof ucm) {
+		if (!to_mucontext(pd->uobject->context)->reg_mr_warned) {
+			mthca_warn(dev, "Process %s did not pass in MR attrs\n",
+				   current->comm);
+			mthca_warn(dev, "  Update libmthca to fix this.\n");
+		}
+		++to_mucontext(pd->uobject->context)->reg_mr_warned;
+		ucmd.mr_attrs = 0;
+	} else if (ib_copy_from_udata(&ucmd, udata, sizeof ucmd))
 		return ERR_PTR(-EFAULT);
 
 	mr = kmalloc(sizeof *mr, GFP_KERNEL);
diff --git a/drivers/infiniband/hw/mthca/mthca_provider.h b/drivers/infiniband/hw/mthca/mthca_provider.h
index 262616c..934bf95 100644
--- a/drivers/infiniband/hw/mthca/mthca_provider.h
+++ b/drivers/infiniband/hw/mthca/mthca_provider.h
@@ -67,6 +67,7 @@ struct mthca_ucontext {
 	struct ib_ucontext          ibucontext;
 	struct mthca_uar            uar;
 	struct mthca_user_db_table *db_tab;
+	int			    reg_mr_warned;
 };
 
 struct mthca_mtt;
diff --git a/drivers/infiniband/hw/mthca/mthca_user.h b/drivers/infiniband/hw/mthca/mthca_user.h
index f8cb3b6..e1262c9 100644
--- a/drivers/infiniband/hw/mthca/mthca_user.h
+++ b/drivers/infiniband/hw/mthca/mthca_user.h
@@ -41,7 +41,7 @@
  * Increment this value if any changes that break userspace ABI
  * compatibility are made.
  */
-#define MTHCA_UVERBS_ABI_VERSION	2
+#define MTHCA_UVERBS_ABI_VERSION	1
 
 /*
  * Make sure that all structs defined in this file remain laid out so
@@ -62,10 +62,12 @@ struct mthca_alloc_pd_resp {
 };
 
 struct mthca_reg_mr {
+/*
+ * Mark the memory region with a DMA attribute that causes
+ * in-flight DMA to be flushed when the region is written to:
+ */
+#define MTHCA_MR_DMASYNC	0x1
 	__u32 mr_attrs;
-#define MTHCA_MR_DMASYNC 0x1
-/* mark the memory region with a DMA attribute that causes
- * in-flight DMA to be flushed when the region is written to */
 	__u32 reserved;
 };
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/