From patchwork Thu Jan 29 21:59:01 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Yann Droneaud X-Patchwork-Id: 5745481 Return-Path: X-Original-To: patchwork-linux-rdma@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 1E9E49F358 for ; Thu, 29 Jan 2015 21:59:15 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 373FA20225 for ; Thu, 29 Jan 2015 21:59:14 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 2AF5D2024C for ; Thu, 29 Jan 2015 21:59:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753418AbbA2V7L (ORCPT ); Thu, 29 Jan 2015 16:59:11 -0500 Received: from ou.quest-ce.net ([195.154.187.82]:45459 "EHLO ou.quest-ce.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751877AbbA2V7K (ORCPT ); Thu, 29 Jan 2015 16:59:10 -0500 Received: from [2a01:e35:2e9f:6ac0:e9f2:c372:e9da:cf57] (helo=test.quest-ce.net) by ou.quest-ce.net with esmtpsa (TLS1.1:RSA_AES_128_CBC_SHA1:128) (Exim 4.80) (envelope-from ) id 1YGx78-000Cff-Hf; Thu, 29 Jan 2015 22:59:03 +0100 Message-ID: <1422568741.3133.247.camel@opteya.com> From: Yann Droneaud To: Jason Gunthorpe Cc: Sagi Grimberg , Shachar Raindel , Eli Cohen , Haggai Eran , Roland Dreier , linux-rdma@vger.kernel.org, linux-api@vger.kernel.org Date: Thu, 29 Jan 2015 22:59:01 +0100 In-Reply-To: <20150129182800.GH11842@obsidianresearch.com> References: <24ceb1fc5b2b6563532e5776b1b2320b1ae37543.1422553023.git.ydroneaud@opteya.com> <20150129182800.GH11842@obsidianresearch.com> Organization: OPTEYA X-Mailer: Evolution 3.12.10 (3.12.10-1.fc21) Mime-Version: 1.0 X-SA-Exim-Connect-IP: 2a01:e35:2e9f:6ac0:e9f2:c372:e9da:cf57 X-SA-Exim-Mail-From: ydroneaud@opteya.com X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 Subject: Re: [PATCH v1 1/5] IB/uverbs: ex_query_device: answer must not depend on request's comp_mask X-SA-Exim-Version: 4.2.1 (built Mon, 26 Dec 2011 16:24:06 +0000) X-SA-Exim-Scanned: Yes (on ou.quest-ce.net) Sender: linux-rdma-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-rdma@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hi, Le jeudi 29 janvier 2015 à 11:28 -0700, Jason Gunthorpe a écrit : > On Thu, Jan 29, 2015 at 06:59:58PM +0100, Yann Droneaud wrote: > > As specified in "Extending Verbs API" presentation [1] by Tzahi Oved > > during OFA International Developer Workshop 2013, the request's > > comp_mask should describe the request data: it's describe the > > availability of extended fields in the request. > > Conversely, the response's comp_mask should describe the presence > > of extended fields in the response. > > Roland: I agree with Yann, these patches need to go in, or the ODP > patches reverted. > Reverting all On Demand Paging patches seems overkill: if something as to be reverted it should be commit 5a77abf9a97a ("IB/core: Add support for extended query device caps") and the part of commit 860f10a799c8 ("IB/core: Add flags for on demand paging support") which modify ib_uverbs_ex_query_device(). But I wonder about this part of commit 860f10a799c8: for some coherency between a subset of the bits (it's not a function dedicated to check flags provided by userspace): include/rdma/ib_verbs.h: 1094 enum ib_access_flags { 1095 IB_ACCESS_LOCAL_WRITE = 1, 1096 IB_ACCESS_REMOTE_WRITE = (1<<1), 1097 IB_ACCESS_REMOTE_READ = (1<<2), 1098 IB_ACCESS_REMOTE_ATOMIC = (1<<3), 1099 IB_ACCESS_MW_BIND = (1<<4), 1100 IB_ZERO_BASED = (1<<5), 1101 IB_ACCESS_ON_DEMAND = (1<<6), 1102 }; drivers/infiniband/core/uverbs_cmd.c: ib_uverbs_reg_mr() 961 ret = ib_check_mr_access(cmd.access_flags); 962 if (ret) 963 return ret; include/rdma/ib_verbs.h: 2643 static inline int ib_check_mr_access(int flags) 2644 { 2645 /* 2646 * Local write permission is required if remote write or 2647 * remote atomic permission is also requested. 2648 */ 2649 if (flags & (IB_ACCESS_REMOTE_ATOMIC | IB_ACCESS_REMOTE_WRITE) && 2650 !(flags & IB_ACCESS_LOCAL_WRITE)) 2651 return -EINVAL; 2652 2653 return 0; 2654 } drivers/infiniband/core/uverbs_cmd.c: ib_uverbs_reg_mr() 990 mr = pd->device->reg_user_mr(pd, cmd.start, cmd.length, cmd.hca_va, 991 cmd.access_flags, &udata); reg_user_mr() functions may call ib_umem_get() and pass access_flags to it: drivers/infiniband/core/umem.c: ib_umem_get() 114 /* 115 * We ask for writable memory if any of the following 116 * access flags are set. "Local write" and "remote write" 117 * obviously require write access. "Remote atomic" can do 118 * things like fetch and add, which will modify memory, and 119 * "MW bind" can change permissions by binding a window. 120 */ 121 umem->writable = !!(access & 122 (IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_WRITE | 123 IB_ACCESS_REMOTE_ATOMIC | IB_ACCESS_MW_BIND)); 124 125 if (access & IB_ACCESS_ON_DEMAND) { 126 ret = ib_umem_odp_get(context, umem); 127 if (ret) { 128 kfree(umem); 129 return ERR_PTR(ret); 130 } 131 return umem; 132 } As you can see only a few bits in access_flags are checked in the end, so it may exist a very unlikely possibility that an existing userspace program is setting this IB_ACCESS_ON_DEMAND bit without the intention of enabling on demand paging as this would be unnoticed by older kernel. In the other hand, a newer program built with on-demand-paging in mind will set the bit, but when run on older kernel, it will be a no-op, allowing the program to continue, perhaps thinking on-demand-paging is available. That should be avoided as much as possible. Unfortunately, I think this cannot be fixed as it's was long since IB_ZERO_BASED was added by commit 7083e42ee2 ("IB/core: Add "type 2" memory windows support"). Anyway there was no check for IB_ACCESS_REMOTE_READ, nor IB_ACCESS_MW_BIND in the uverb layer either. So, just as the second argument of open() syscall (remember O_TMPFILE, see http://lwn.net/Articles/562294/ ), we will have to live with and be careful ... Regards. diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c index c7a43624c96b..f9326ccda4b5 100644 --- a/drivers/infiniband/core/uverbs_cmd.c +++ b/drivers/infiniband/core/uverbs_cmd.c @@ -953,6 +953,18 @@ ssize_t ib_uverbs_reg_mr(struct ib_uverbs_file *file, goto err_free; } + if (cmd.access_flags & IB_ACCESS_ON_DEMAND) { + struct ib_device_attr attr; + + ret = ib_query_device(pd->device, &attr); + if (ret || !(attr.device_cap_flags & + IB_DEVICE_ON_DEMAND_PAGING)) { + pr_debug("ODP support not available\n"); + ret = -EINVAL; + goto err_put; + } + } + AFAICT (1 << 6) bit from struct ib_uverbs_reg_mr access_flags field was not enforced to be 0 previously, as ib_check_mr_access() only check