From owner-svn-src-head@freebsd.org Sun Nov 24 14:37:15 2019 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 65B431B09E2 for ; Sun, 24 Nov 2019 14:37:15 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-qk1-x741.google.com (mail-qk1-x741.google.com [IPv6:2607:f8b0:4864:20::741]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 47LXn63Gwlz44lb for ; Sun, 24 Nov 2019 14:37:14 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-qk1-x741.google.com with SMTP id d13so10494299qko.3 for ; Sun, 24 Nov 2019 06:37:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=qEJARp+MD8dHucsPZ4rIDMzVDGQtn4wy0pQpMomYnKY=; b=X0QDfnu1i+SdG+7MwQ+IE7I/MCcKPx4au+yeZlZaHm0bXwrYY+mSQLRrIcEML25SDI ykV1tCn/emeEL8fAUdFRvf7QCLweaMnppsvV3e4x6H8GR9eydUFuwE2TKd0qUjlHvDoN XcQnNa8O/aK/oUAtPnoBDolrZoc79wxgmUYBhzehLhqszW9VZRddBWItz7LMLIHjVjXz zD4RDOO+eg3G2oYCuuWktaKwOOSwyCMCvzd2wow2fA0yhGh01hRCMaeQZk81JXYhZegE nF57mMlOdI24W2suXQ3LRtjqoGKeAS0RR2icLXQ4jLQ0HDzd32lgtYBxQnKIS3ywQIrp OeNQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=qEJARp+MD8dHucsPZ4rIDMzVDGQtn4wy0pQpMomYnKY=; b=Gj29p4M0VRAmdcn42ku7fl7Gu8aSp20TtxwuwbrhxG1E2P9q+t+LuZHRJDDTEWHAMl VeIprwdfu8QZbcI3YrWQ8M7c5BhAREw0m+AikmiTjzQSXh89xndmTShlTxPz2DbrbYs3 N23T6KMfwlDXLW3tSGLpeNxe6+1xOOtEZ/oLLchtaHtoYfw2BCB0RmPi9DUfGHqdeFcq Y1v5QIuQobM3sAtx9n8l1uQ6bFpNiUOCk4HmrW4YBb1INpDun/DCIvi1turZye6v1xRm R+HILltB19HhcakDfJ2BHnVT0bqFmw0fLUY4gqDm784caKIcgOUZpZMCPiqHov0KKmOp h7tw== X-Gm-Message-State: APjAAAVPAB6HitV5CbLmv017tWKLY0hwiF91GT0z7J1h+a02HJVvGZ9v 7ROgnRQtsFwqhRkMQGMcOOZ6vupQ/4fnn4pT2xte8A== X-Google-Smtp-Source: APXvYqxTM3aSEzJ6wWPbL+KOk5axpu5x+/WkBHToPa25/71HGjuwCEsjH+rQK3s6NqO8EUW4n/4+joKGVc7r23AMxqc= X-Received: by 2002:a37:4716:: with SMTP id u22mr22242527qka.495.1574606232786; Sun, 24 Nov 2019 06:37:12 -0800 (PST) MIME-Version: 1.0 References: <201911232343.xANNhqkQ097797@repo.freebsd.org> <20191124131010.GB2707@kib.kiev.ua> In-Reply-To: <20191124131010.GB2707@kib.kiev.ua> From: Warner Losh Date: Sun, 24 Nov 2019 07:37:01 -0700 Message-ID: Subject: Re: svn commit: r355037 - head/sys/dev/pci To: Konstantin Belousov Cc: Warner Losh , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org X-Rspamd-Queue-Id: 47LXn63Gwlz44lb X-Spamd-Bar: -- Authentication-Results: mx1.freebsd.org; dkim=pass header.d=bsdimp-com.20150623.gappssmtp.com header.s=20150623 header.b=X0QDfnu1; dmarc=none; spf=none (mx1.freebsd.org: domain of wlosh@bsdimp.com has no SPF policy when checking 2607:f8b0:4864:20::741) smtp.mailfrom=wlosh@bsdimp.com X-Spamd-Result: default: False [-2.37 / 15.00]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-1.00)[-0.999,0]; R_DKIM_ALLOW(-0.20)[bsdimp-com.20150623.gappssmtp.com:s=20150623]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; MIME_GOOD(-0.10)[multipart/alternative,text/plain]; PREVIOUSLY_DELIVERED(0.00)[svn-src-head@freebsd.org]; DMARC_NA(0.00)[bsdimp.com]; RCPT_COUNT_FIVE(0.00)[5]; TO_MATCH_ENVRCPT_SOME(0.00)[]; DKIM_TRACE(0.00)[bsdimp-com.20150623.gappssmtp.com:+]; RCVD_IN_DNSWL_NONE(0.00)[1.4.7.0.0.0.0.0.0.0.0.0.0.0.0.0.0.2.0.0.4.6.8.4.0.b.8.f.7.0.6.2.list.dnswl.org : 127.0.5.0]; R_SPF_NA(0.00)[]; FORGED_SENDER(0.30)[imp@bsdimp.com,wlosh@bsdimp.com]; FREEMAIL_TO(0.00)[gmail.com]; MIME_TRACE(0.00)[0:+,1:+,2:~]; IP_SCORE(-0.37)[ip: (2.45), ipnet: 2607:f8b0::/32(-2.28), asn: 15169(-1.96), country: US(-0.05)]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; FROM_NEQ_ENVFROM(0.00)[imp@bsdimp.com,wlosh@bsdimp.com]; RCVD_TLS_ALL(0.00)[]; RCVD_COUNT_TWO(0.00)[2] Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 24 Nov 2019 14:37:15 -0000 On Sun, Nov 24, 2019, 6:10 AM Konstantin Belousov wrote: > On Sat, Nov 23, 2019 at 11:43:52PM +0000, Warner Losh wrote: > > Author: imp > > Date: Sat Nov 23 23:43:52 2019 > > New Revision: 355037 > > URL: https://svnweb.freebsd.org/changeset/base/355037 > > > > Log: > > Push Giant down one layer > > > > The /dev/pci device doesn't need GIANT, per se. However, one routine > > that it calls, pci_find_dbsf implicitly does. It walks a list that can > > change when PCI scans a new bus. With hotplug, this means we could > > have a race with that scanning. To prevent that, take out Giant around > > scanning the list. > > > > However, given that we have places in the tree that drop giant, if > > held when we call into them, the whole use of Giant to protect newbus > > may be less effective that we desire, so add a comment about why we're > > talking it out, and we'll address the issue when we lock newbus with > > something other than Giant. > > > > Modified: > > head/sys/dev/pci/pci.c > > head/sys/dev/pci/pci_user.c > > > > Modified: head/sys/dev/pci/pci.c > > > ============================================================================== > > --- head/sys/dev/pci/pci.c Sat Nov 23 23:41:21 2019 (r355036) > > +++ head/sys/dev/pci/pci.c Sat Nov 23 23:43:52 2019 (r355037) > > @@ -445,18 +445,21 @@ pci_find_bsf(uint8_t bus, uint8_t slot, uint8_t > func) > > device_t > > pci_find_dbsf(uint32_t domain, uint8_t bus, uint8_t slot, uint8_t func) > > { > > - struct pci_devinfo *dinfo; > > + struct pci_devinfo *dinfo = NULL; > > > > + /* Giant because newbus is Giant locked revisit with newbus > locking */ > > + mtx_lock(&Giant); > > STAILQ_FOREACH(dinfo, &pci_devq, pci_links) { > > if ((dinfo->cfg.domain == domain) && > > (dinfo->cfg.bus == bus) && > > (dinfo->cfg.slot == slot) && > > (dinfo->cfg.func == func)) { > > - return (dinfo->cfg.dev); > > + break; > > } > > } > > + mtx_unlock(&Giant); > > > > - return (NULL); > > + return (dinfo != NULL ? dinfo->cfg.dev : NULL); > I do not think this change is correct. If the parallel hotplug, or > rather, hot-unplug event occurs, then dinfo potentially becomes invalid > right after the Giant unlock, which makes both this function and its > callers to access freed memory. Having caller to lock a newbus lock > around both the call and consumption of the returned data is required. There are many data lifetime issues. If anything the PCI user device calls drops Giant and then picks it back up again we are in the same boat... I totally agree this is a bad situation, but can only really be fixed by locking newbus with a different lock than Giant and likely using some kind of reference count for device_t that are handed out... In the mean time, I'll move giant back up into the ioctl routine and hope it isn't dropped by things it calls..m Warner > } > > > > /* Find a device_t by vendor/device ID */ > > > > Modified: head/sys/dev/pci/pci_user.c > > > ============================================================================== > > --- head/sys/dev/pci/pci_user.c Sat Nov 23 23:41:21 2019 > (r355036) > > +++ head/sys/dev/pci/pci_user.c Sat Nov 23 23:43:52 2019 > (r355037) > > @@ -119,7 +119,7 @@ static d_ioctl_t pci_ioctl; > > > > struct cdevsw pcicdev = { > > .d_version = D_VERSION, > > - .d_flags = D_NEEDGIANT, > > + .d_flags = 0, > > .d_open = pci_open, > > .d_close = pci_close, > > .d_ioctl = pci_ioctl, >