From owner-freebsd-current@freebsd.org Sat Jun 6 14:55:20 2020 Return-Path: Delivered-To: freebsd-current@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 1360F336505 for ; Sat, 6 Jun 2020 14:55:20 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-qk1-x733.google.com (mail-qk1-x733.google.com [IPv6:2607:f8b0:4864:20::733]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 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 49fMxz1Zdjz45bQ for ; Sat, 6 Jun 2020 14:55:18 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-qk1-x733.google.com with SMTP id s1so12821553qkf.9 for ; Sat, 06 Jun 2020 07:55:18 -0700 (PDT) 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=v0r6KCtmI+SEmvxRPUi6lI1somaTQwxFFLr8qK6Y4vs=; b=FBBT746FfyYoKY4d0EXdTUPfPFhwn3pp8MXAWOULVgzGydmVQielHuzXZLmGbtoLsT A4MOiTRFk1O8oCCq+gO2UKBEZJcg7ONHGsuI280gegApJ2kRCkqCoQ5Q/FLKQ/2ghGVr IUPUzk2RLvhbwBVLd0dsG2/ytkg0nlCWeuZ4Lmsl2W95bCesCtwnpm6XLgR1bfwrbE8+ 3k0JTdgWjba7Mc4oVCTlpbQ0VJhnY7daIzz/RI35AXf0lV4O/CMBpMiz3K3wdcNEB45Z M+2W/E5RsKEJep5TeRN3MCA+e+yDuaNQwT0xt37joyzrvtF/dRZOnqdcssS127QtHSjE 4skw== 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=v0r6KCtmI+SEmvxRPUi6lI1somaTQwxFFLr8qK6Y4vs=; b=PuP8bzGUxBQDJW7XxYenNT4UgSahzmrXuldtma5TeNycQDnDoZ7mlNZ7NBXTCBo0bR ztxX5ZsNJMerfJMA43gMTa6Ur493saMT9HqGcyEvs5tekpKTkXGNpKJSaSPQ95HwZE5x +U2ehYqO7rRIohlTNFhVNktifTSiwGGFxi4SaLs9Xos97V2qZoBs5/cGTIN0wjMhPJYF YAjnu52cBtV9UjreQV6Ajuf94IC23Gb2p1T6IA23o4Wim1s2KhHuGzrd+8rlsJTKErTb +XLz1OubpRbEy2Gq1B4nsboYgwRzRnNzGFFprcQ1/lPhXBBccVzUwCIZd/o+2F9EpYPo 52DA== X-Gm-Message-State: AOAM530Z1Ab/gO0U1DjnzxKOuek4JOT3iTLK2GlsBlNI1bCENkA5w7gZ G9su5L4nbDYNu9fz4TT5F6dFyBh04JpbOLqmRHUNyA== X-Google-Smtp-Source: ABdhPJyA8plYxm8ihgmW5xp20pfUCgevMPifZNbMYNPEBkIiaXk0FZOcOytyWUT/UZ5PYXnG/dormqyvHB6R0PFCPcA= X-Received: by 2002:a37:5c47:: with SMTP id q68mr14499880qkb.495.1591455318013; Sat, 06 Jun 2020 07:55:18 -0700 (PDT) MIME-Version: 1.0 References: <202006051612.055GCL11009491@repo.freebsd.org> In-Reply-To: From: Warner Losh Date: Sat, 6 Jun 2020 08:55:07 -0600 Message-ID: Subject: Re: HEADSUP: GEOM label may be broken [Was Re: svn commit: r361838 - in head/sys/geom: . label] To: Xin LI Cc: Conrad Meyer , Warner Losh , FreeBSD Current X-Rspamd-Queue-Id: 49fMxz1Zdjz45bQ X-Spamd-Bar: - Authentication-Results: mx1.freebsd.org; dkim=pass header.d=bsdimp-com.20150623.gappssmtp.com header.s=20150623 header.b=FBBT746F; dmarc=none; spf=none (mx1.freebsd.org: domain of wlosh@bsdimp.com has no SPF policy when checking 2607:f8b0:4864:20::733) smtp.mailfrom=wlosh@bsdimp.com X-Spamd-Result: default: False [-1.98 / 15.00]; RCVD_TLS_ALL(0.00)[]; ARC_NA(0.00)[]; R_DKIM_ALLOW(-0.20)[bsdimp-com.20150623.gappssmtp.com:s=20150623]; NEURAL_HAM_MEDIUM(-0.93)[-0.932]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_THREE(0.00)[4]; NEURAL_HAM_LONG(-1.00)[-1.003]; MIME_GOOD(-0.10)[multipart/alternative,text/plain]; PREVIOUSLY_DELIVERED(0.00)[freebsd-current@freebsd.org]; DMARC_NA(0.00)[bsdimp.com]; TO_MATCH_ENVRCPT_SOME(0.00)[]; TO_DN_ALL(0.00)[]; DKIM_TRACE(0.00)[bsdimp-com.20150623.gappssmtp.com:+]; NEURAL_HAM_SHORT(-0.04)[-0.040]; RCVD_IN_DNSWL_NONE(0.00)[2607:f8b0:4864:20::733:from]; R_SPF_NA(0.00)[no SPF record]; FORGED_SENDER(0.30)[imp@bsdimp.com,wlosh@bsdimp.com]; MIME_TRACE(0.00)[0:+,1:+,2:~]; RCVD_COUNT_TWO(0.00)[2]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; FROM_NEQ_ENVFROM(0.00)[imp@bsdimp.com,wlosh@bsdimp.com] Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.33 X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.33 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 06 Jun 2020 14:55:20 -0000 On Sat, Jun 6, 2020 at 2:17 AM Xin Li wrote: > I just spent quite some time to revive my laptop. TL;DR: if you are > using /dev/diskid or /dev/gptid labels and GELI, please wait until > things settled. > > === > > On 6/5/20 9:12 AM, Conrad Meyer wrote: > > Author: cem > > Date: Fri Jun 5 16:12:21 2020 > > New Revision: 361838 > > URL: https://svnweb.freebsd.org/changeset/base/361838 > > > > Log: > > geom_label: Use provider aliasing to alias upstream geoms > > > > For synthetic aliases (just pseudonyms inferred from metadata like GPT > or > > UFS labels, GPT UUIDs, etc), use the GEOM provider aliasing system to > create > > a symlink to the real device instead of creating an independent device. > > This makes it more clear which labels and devices correspond, and we > can > > safely have multiple labels to a single device accessed at once. > > > > The confusingly named geom_label on-disk construct continues to behave > > identically to how it did before. > > > > This requires teaching GEOM's provider aliasing about the possibility > > that aliases might be added later in time, and GEOM's devfs interaction > > layer not to worry about existing aliases during retaste. > > > > Discussed with: imp > > Relnotes: sure, if we don't end up reverting it > > This would break (and the effect can be quite persistent and hard to > repair, see explanation below) existing configuration as some GEOM > classes are not converted to support the new way of device > representation, so I'd like to request this change be reverted for now > until these are fixed. > > Consider the following configuration, where one have a hard drive > partitioned with GPT, like: > > => 40 1953525088 ada1 GPT (932G) > 40 262144 1 efi (128M) > 262184 8388608 2 freebsd-zfs (4.0G) > 8650792 67108864 3 freebsd-swap (32G) > 75759656 1877765472 4 freebsd-zfs (895G) > > Now, the first ZFS pool is created as root pool. ZFS gets an exclusive > hold of 'ada1p2' despite the pool is carefully created to use > /dev/diskid/p2 instead of ada1p2. > > ZFS writes the new device path to vdev label. For ZFS, this doesn't > matter much as it always checks the label. > > However, this will prevent GEOM from properly creating > /dev/diskid/. > > In order to prevent accidentally writing data to wrong disk, for "raw" > disk partitions it's usually a good idea to reference them with labels, > either by /dev/diskid or /dev/gptid. With /dev/ada1p2 exclusively > accessed by ZFS, the /dev/diskid/ representing the disk is > now gone. > > And to make the situation even worse, simply changing the partition > reference to the corresponding /dev/gptid/ doesn't really work > either, when one is encrypting partitions individually. In the example > above, adap4 is an encrypted partition and with the alias change, one > can no longer "geli attach" them via /dev/gptid/. They can still > be attached by the "canonical" path (/dev/ada1p4), but for the swap > partition that would completely defeat the purpose of using label (to > prevent accidentally writing to the wrong disk). > > For my case, reverting to an older kernel is not sufficient to fix the > configuration, because ZFS is recording the "canonical" device path > (/dev/ada1p2) and /dev/diskid label for this disk disappeared somewhat > permanently (I would have to find a USB drive somewhere to fix the root > pool to use the "right" device path). > > > Differential Revision: https://reviews.freebsd.org/D24968 > [...] > > Modified: head/sys/geom/label/g_label.c > > > ============================================================================== > > --- head/sys/geom/label/g_label.c Fri Jun 5 16:05:09 2020 > (r361837) > > +++ head/sys/geom/label/g_label.c Fri Jun 5 16:12:21 2020 > (r361838) > > @@ -344,18 +345,16 @@ g_label_taste(struct g_class *mp, struct > g_provider *p > > { > > struct g_label_metadata md; > > struct g_consumer *cp; > > + struct g_class *clsp; > > struct g_geom *gp; > > int i; > > + bool changed; > > > > g_trace(G_T_TOPOLOGY, "%s(%s, %s)", __func__, mp->name, pp->name); > > g_topology_assert(); > > > > G_LABEL_DEBUG(2, "Tasting %s.", pp->name); > > > > - /* Skip providers that are already open for writing. */ > > - if (pp->acw > 0) > > - return (NULL); > > - > > if (strcmp(pp->geom->class->name, mp->name) == 0) > > return (NULL); > > > > @@ -391,9 +390,16 @@ g_label_taste(struct g_class *mp, struct g_provider > *p > > if (md.md_provsize != pp->mediasize) > > break; > > > > + /* Skip providers that are already open for writing. */ > > + if (pp->acw > 0) { > > + g_access(cp, -1, 0, 0); > > + goto end; > > + } > > + > > (Is this still necessary when the eventual provider would be the real one?) > > I think symlink aliasing is a the right direction but we need to be > really careful to not break existing and legitimate usage. Since this > also breaks GELI when using with labels, I'd like to request that this > change be backed out for now until the consumers are correctly fixed. > These were the sorts of concerns I had when talking to cem@, but in his defense I wasn't able to come up with a concrete example so we convinced ourselves that my worry wasn't real. In light of this new data, I agree we should revert. This is complicated enough that we should have a couple of 'gotta work' use cases and make sure those work before we redo. There's a big reason I only did it for the 'nvd compat for nda' use case: I had what I thought was an irrational fear of sorting out this mess... Guess it turns out to be rational. :( Thanks Xin for the excellent explanation and example. Warner