From patchwork Wed Nov 9 09:57:32 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Borkmann X-Patchwork-Id: 9418691 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 CC447601C0 for ; Wed, 9 Nov 2016 10:20:38 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id BC6F828A6B for ; Wed, 9 Nov 2016 10:20:38 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id B0EE42923A; Wed, 9 Nov 2016 10:20:38 +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, T_TVD_MIME_EPI 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 3ADBE28A6B for ; Wed, 9 Nov 2016 10:20:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752195AbcKIKUS (ORCPT ); Wed, 9 Nov 2016 05:20:18 -0500 Received: from www62.your-server.de ([213.133.104.62]:51588 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751934AbcKIKUQ (ORCPT ); Wed, 9 Nov 2016 05:20:16 -0500 Received: from [188.62.51.164] (helo=localhost.localdomain) by www62.your-server.de with esmtpsa (TLSv1.2:DHE-RSA-AES256-SHA:256) (Exim 4.85_2) (envelope-from ) id 1c4PdM-0007a2-Tf; Wed, 09 Nov 2016 10:57:33 +0100 Message-ID: <5822F30C.1050900@iogearbox.net> Date: Wed, 09 Nov 2016 10:57:32 +0100 From: Daniel Borkmann User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Zhiyi Sun CC: bblanco@plumgrid.com, Tariq Toukan , Yishai Hadas , netdev@vger.kernel.org, linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org, alexei.starovoitov@gmail.com Subject: Re: [PATCH] net/mlx4_en: Fix bpf_prog_add ref_cnt in mlx4 References: <20161109073544.jbufjqn7y7oa6ptg@ubuntu> <5822E6DB.40204@iogearbox.net> <20161109094546.jtmzc4xwtaavzcnt@ubuntu> In-Reply-To: <20161109094546.jtmzc4xwtaavzcnt@ubuntu> X-Authenticated-Sender: daniel@iogearbox.net X-Virus-Scanned: Clear (ClamAV 0.99.2/22503/Wed Nov 9 08:11:10 2016) 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 11/09/2016 10:45 AM, Zhiyi Sun wrote: > On Wed, Nov 09, 2016 at 10:05:31AM +0100, Daniel Borkmann wrote: >> On 11/09/2016 08:35 AM, Zhiyi Sun wrote: >>> There are rx_ring_num queues. Each queue will load xdp prog. So >>> bpf_prog_add() should add rx_ring_num to ref_cnt. >>> >>> Signed-off-by: Zhiyi Sun >> >> Your analysis looks incorrect to me. Please elaborate in more detail why >> you think current code is buggy ... > > Yes, you are correct. My patch is incorrect. It is not a bug. > >> Call path is dev_change_xdp_fd(), which does bpf_prog_get_type() on the >> fd. This already takes a ref and only drops it in case of error. Thus >> in mlx4_xdp_set(), you only need priv->rx_ring_num - 1 refs for the rest >> of the rings, so that dropping refs from old_prog makes sure we release >> it again. Looks correct to me (maybe a comment would have helped there). > > I thought mlx4's code is incorrect because in mlx5's driver, function > mlx5e_xdp_set() calls a pair of bpf_prog_add/put, the number of add and > put to the refs are same. I didn't notice that one "add" has been called in its > calller. So, it seems that mlx5's code is incorrect, right? Yep, I think the two attached patches are needed. The other thing I noticed in mlx5e_create_rq() is that it calls bpf_prog_add(rq->xdp_prog, 1) without actually checking for errors. From f0789544432bbb89c53c3b8ac6575d48fed97786 Mon Sep 17 00:00:00 2001 Message-Id: In-Reply-To: References: From: Daniel Borkmann Date: Wed, 9 Nov 2016 10:51:26 +0100 Subject: [PATCH net-next 2/2] bpf, mlx5: fix prog refcount in mlx5e_xdp_set dev_change_xdp_fd() already holds a reference, so bpf_prog_add(prog, 1) is not correct as it takes one reference too much and will thus leak the prog eventually. Also, bpf_prog_add() can fail and is not checked for errors here. Fixes: 86994156c736 ("net/mlx5e: XDP fast RX drop bpf programs support") Signed-off-by: Daniel Borkmann --- drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c index ba0c774..63309dd 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c @@ -3121,8 +3121,6 @@ static int mlx5e_xdp_set(struct net_device *netdev, struct bpf_prog *prog) /* exchange programs */ old_prog = xchg(&priv->xdp_prog, prog); - if (prog) - bpf_prog_add(prog, 1); if (old_prog) bpf_prog_put(old_prog); -- 1.9.3