From owner-svn-src-all@freebsd.org Sun Nov 24 14:37:15 2019 Return-Path: Delivered-To: svn-src-all@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 D91271B09E7 for ; Sun, 24 Nov 2019 14:37:15 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-qk1-x742.google.com (mail-qk1-x742.google.com [IPv6:2607:f8b0:4864:20::742]) (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 47LXn62rMWz44lY for ; Sun, 24 Nov 2019 14:37:14 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-qk1-x742.google.com with SMTP id i3so10462493qkk.9 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=EfZohD0nWsN5HHVA7+sbqbMzVGKU+31OEzyubCUMk8Xn4bdwqChRALlj+sRqOBJ408 ED4tLdB734T7sM1RN8RuwsvKtZn0kvh5FBdUV/gRAzzEVR7DPqmVW2d4/Nf7x6nIwn9x JEt0b1zOUxJZmpAFdcagnX0H5NR27PL3k9EJp0FsZ88Ze+Y2ZgfXHIBNUYSJBYtUqONS /XznBrQ4lVWQP2JoIZHiJw2cNVYA2LfY5Z8EXltudgMwW1nBbcvK0pVRxJCpX6WN0J77 XjU7tykmcSYeB4ftnjMPrgKBhYX/nFKh5kFaluHiHUPQHcQC8916vCa8hiBPRt9puiYj 9Z8A== X-Gm-Message-State: APjAAAXkQP/NmePUJRapnIjp5mW3bX88iF7NbHCKV/gJBNm2yaaF1571 yToNchRYVWOl+GvcVUYIYHWCBX0ltaEKrE0D83Qc3Q== 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: 47LXn62rMWz44lY 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::742) smtp.mailfrom=wlosh@bsdimp.com X-Spamd-Result: default: False [-2.25 / 15.00]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-1.00)[-0.998,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)[-0.999,0]; MIME_GOOD(-0.10)[multipart/alternative,text/plain]; PREVIOUSLY_DELIVERED(0.00)[svn-src-all@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)[2.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.26)[ip: (3.01), 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-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" 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, >