From patchwork Tue Jan 2 21:14:28 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jason Gunthorpe X-Patchwork-Id: 10141373 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 D9B036035E for ; Tue, 2 Jan 2018 21:14:35 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id C943B28E65 for ; Tue, 2 Jan 2018 21:14:35 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id BC9FF28E67; Tue, 2 Jan 2018 21:14:35 +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.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, 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 EFAA328E65 for ; Tue, 2 Jan 2018 21:14:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750934AbeABVOe (ORCPT ); Tue, 2 Jan 2018 16:14:34 -0500 Received: from mail-wr0-f193.google.com ([209.85.128.193]:45465 "EHLO mail-wr0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750800AbeABVOd (ORCPT ); Tue, 2 Jan 2018 16:14:33 -0500 Received: by mail-wr0-f193.google.com with SMTP id o15so17552156wrf.12 for ; Tue, 02 Jan 2018 13:14:32 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ziepe.ca; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=YAe7N6kmWSIR5cbas23TugM7F1oCrnqs7K6pdsjlQ18=; b=VGls/4vhFGihrKx4PFnx09A/jyyXsg3ZDi3r/sBQpcfRLAPWF+g4ktsX782sjYEXQW 6Ee7UsJAEHcgjX4EDhngZuELU8xh5QQeMXvynE3Swu6eialybkISJRvyw+tdgJRc8/Yb GzaY49oxiRbQLyG+Dci5J+Jt4slN8S8XvuEsoAh+uSiF1FN7PaoLex8ZSxUSo/TflasU bNpKk7L59ZdSdXwaXGhndersjR23LfPN2/KrEK3ppJssrmc/ccqDf7BzRymUVEzofdVo FDlIzXubMWVpKm/XQ2TMr72mOv3VfeRoWK5VyZOQNzzFRaMR/hkEuZlV1Nh10JtTZYfy 9s1g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=YAe7N6kmWSIR5cbas23TugM7F1oCrnqs7K6pdsjlQ18=; b=ZB2Q+w6Yn9wZVpg8uz3Pw5LYAeDSbZoHFdFweMuSBUP8EFN6zSJ5t3IEuDiXk+lJCj w+EvYOYz1vUHPWneSzcqjQ2uj2eheptJ/ck02QPZ5ML0F9Ts4Ckbdibv3K0FEUlGDkll RMh3bwDWccWyIqBVM4g7wy+taoUwDvfyqleeaip+SBMsEluIIyyzcPkcn+cxIM9kHezm SIjWKHipSxCmFcLPNcQzUpAWlA1UTQ+E1XRvOFyL8K4Qxky/JsgwJupsAGqYCFo8Pzo+ XLw+DB3idHi4yaaj8jx7xI/bCbjID31yLoDijm13bo8vsB7stCWk2qbCWU5nNj9qHigB uDSw== X-Gm-Message-State: AKGB3mJgDaWnIibYztXtbv16a6mOyEstu5BKgL4BL/gV8jBhckbI9wag B4GzOoVICXVLqETYqjo+NdfGAnZNwN4= X-Google-Smtp-Source: ACJfBotNLnIgcN7eUXE0FH5isitr/QujTehU9DfzP9ecmbFUpbt0sAFFH3v32wHORoo7A2fV1Kyxpw== X-Received: by 10.223.139.135 with SMTP id o7mr32333700wra.263.1514927671829; Tue, 02 Jan 2018 13:14:31 -0800 (PST) Received: from ziepe.ca (S010614cc2056d97f.ed.shawcable.net. [70.74.179.152]) by smtp.gmail.com with ESMTPSA id t61sm82931917wrc.21.2018.01.02.13.14.30 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 02 Jan 2018 13:14:31 -0800 (PST) Received: from jgg by mlx.ziepe.ca with local (Exim 4.86_2) (envelope-from ) id 1eWTtE-0002SL-8Z; Tue, 02 Jan 2018 14:14:28 -0700 Date: Tue, 2 Jan 2018 14:14:28 -0700 From: Jason Gunthorpe To: Leon Romanovsky Cc: Doug Ledford , RDMA mailing list , Mark Bloch , Leon Romanovsky Subject: Re: [PATCH rdma-next v1 6/7] RDMA/netlink: Protect device and port queries from device removal Message-ID: <20180102211428.GA7831@ziepe.ca> References: <20180101110717.29686-1-leon@kernel.org> <20180101110717.29686-7-leon@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20180101110717.29686-7-leon@kernel.org> User-Agent: Mutt/1.5.24 (2015-08-30) 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 Mon, Jan 01, 2018 at 01:07:16PM +0200, Leon Romanovsky wrote: > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c > index 34c6cb2a0977..a0ea3dca479d 100644 > +++ b/drivers/infiniband/core/device.c > @@ -134,7 +134,10 @@ static int ib_device_check_mandatory(struct ib_device *device) > return 0; > } > > -struct ib_device *__ib_device_get_by_index(u32 index) > +/* > + * Caller to this function should hold lists_rwsem > + */ This comment isn't 100% right, the device mutex is also OK to hold, I dropped it. > @@ -141,36 +141,41 @@ static int nldev_get_doit(struct sk_buff *skb, struct nlmsghdr *nlh, > struct ib_device *device; > struct sk_buff *msg; > u32 index; > - int err; > + int ret = -ENOMEM; > > - err = nlmsg_parse(nlh, 0, tb, RDMA_NLDEV_ATTR_MAX - 1, > + ret = nlmsg_parse(nlh, 0, tb, RDMA_NLDEV_ATTR_MAX - 1, > nldev_policy, extack); Initializing ret, then overwriting it with nlmsg_parse is silly, and leads to this bug: > msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); > if (!msg) > - return -ENOMEM; > + goto err; Wrong return code after the change. I fixed it also dropped replacing err with ret since this is probably going to be backported. > port = nla_get_u32(tb[RDMA_NLDEV_ATTR_PORT_INDEX]); > if (!rdma_is_port_valid(device, port)) > - return -EINVAL; > + goto err; Same mistake again I also squashed this with the prior patch to make backporting simpler RDMA/core: Provide locked variant of device name to index function And queued it to for-rc See below, please check my work. Author: Leon Romanovsky Date: Mon Jan 1 13:07:15 2018 +0200 RDMA/netlink: Fix locking around __ib_get_device_by_index Holding locks is mandatory when calling __ib_device_get_by_index, otherwise there are races during the list iteration with device removal. Since the locks are static to device.c, __ib_device_get_by_index can never be called correctly by any user out side the file. Make the function static and provide a safe function that gets the correct locks and returns a kref'd pointer. Fix all callers. Fixes: e5c9469efcb1 ("RDMA/netlink: Add nldev device doit implementation") Fixes: c3f66f7b0052 ("RDMA/netlink: Implement nldev port doit callback") Fixes: 7d02f605f0dc ("RDMA/netlink: Add nldev port dumpit implementation") Reviewed-by: Mark Bloch Signed-off-by: Leon Romanovsky Signed-off-by: Jason Gunthorpe --- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/infiniband/core/core_priv.h b/drivers/infiniband/core/core_priv.h index 0deff4c4911b2b..8dbfc3ab48a608 100644 --- a/drivers/infiniband/core/core_priv.h +++ b/drivers/infiniband/core/core_priv.h @@ -294,7 +294,7 @@ static inline int ib_mad_enforce_security(struct ib_mad_agent_private *map, } #endif -struct ib_device *__ib_device_get_by_index(u32 ifindex); +struct ib_device *ib_device_get_by_index(u32 ifindex); /* RDMA device netlink */ void nldev_init(void); void nldev_exit(void); diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c index a8757b5552de1d..1b413a2df9489b 100644 --- a/drivers/infiniband/core/device.c +++ b/drivers/infiniband/core/device.c @@ -134,7 +134,7 @@ static int ib_device_check_mandatory(struct ib_device *device) return 0; } -struct ib_device *__ib_device_get_by_index(u32 index) +static struct ib_device *__ib_device_get_by_index(u32 index) { struct ib_device *device; @@ -145,6 +145,22 @@ struct ib_device *__ib_device_get_by_index(u32 index) return NULL; } +/* + * Caller is responsible to return refrerence count by calling put_device() + */ +struct ib_device *ib_device_get_by_index(u32 index) +{ + struct ib_device *device; + + down_read(&lists_rwsem); + device = __ib_device_get_by_index(index); + if (device) + get_device(&device->dev); + + up_read(&lists_rwsem); + return device; +} + static struct ib_device *__ib_device_get_by_name(const char *name) { struct ib_device *device; diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c index 2b631307349ddc..5d790c507c7e17 100644 --- a/drivers/infiniband/core/nldev.c +++ b/drivers/infiniband/core/nldev.c @@ -150,27 +150,34 @@ static int nldev_get_doit(struct sk_buff *skb, struct nlmsghdr *nlh, index = nla_get_u32(tb[RDMA_NLDEV_ATTR_DEV_INDEX]); - device = __ib_device_get_by_index(index); + device = ib_device_get_by_index(index); if (!device) return -EINVAL; msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); - if (!msg) - return -ENOMEM; + if (!msg) { + err = -ENOMEM; + goto err; + } nlh = nlmsg_put(msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq, RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_GET), 0, 0); err = fill_dev_info(msg, device); - if (err) { - nlmsg_free(msg); - return err; - } + if (err) + goto err_free; nlmsg_end(msg, nlh); + put_device(&device->dev); return rdma_nl_unicast(msg, NETLINK_CB(skb).portid); + +err_free: + nlmsg_free(msg); +err: + put_device(&device->dev); + return err; } static int _nldev_get_dumpit(struct ib_device *device, @@ -228,31 +235,40 @@ static int nldev_port_get_doit(struct sk_buff *skb, struct nlmsghdr *nlh, return -EINVAL; index = nla_get_u32(tb[RDMA_NLDEV_ATTR_DEV_INDEX]); - device = __ib_device_get_by_index(index); + device = ib_device_get_by_index(index); if (!device) return -EINVAL; port = nla_get_u32(tb[RDMA_NLDEV_ATTR_PORT_INDEX]); - if (!rdma_is_port_valid(device, port)) - return -EINVAL; + if (!rdma_is_port_valid(device, port)) { + err = -EINVAL; + goto err; + } msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); - if (!msg) - return -ENOMEM; + if (!msg) { + err = -ENOMEM; + goto err; + } nlh = nlmsg_put(msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq, RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_GET), 0, 0); err = fill_port_info(msg, device, port); - if (err) { - nlmsg_free(msg); - return err; - } + if (err) + goto err_free; nlmsg_end(msg, nlh); + put_device(&device->dev); return rdma_nl_unicast(msg, NETLINK_CB(skb).portid); + +err_free: + nlmsg_free(msg); +err: + put_device(&device->dev); + return err; } static int nldev_port_get_dumpit(struct sk_buff *skb, @@ -273,7 +289,7 @@ static int nldev_port_get_dumpit(struct sk_buff *skb, return -EINVAL; ifindex = nla_get_u32(tb[RDMA_NLDEV_ATTR_DEV_INDEX]); - device = __ib_device_get_by_index(ifindex); + device = ib_device_get_by_index(ifindex); if (!device) return -EINVAL; @@ -307,7 +323,9 @@ static int nldev_port_get_dumpit(struct sk_buff *skb, nlmsg_end(skb, nlh); } -out: cb->args[0] = idx; +out: + put_device(&device->dev); + cb->args[0] = idx; return skb->len; }