From patchwork Wed Jul 11 02:55:21 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jason Gunthorpe X-Patchwork-Id: 10518709 X-Patchwork-Delegate: jgg@ziepe.ca Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id A60FF6020F for ; Wed, 11 Jul 2018 02:55:41 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 9758628BD3 for ; Wed, 11 Jul 2018 02:55:41 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 8BE2B28CC7; Wed, 11 Jul 2018 02:55:41 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI, T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E5D2928CEF for ; Wed, 11 Jul 2018 02:55:40 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732545AbeGKC5o (ORCPT ); Tue, 10 Jul 2018 22:57:44 -0400 Received: from mail-wr1-f67.google.com ([209.85.221.67]:39749 "EHLO mail-wr1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1732411AbeGKC5o (ORCPT ); Tue, 10 Jul 2018 22:57:44 -0400 Received: by mail-wr1-f67.google.com with SMTP id h10-v6so16516984wre.6 for ; Tue, 10 Jul 2018 19:55:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ziepe.ca; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=xTNc/qH9lYof7d3uhnI7yR/J+mJYJA/oGFroN7xoXNs=; b=nQk5lZRUi36YD896htfCgNk6l+I6qPUig+iJpi1J1PZrBhwdzQSdsf96afohZ/NOo1 A70UOko1w6ZqpyD/qMf+g1e6+FLlMAfHos2+KOlukDCf44P5eYJqJO/R0dlorN6oNMju jL7xwDwwd5hlKfl1758MA8o8bcRRgljaJ1b8vwg+slvG8yoVW9a//kWkwW1uuQDSSbCb 2waqM0UeHAEoeCFBRTSWSHAaVSOhfM/6A+7qZ7I/j/+9BMKky+VMtnatdbOxS4oExcMP sYXI60U5Trct+xe/By6uAWhey/bxm9TWBrBMZFb5PXL7nUj3IaJ+XrZGJB7H1pMiFV0g AxVg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=xTNc/qH9lYof7d3uhnI7yR/J+mJYJA/oGFroN7xoXNs=; b=rNYblK4cpvEsAKeqjyA8aU0e6a+brUVgEFyehalNzBgHaltZGe4dz8hptaFQe108MG 1c2rLaP+mlygO9N5oluqIGyqIecsnNHJ08D4JtIhwUwQCVHdb5lRP2fsl8Ha9vceaMIo bV1Aetb/t5/V+6Ic9OxsZJ6XYhYY8E0JaxpUs6ncViR8HwDDkjCt8GxHbafXz9QFvszd vuo4LzJfmfytTwKG4V38BgnSUt+oIJEQeMY5pBN63lO/Iqa/1zr6zfELtdhlK0uG9Anw 6jqfgobT7oiIalyvIOKFAiLo6Km3Jj3g/CrJeAvBfASyt2FjODlUzTyvJniJWoEq1qgh t6xA== X-Gm-Message-State: AOUpUlEfRDgxScZT6p1Y2B0mo7qK2o75KUcH7an9Io/d8UdqjBV5EnIR xoMKa2hdvkLNCxpWv7jyAdFei6pogAk= X-Google-Smtp-Source: AAOMgpcUxas+c34gYHBiMOuzWvFsnZKkbt4ID98DKc3h0wqc8ppGQVEtUhHyRdKyZS94+z/jdvFsvQ== X-Received: by 2002:adf:ef03:: with SMTP id e3-v6mr9859844wro.182.1531277736430; Tue, 10 Jul 2018 19:55:36 -0700 (PDT) Received: from ziepe.ca (S010614cc2056d97f.ed.shawcable.net. [174.3.196.123]) by smtp.gmail.com with ESMTPSA id k3-v6sm752266wme.44.2018.07.10.19.55.31 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 10 Jul 2018 19:55:32 -0700 (PDT) Received: from jgg by mlx.ziepe.ca with local (Exim 4.86_2) (envelope-from ) id 1fd5Hs-0007rU-GB; Tue, 10 Jul 2018 20:55:28 -0600 From: Jason Gunthorpe To: linux-rdma@vger.kernel.org, Leon Romanovsky , "Guy Levi(SW)" , Yishai Hadas , "Ruhl, Michael J" Cc: Jason Gunthorpe Subject: [PATCH 09/11] IB/uverbs: Move the FD uobj type struct file allocation to alloc_commit Date: Tue, 10 Jul 2018 20:55:21 -0600 Message-Id: <20180711025523.30102-10-jgg@ziepe.ca> X-Mailer: git-send-email 2.18.0 In-Reply-To: <20180711025523.30102-1-jgg@ziepe.ca> References: <20180711025523.30102-1-jgg@ziepe.ca> 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 From: Jason Gunthorpe Allocating the struct file during alloc_begin creates this strange asymmetry with IDR, where the FD has two krefs pointing at it during the pre-commit phase. In particular this makes the abort process for FD very strange and confusing. For instance abort currently calls the type's destroy_object twice, and the fops release once if abort is done. This is very counter intuitive. No fops should be called until alloc_commit succeeds, and destroy_object should only ever be called once. Moving the struct file allocation to the alloc_commit is now simple, as we already support failure of rdma_alloc_commit_uobject, with all the required rollback pieces. This creates an understandable symmetry with IDR and simplifies/fixes the abort handling for FD types. Signed-off-by: Jason Gunthorpe --- drivers/infiniband/core/rdma_core.c | 83 ++++++++++++++++------------- include/rdma/uverbs_types.h | 2 +- 2 files changed, 47 insertions(+), 38 deletions(-) diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c index 2aab8cd2ca6bd7..8a6ce66d4726f5 100644 --- a/drivers/infiniband/core/rdma_core.c +++ b/drivers/infiniband/core/rdma_core.c @@ -328,11 +328,8 @@ static struct ib_uobject *alloc_begin_idr_uobject(const struct uverbs_obj_type * static struct ib_uobject *alloc_begin_fd_uobject(const struct uverbs_obj_type *type, struct ib_uverbs_file *ufile) { - const struct uverbs_obj_fd_type *fd_type = - container_of(type, struct uverbs_obj_fd_type, type); int new_fd; struct ib_uobject *uobj; - struct file *filp; new_fd = get_unused_fd_flags(O_CLOEXEC); if (new_fd < 0) @@ -344,28 +341,8 @@ static struct ib_uobject *alloc_begin_fd_uobject(const struct uverbs_obj_type *t return uobj; } - /* - * The kref for uobj is moved into filp->private data and put in - * uverbs_close_fd(). Once anon_inode_getfile() succeeds - * uverbs_close_fd() must be guaranteed to be called from the provided - * fops release callback. We piggyback our kref of uobj on the stack - * with the lifetime of the struct file. - */ - filp = anon_inode_getfile(fd_type->name, - fd_type->fops, - uobj, - fd_type->flags); - if (IS_ERR(filp)) { - put_unused_fd(new_fd); - uverbs_uobject_put(uobj); - return (void *)filp; - } - uobj->id = new_fd; - uobj->object = filp; uobj->ufile = ufile; - /* Matching put will be done in uverbs_close_fd() */ - kref_get(&ufile->ref); return uobj; } @@ -407,12 +384,10 @@ static int __must_check remove_commit_idr_uobject(struct ib_uobject *uobj, static void alloc_abort_fd_uobject(struct ib_uobject *uobj) { - struct file *filp = uobj->object; - int id = uobj->id; + put_unused_fd(uobj->id); - /* Unsuccessful NEW */ - fput(filp); - put_unused_fd(id); + /* Pairs with the kref from alloc_begin_idr_uobject */ + uverbs_uobject_put(uobj); } static int __must_check remove_commit_fd_uobject(struct ib_uobject *uobj, @@ -500,7 +475,7 @@ int rdma_explicit_destroy(struct ib_uobject *uobject) return ret; } -static void alloc_commit_idr_uobject(struct ib_uobject *uobj) +static int alloc_commit_idr_uobject(struct ib_uobject *uobj) { struct ib_uverbs_file *ufile = uobj->ufile; @@ -514,11 +489,34 @@ static void alloc_commit_idr_uobject(struct ib_uobject *uobj) */ WARN_ON(idr_replace(&ufile->idr, uobj, uobj->id)); spin_unlock(&ufile->idr_lock); + + return 0; } -static void alloc_commit_fd_uobject(struct ib_uobject *uobj) +static int alloc_commit_fd_uobject(struct ib_uobject *uobj) { + const struct uverbs_obj_fd_type *fd_type = + container_of(uobj->type, struct uverbs_obj_fd_type, type); int fd = uobj->id; + struct file *filp; + + /* + * The kref for uobj is moved into filp->private data and put in + * uverbs_close_fd(). Once alloc_commit() succeeds uverbs_close_fd() + * must be guaranteed to be called from the provided fops release + * callback. + */ + filp = anon_inode_getfile(fd_type->name, + fd_type->fops, + uobj, + fd_type->flags); + if (IS_ERR(filp)) + return PTR_ERR(filp); + + uobj->object = filp; + + /* Matching put will be done in uverbs_close_fd() */ + kref_get(&uobj->ufile->ref); /* This shouldn't be used anymore. Use the file object instead */ uobj->id = 0; @@ -527,7 +525,9 @@ static void alloc_commit_fd_uobject(struct ib_uobject *uobj) * NOTE: Once we install the file we loose ownership of our kref on * uobj. It will be put by uverbs_close_fd() */ - fd_install(fd, uobj->object); + fd_install(fd, filp); + + return 0; } /* @@ -538,11 +538,10 @@ static void alloc_commit_fd_uobject(struct ib_uobject *uobj) int __must_check rdma_alloc_commit_uobject(struct ib_uobject *uobj) { struct ib_uverbs_file *ufile = uobj->ufile; + int ret; /* Cleanup is running. Calling this should have been impossible */ if (!down_read_trylock(&ufile->hw_destroy_rwsem)) { - int ret; - WARN(true, "ib_uverbs: Cleanup is running while allocating an uobject\n"); ret = uobj->type->type_class->remove_commit(uobj, RDMA_REMOVE_DURING_CLEANUP); @@ -552,9 +551,18 @@ int __must_check rdma_alloc_commit_uobject(struct ib_uobject *uobj) return ret; } - /* matches atomic_set(-1) in alloc_uobj */ assert_uverbs_usecnt(uobj, true); - atomic_set(&uobj->usecnt, 0); + + /* alloc_commit consumes the uobj kref */ + ret = uobj->type->type_class->alloc_commit(uobj); + if (ret) { + if (uobj->type->type_class->remove_commit( + uobj, RDMA_REMOVE_DURING_CLEANUP)) + pr_warn("ib_uverbs: cleanup of idr object %d failed\n", + uobj->id); + up_read(&ufile->hw_destroy_rwsem); + return ret; + } /* kref is held so long as the uobj is on the uobj list. */ uverbs_uobject_get(uobj); @@ -562,8 +570,9 @@ int __must_check rdma_alloc_commit_uobject(struct ib_uobject *uobj) list_add(&uobj->list, &ufile->uobjects); spin_unlock_irq(&ufile->uobjects_lock); - /* alloc_commit consumes the uobj kref */ - uobj->type->type_class->alloc_commit(uobj); + /* matches atomic_set(-1) in alloc_uobj */ + atomic_set(&uobj->usecnt, 0); + up_read(&ufile->hw_destroy_rwsem); return 0; diff --git a/include/rdma/uverbs_types.h b/include/rdma/uverbs_types.h index 9b82e36128aa83..cfc50fcdbff63e 100644 --- a/include/rdma/uverbs_types.h +++ b/include/rdma/uverbs_types.h @@ -73,7 +73,7 @@ struct uverbs_obj_type_class { */ struct ib_uobject *(*alloc_begin)(const struct uverbs_obj_type *type, struct ib_uverbs_file *ufile); - void (*alloc_commit)(struct ib_uobject *uobj); + int (*alloc_commit)(struct ib_uobject *uobj); void (*alloc_abort)(struct ib_uobject *uobj); struct ib_uobject *(*lookup_get)(const struct uverbs_obj_type *type,