From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755382AbdKJFzV convert rfc822-to-8bit (ORCPT ); Fri, 10 Nov 2017 00:55:21 -0500 Received: from shards.monkeyblade.net ([184.105.139.130]:51078 "EHLO shards.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751083AbdKJFzU (ORCPT ); Fri, 10 Nov 2017 00:55:20 -0500 Date: Fri, 10 Nov 2017 14:55:16 +0900 (KST) Message-Id: <20171110.145516.1895467386398674445.davem@davemloft.net> To: Haakon.Bugge@oracle.com Cc: santosh.shilimkar@oracle.com, netdev@vger.kernel.org, linux-rdma@vger.kernel.org, rds-devel@oss.oracle.com, linux-kernel@vger.kernel.org, knut.omang@oracle.com, wei.lin.guay@oracle.com Subject: Re: [PATCH net] rds: ib: Fix NULL pointer dereference in debug code From: David Miller In-Reply-To: <20171107153334.24401-1-Haakon.Bugge@oracle.com> References: <20171107153334.24401-1-Haakon.Bugge@oracle.com> X-Mailer: Mew version 6.7 on Emacs 25.3 / Mule 6.0 (HANACHIRUSATO) Mime-Version: 1.0 Content-Type: Text/Plain; charset=iso-8859-1 Content-Transfer-Encoding: 8BIT X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.12 (shards.monkeyblade.net [149.20.54.216]); Thu, 09 Nov 2017 21:55:19 -0800 (PST) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org From: Håkon Bugge Date: Tue, 7 Nov 2017 16:33:34 +0100 > rds_ib_recv_refill() is a function that refills an IB receive > queue. It can be called from both the CQE handler (tasklet) and a > worker thread. > > Just after the call to ib_post_recv(), a debug message is printed with > rdsdebug(): > > ret = ib_post_recv(ic->i_cm_id->qp, &recv->r_wr, &failed_wr); > rdsdebug("recv %p ibinc %p page %p addr %lu ret %d\n", recv, > recv->r_ibinc, sg_page(&recv->r_frag->f_sg), > (long) ib_sg_dma_address( > ic->i_cm_id->device, > &recv->r_frag->f_sg), > ret); > > Now consider an invocation of rds_ib_recv_refill() from the worker > thread, which is preemptible. Further, assume that the worker thread > is preempted between the ib_post_recv() and rdsdebug() statements. > > Then, if the preemption is due to a receive CQE event, the > rds_ib_recv_cqe_handler() will be invoked. This function processes > receive completions, including freeing up data structures, such as the > recv->r_frag. > > In this scenario, rds_ib_recv_cqe_handler() will process the receive > WR posted above. That implies, that the recv->r_frag has been freed > before the above rdsdebug() statement has been executed. When it is > later executed, we will have a NULL pointer dereference: ... > This bug was provoked by compiling rds out-of-tree with > EXTRA_CFLAGS="-DRDS_DEBUG -DDEBUG" and inserting an artificial delay > between the rdsdebug() and ib_ib_port_recv() statements: > > /* XXX when can this fail? */ > ret = ib_post_recv(ic->i_cm_id->qp, &recv->r_wr, &failed_wr); > + if (can_wait) > + usleep_range(1000, 5000); > rdsdebug("recv %p ibinc %p page %p addr %lu ret %d\n", recv, > recv->r_ibinc, sg_page(&recv->r_frag->f_sg), > (long) ib_sg_dma_address( > > The fix is simply to move the rdsdebug() statement up before the > ib_post_recv() and remove the printing of ret, which is taken care of > anyway by the non-debug code. > > Signed-off-by: Håkon Bugge > Reviewed-by: Knut Omang > Reviewed-by: Wei Lin Guay Applied, thank you.