From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754656AbdJINQr (ORCPT ); Mon, 9 Oct 2017 09:16:47 -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 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 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. Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit 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-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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) -- Doug Ledford GPG KeyID: B826A3330E572FDD Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD