Received: by 2002:a05:6a10:206:0:0:0:0 with SMTP id 6csp175873pxj; Fri, 14 May 2021 00:35:15 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwJhdtKfQ7teDN/QHM+4f4QffPLX7QyEUVSbSVST2fydCnSGyNsUmVT1uPIxrbyA4oO+N4K X-Received: by 2002:a17:906:3544:: with SMTP id s4mr48205809eja.73.1620977715059; Fri, 14 May 2021 00:35:15 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1620977715; cv=none; d=google.com; s=arc-20160816; b=hN1NbOcgjw6rt8FqZhbOKVmRLqZ2nhgeJgCCZf3fEzfA6XAK2aP3jS+RLSWuwZYF/2 KZ034Yy57VsWu/PSfztgrwSKUD8houeHNCRA9rwiCqRYBmpq07kMJbYHoJH9S9NrIsyb xsttSGia4Jjh2H3wUkepuDmMttoqVx2WcOdSRkyP9tJvLe6Y0twlDXkK576+LPx9wt3V zkU0WWQDlt44LygwpUsqKmkSkJpW/+/Rs1+FMT6z/7jfd/egvuRTNN7Ngy1aO0dChjbE sDe7c9EWt8shOfieozsD6JhHcPsF5KAYDjIVxb5q3OZtCduhsqbMwMmIRIYP8BUK2vuW aZbw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:subject:message-id:date:from:in-reply-to :references:mime-version:dkim-signature; bh=KrY5KBxRbqr/N+zWPtbQO+0iHVf/6xLbd2WWFf5zkXE=; b=FIzb3OwegPV9PUnSOPcMVCGJUl+RfmFC3JyPKCAdktAoYAJMxyr8KTVlnAy5l04dek EG9sgcHL9MDKQ2XpivLvjyzupL1LwbJMZ6Ab+Hp1MZQ2J0/5NMBk47NnGKB9jNpZ/a21 ZWXR1MZJWufvpW7c3ehuXcAIYA6NIw4e2+027iaGQDY6mC0p7M3oC3jy55ZXdwuXfAYr n3VGqVumBPAmSO6TVk5/LUI6foAcNvjtwfS61lziH2Al5cgiAkzZwYYTURM14cZ9UQxG eyLSYIMzhUd+isa0LIC+ufloMKmptns0EmpBKTWhS/JOkM/Vhq19d/ld4azB7bKKKhwF 4mSA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20161025 header.b=bxOU9My3; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id bz8si5394160ejc.547.2021.05.14.00.34.52; Fri, 14 May 2021 00:35:15 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20161025 header.b=bxOU9My3; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232959AbhEMUgN (ORCPT + 99 others); Thu, 13 May 2021 16:36:13 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40514 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233056AbhEMUgL (ORCPT ); Thu, 13 May 2021 16:36:11 -0400 Received: from mail-lf1-x12b.google.com (mail-lf1-x12b.google.com [IPv6:2a00:1450:4864:20::12b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DF7A7C061574 for ; Thu, 13 May 2021 13:35:00 -0700 (PDT) Received: by mail-lf1-x12b.google.com with SMTP id g4so10817761lfv.6 for ; Thu, 13 May 2021 13:35:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=KrY5KBxRbqr/N+zWPtbQO+0iHVf/6xLbd2WWFf5zkXE=; b=bxOU9My3lmGVJSFRnbTRSHl9P3N7k0MMbptVK/37OKlEAzjH0g1lnd0ECT+Jisx0wW aHfZsmLawhD0Uf1o43iTvH/4ZzEZk5OQ1sq401azt/4bKq7vO/ZCamvHFKbwcjmMkhJ8 4JePMv2sGKK7cXTnjXdXBRB+uFlGScv6TBv5V4SSaEjvFSk/4v2S3cLjyZ5ESI5V8p4r +HAslxEfxHYBwwXkrxeCbvFyjRViqG0tPFkMywbAPO9bpTZ1N8VO6R1VW+BbuPZIaJeZ YxyLnbPjC5+w1amKKeHy1nmgP4goAOhRM7hFsJoUndzEkdmjBJEgNroy39r8uQwWhwLR 0xCw== 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=KrY5KBxRbqr/N+zWPtbQO+0iHVf/6xLbd2WWFf5zkXE=; b=AZUgEUbr13q93uRmKhGqTIvPSWE4kmIl6v8x3/pLDJJ2YC/+RrfJGgYpjRNQTdPKUH mumLhBkdMNYi1/xbwC+m2Q5Cvro85dv9LX+GT/qhuDs54SROZPGHYU7aeoRrTATu6bru Ui+Oa2GB8nd/TJQHSt6VvC3q2Woxfpc6IbF02krks/CoviLXY1owcAcxWHbz+f7M7fDB 1aKVLgpr2VP64zxq/QkENpn8E9aZ28fUj6+t8gLR4Z4ji54vsVGFuas5tCumojC4ngBD QSoEOEW+LbAU/2ypbkfyH/VY/pKHwpDSOKC7WHjx6tRUylqPkbzG6XcedVpokUNbNSeK /y0A== X-Gm-Message-State: AOAM530oKUlo/c4J1RE2m2Re0/09fYXSr1/SyQFOxUm6XOCz+w4W61ij vTFP+FqZV+CdUMiGu0su7jbGB8rZOivw8XrDSLF3Rw== X-Received: by 2002:ac2:414e:: with SMTP id c14mr30725547lfi.155.1620938099093; Thu, 13 May 2021 13:34:59 -0700 (PDT) MIME-Version: 1.0 References: <20210513200550.GA2604592@bjorn-Precision-5520> In-Reply-To: <20210513200550.GA2604592@bjorn-Precision-5520> From: Rajat Jain Date: Thu, 13 May 2021 13:34:23 -0700 Message-ID: Subject: Re: [PATCH v3 2/2] PCI: Add sysfs "removable" attribute To: Bjorn Helgaas Cc: Greg Kroah-Hartman , "Rafael J. Wysocki" , Bjorn Helgaas , Alan Stern , Linux Kernel Mailing List , linux-pci , "open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:" , Oliver Neukum , David Laight , =?UTF-8?Q?Krzysztof_Wilczy=C5=84ski?= , Rajat Jain , Jesse Barnes , Dmitry Torokhov Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, May 13, 2021 at 1:05 PM Bjorn Helgaas wrote: > > On Thu, May 13, 2021 at 11:02:10AM -0700, Rajat Jain wrote: > > On Wed, May 12, 2021 at 2:35 PM Rajat Jain wrote: > > > > > > A PCI device is "external_facing" if it's a Root Port with the ACPI > > > "ExternalFacingPort" property or if it has the DT "external-facing" > > > property. We consider everything downstream from such a device to > > > be removable by user. > > > > > > We're mainly concerned with consumer platforms with user accessible > > > thunderbolt ports that are vulnerable to DMA attacks, and we expect those > > > ports to be identified as "ExternalFacingPort". Devices in traditional > > > hotplug slots can technically be removed, but the expectation is that > > > unless the port is marked with "ExternalFacingPort", such devices are less > > > accessible to user / may not be removed by end user, and thus not exposed > > > as "removable" to userspace. > > s/thunderbolt/Thunderbolt/ since I think it's a trademark > s/identified as/identified by firmware as/ Ack, will do. > > > > Set pci_dev_type.supports_removable so the device core exposes the > > > "removable" file in sysfs, and tell the device core about removable > > > devices. > > > > > > This can be used by userspace to implment any policies it wants to, > > > tailored specifically for user removable devices. Eg usage: > > > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2591812 > > > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2795038 > > > (code uses such an attribute to remove external PCI devicces or disable > > > features on them as needed by the policy desired) > > s/implment/implement/ > s/devicces/devices/ > > Or maybe something like: > > This can be used to implement userspace policies tailored for > user-removable devices. Ack, will do. > > Not sure exactly what "remove external PCI devices" means. You're > talking about the *code* doing something, so I don't think it means > physically unplugging the device from the system. Maybe preventing a > driver from binding to it or something similar? echo 1 > /sys/bus/pci/devices//remove > > I hesitate slightly to rely on URLs like googlesource.com in commit > logs because we don't know how long they will remain valid. But I > guess there's no real alternative here, since this code probably > hasn't been posted to any public mailing lists like the ones archived > at https://lore.kernel.org/lists.html, right? Yes, chromium reviews (userspace code that shall use the new attribute) happen over gerrit, and so the publicly available links would be googlesource.com. > > > > Signed-off-by: Rajat Jain > > > > +static void pci_set_removable(struct pci_dev *dev) > > > +{ > > > + struct pci_dev *parent = pci_upstream_bridge(dev); > > > + if (parent && > > > + (parent->external_facing || dev_is_removable(&parent->dev))) > > > + dev_set_removable(&dev->dev, DEVICE_REMOVABLE); > > > + else > > > + dev_set_removable(&dev->dev, DEVICE_FIXED); > > > +} > > > > Copying comments from Krzysztof from another thread: > > > > [Krzysztof] We were also wondering if we should only set DEVICE_REMOVABLE for > > devices known to be behind an external-facing port, and let everything > > else be set to "unknown" (or whatever the default would be). > > > > [Rajat]: I think I'm fine with this proposal if Bjorn & PCI community > > also sees this as a better idea. Essentially the question here is, > > would it be better for the non-removable PCI devices to be shown as > > "fixed" or "unknown"? > > I think I would rather see this as: > > struct pci_dev *parent = pci_upstream_bridge(dev); > > if (parent && > (parent->external_facing || dev_is_removable(&parent->dev))) > dev_set_removable(&dev->dev, DEVICE_REMOVABLE); > > In other words, assume only that everything below an "external-facing" > device is removable. > > In the absence of an "external-facing" property, we don't know > anything about the connection, and I'd rather use the default > (probably "unknown") instead of assuming "fixed." Ack, will do. One question: Under Greg's latest suggestion, the decision to show this attribute does not have to be bus wide / device_type wide i.e. subsystem can choose for this attribute to show up only under certain devices. So if it is more preferable, I can have this attribute show under ONLY PCI devices that attach below "external-facing" PCI devices (and any other PCI devices will not have this attribute show up at all). I guess this sounds better than having "unknown" show up on the rest of the devices that are not removable? > > I don't think we have anything that depends on "fixed," so I don't > think there's value in setting it. > > (Note the blank line between local variables and the "if"; maybe > that's what Greg hinted at?) Ack, will remove the blank line (didn't know that blank lines between variables and code is not preferred). Thanks, Rajat > > Bjorn