From patchwork Mon Oct 9 13:16:35 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Doug Ledford X-Patchwork-Id: 9993093 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 D0FFE602D7 for ; Mon, 9 Oct 2017 13:17:04 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id C07F2287A8 for ; Mon, 9 Oct 2017 13:17:04 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id B5410287CD; Mon, 9 Oct 2017 13:17:04 +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=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=unavailable 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 32957287A8 for ; Mon, 9 Oct 2017 13:17:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751184AbdJINQp (ORCPT ); Mon, 9 Oct 2017 09:16:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49934 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754259AbdJINQh (ORCPT ); Mon, 9 Oct 2017 09:16:37 -0400 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0B6684E028; Mon, 9 Oct 2017 13:16:37 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 0B6684E028 Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=dledford@redhat.com Received: from linux-ws.xsintricity.com (ovpn-123-108.rdu2.redhat.com [10.10.123.108]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 2F44F67C6D; Mon, 9 Oct 2017 13:16:36 +0000 (UTC) Message-ID: <1507554995.46071.25.camel@redhat.com> Subject: Re: [PATCH] IB/rxe: check for allocation failure on elem From: Doug Ledford To: Leon Romanovsky , Colin King Cc: Moni Shoua , Sean Hefty , Hal Rosenstock , linux-rdma@vger.kernel.org, kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org Date: Mon, 09 Oct 2017 09:16:35 -0400 In-Reply-To: <20170912144829.GR3405@mtr-leonro.local> References: <20170908143745.2445-1-colin.king@canonical.com> <20170909125607.GA22465@mtr-leonro.local> <20170912144829.GR3405@mtr-leonro.local> Organization: Red Hat, Inc. Mime-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Mon, 09 Oct 2017 13:16:37 +0000 (UTC) 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 On Tue, 2017-09-12 at 17:48 +0300, Leon Romanovsky wrote: > On Sat, Sep 09, 2017 at 03:56:07PM +0300, Leon Romanovsky wrote: > > On Fri, Sep 08, 2017 at 03:37:45PM +0100, Colin King wrote: > > > From: Colin Ian King > > > > > > The allocation for elem may fail (especially because we're using > > > GFP_ATOMIC) so best to check for a null return. This fixes a > > > potential > > > null pointer dereference when assigning elem->pool. > > > > > > Detected by CoverityScan CID#1357507 ("Dereference null return > > > value") > > > > > > Fixes: 8700e3e7c485 ("Soft RoCE driver") > > > Signed-off-by: Colin Ian King > > > --- > > > drivers/infiniband/sw/rxe/rxe_pool.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c > > > b/drivers/infiniband/sw/rxe/rxe_pool.c > > > index c1b5f38f31a5..3b4916680018 100644 > > > --- a/drivers/infiniband/sw/rxe/rxe_pool.c > > > +++ b/drivers/infiniband/sw/rxe/rxe_pool.c > > > @@ -404,6 +404,8 @@ void *rxe_alloc(struct rxe_pool *pool) > > > elem = kmem_cache_zalloc(pool_cache(pool), > > > (pool->flags & RXE_POOL_ATOMIC) > > > ? > > > GFP_ATOMIC : GFP_KERNEL); > > > + if (!elem) > > > + return NULL; > > > > > > > It is not enough to simply return NULL, you also should release > > "pool" too. > > Colin, > do you plan to fix the comment and resend it? Since Colin is non-responsive in this thread, I went ahead and took his patch, but then applied a fixup of my own: commit a79c0f939da23740c12f43019720055aade89367 (HEAD -> k.o/for-next) Author: Doug Ledford Date: Mon Oct 9 09:11:32 2017 -0400 IB/rxe: put the pool on allocation failure If the allocation of elem fails, it is not sufficient to simply check for NULL and return. We need to also put our reference on the pool or else we will leave the pool with a permanent ref count and we will never be able to free it. Fixes: 4831ca9e4a8e (IB/rxe: check for allocation failure on elem) Suggested-by: Leon Romanovsky Signed-off-by: Doug Ledford diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c index 3b4916680018..b4a8acc7bb7d 100644 --- a/drivers/infiniband/sw/rxe/rxe_pool.c +++ b/drivers/infiniband/sw/rxe/rxe_pool.c @@ -394,23 +394,25 @@ void *rxe_alloc(struct rxe_pool *pool) kref_get(&pool->rxe->ref_cnt); - if (atomic_inc_return(&pool->num_elem) > pool->max_elem) { - atomic_dec(&pool->num_elem); - rxe_dev_put(pool->rxe); - rxe_pool_put(pool); - return NULL; - } + if (atomic_inc_return(&pool->num_elem) > pool->max_elem) + goto out_put_pool; elem = kmem_cache_zalloc(pool_cache(pool), (pool->flags & RXE_POOL_ATOMIC) ? GFP_ATOMIC : GFP_KERNEL); if (!elem) - return NULL; + goto out_put_pool; elem->pool = pool; kref_init(&elem->ref_cnt); return elem; + +out_put_pool: + atomic_dec(&pool->num_elem); + rxe_dev_put(pool->rxe); + rxe_pool_put(pool); + return NULL; } void rxe_elem_release(struct kref *kref)