From nobody Wed Apr 24 01:48:14 2024 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4VPMMc5h05z5JVvh for ; Wed, 24 Apr 2024 01:48:28 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-ej1-x634.google.com (mail-ej1-x634.google.com [IPv6:2a00:1450:4864:20::634]) (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 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4VPMMc3kW1z47Y4 for ; Wed, 24 Apr 2024 01:48:28 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-ej1-x634.google.com with SMTP id a640c23a62f3a-a55b93f5540so339133066b.1 for ; Tue, 23 Apr 2024 18:48:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20230601.gappssmtp.com; s=20230601; t=1713923307; x=1714528107; darn=freebsd.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=gebWBBrTSu7bpKrnWlPYXjxLzXv1sON+vhM7CEbEn2k=; b=YiZ4iRTAG2tZZS57u1/Sn1lzwp33sNJFenz/jqRIwvIuO2a8GB4nrvtuA6pxeE2Fyu ztCbHgUEP1YO0R9mMGLFsLNeLDf/Him3T3ePs5IlXJZg0ba+xqZ6qyazk6IIrjuyDbX9 nuP1SMcRKrFp8plTN4pG2atsAYrXDZ4XpRxHz6x3MfrwkUuyF2GRxD5QWczdjS5VVSY3 c8785blaJAvT6x5qknxg+QEG5J1oL9hsiAFORvhCW3euou6WR5AGfj7e2esnFDpcFuwF X7PxPojkO/tbnV0BFT5+FueqLjSShvuhGYWgTM4GvvvD0ZEItuwlO59P8U0ow8prcauz C1yA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1713923307; x=1714528107; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=gebWBBrTSu7bpKrnWlPYXjxLzXv1sON+vhM7CEbEn2k=; b=E72NpK7ARIJmSJiZMoKPVewKgcGb8G0pjCrimRKck7Bq99Ev9/s3Xi/zeNoyxmqFWr qwz6UtrMZ8S8iix8NZrjw9gtGi12nVuomzZhDVYmBfT3ICTNtq4Bn1ro+/RFrH6E3uK3 N3+QkPzX2jd2PlaEzmlssYrdY4qRL1rMAU0/SSQCW6S/y/ubza7xXBV8XK3LWhgtwZt2 EeTUMwNln40RAj2w06CoeER28VvNegndpkEdncJVXMnntGCYPiMZ9s9eUv2+mG8QeD9n qIlGk8PjXo0GBqjUm8/QXX1Zyqcx4upb1riADq0zvDuW6dBiWkZzANPMUs29T7yUkk2h xoPg== X-Forwarded-Encrypted: i=1; AJvYcCXG/Du4kcwREEGRQjqYhR4fsNW0okqhb4FayQvrR2a/MfI8rxkclux8iQE7sIvMnePewO7dhjvxK6fD8UYBthJPvZbrPYdWIbp9i9qKUC9C X-Gm-Message-State: AOJu0Yz32avZdwrcJEnh6i19b1yhZ8c4PtfjwR6MTYWsT/JudsRxgqtL TEx1upXi4NWqJOrzuKojzg4ypGAu3i9VAu2WxfOFG8MI8xEa1DUji78CNAYNwgNUHofQTRsY5Kw gsycbgYV12cGg18WLiEhYXqiag+AMH6tnU+t1sg== X-Google-Smtp-Source: AGHT+IGnfIafLiwY2r+J411vyzZBKl40iA2T89hKF4lbKaPJxagoXpiEqXNaZzPa+nlm5FAth02udHt5zpDnnPSO/A0= X-Received: by 2002:a17:906:b00b:b0:a58:7832:c278 with SMTP id v11-20020a170906b00b00b00a587832c278mr550823ejy.63.1713923306707; Tue, 23 Apr 2024 18:48:26 -0700 (PDT) List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-all@freebsd.org Sender: owner-dev-commits-src-all@FreeBSD.org MIME-Version: 1.0 References: <202404232150.43NLoEsI087796@gitrepo.freebsd.org> In-Reply-To: From: Warner Losh Date: Tue, 23 Apr 2024 19:48:14 -0600 Message-ID: Subject: Re: git: cf04a7775a4e - main - swapon: Do not overwrite Linux swap header To: Jessica Clarke Cc: Warner Losh , src-committers , "" , "" Content-Type: multipart/alternative; boundary="000000000000de798a0616cddde4" X-Spamd-Bar: ---- X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:2a00:1450::/32, country:US] X-Rspamd-Queue-Id: 4VPMMc3kW1z47Y4 --000000000000de798a0616cddde4 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, Apr 23, 2024, 6:16=E2=80=AFPM Jessica Clarke w= rote: > On 23 Apr 2024, at 22:50, Warner Losh wrote: > > > The branch main has been updated by imp: > > > > URL: > https://cgit.FreeBSD.org/src/commit/?id=3Dcf04a7775a4e8ff6fd28c768be9daa3= d83dd382e > > > > commit cf04a7775a4e8ff6fd28c768be9daa3d83dd382e > > Author: Ricardo Branco > > AuthorDate: 2024-04-23 21:47:56 +0000 > > Commit: Warner Losh > > CommitDate: 2024-04-23 21:48:01 +0000 > > > > swapon: Do not overwrite Linux swap header > > I don=E2=80=99t think this is a sensible default. swapon should, by defau= lt, do > exactly what you asked for, rather than magically do something more > convoluted and different, with no way to opt out of it. The fact that > it looks like some other random OS happened to touch the partition in > the past shouldn=E2=80=99t be relevant; we=E2=80=99re not Linux, we=E2=80= =99re FreeBSD. > Especially in the age of GPT where the OSes have different GUIDs for > their swap partitions. Having some user-friendly way to the gnop dance > seems fine, but it should be optional and off by default, not forced > upon you. As it stands the only way to avoid this silent behaviour > change is to zero out the first 4K of the partition, which is not > user-friendly (and starts to sound like going back to the old days when > that was a common thing to do when partitioning). > > I also don=E2=80=99t believe the patch to be uncontested as you state in = your > comment on the PR; kib@ had last commented on 1083 that it should be > documented and left up to the user to configure. > Yea. I'll back it out... i didn't realize the coupling of the issues until i re-read it... also armv7 is broken... I'm thinking we should just close those two PRs: they aren't ready and the argument is whether to do it or not... Warner Jess > > > Reviewed by: imp, jhb > > Pull Request: https://github.com/freebsd/freebsd-src/pull/1084 > > --- > > sbin/swapon/Makefile | 2 ++ > > sbin/swapon/extern.h | 5 ++++ > > sbin/swapon/swaplinux.c | 66 > +++++++++++++++++++++++++++++++++++++++++++++++++ > > sbin/swapon/swapon.c | 54 +++++++++++++++++++++++++++++++++++++++- > > 4 files changed, 126 insertions(+), 1 deletion(-) > > > > diff --git a/sbin/swapon/Makefile b/sbin/swapon/Makefile > > index 27808aed5857..930e938e5b05 100644 > > --- a/sbin/swapon/Makefile > > +++ b/sbin/swapon/Makefile > > @@ -6,6 +6,8 @@ LINKS=3D ${BINDIR}/swapon ${BINDIR}/swapoff > > LINKS+=3D ${BINDIR}/swapon ${BINDIR}/swapctl > > MLINKS=3D swapon.8 swapoff.8 > > MLINKS+=3Dswapon.8 swapctl.8 > > +SRCS=3D swapon.c swaplinux.c > > +HDRS+=3D extern.h > > > > LIBADD=3D util > > > > diff --git a/sbin/swapon/extern.h b/sbin/swapon/extern.h > > new file mode 100644 > > index 000000000000..cb33dbbee953 > > --- /dev/null > > +++ b/sbin/swapon/extern.h > > @@ -0,0 +1,5 @@ > > +/*- > > + * SPDX-License-Identifier: BSD-3-Clause > > + */ > > + > > +int is_linux_swap(const char *); > > diff --git a/sbin/swapon/swaplinux.c b/sbin/swapon/swaplinux.c > > new file mode 100644 > > index 000000000000..d01a94345e68 > > --- /dev/null > > +++ b/sbin/swapon/swaplinux.c > > @@ -0,0 +1,66 @@ > > +/*- > > + * SPDX-License-Identifier: BSD-3-Clause > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include "extern.h" > > + > > +/* > > + * Definitions and structure taken from > > + * > https://github.com/util-linux/util-linux/blob/master/include/swapheader.h > > + */ > > + > > +#define SWAP_VERSION 1 > > +#define SWAP_UUID_LENGTH 16 > > +#define SWAP_LABEL_LENGTH 16 > > +#define SWAP_SIGNATURE "SWAPSPACE2" > > +#define SWAP_SIGNATURE_SZ (sizeof(SWAP_SIGNATURE) - 1) > > + > > +struct swap_header_v1_2 { > > + char bootbits[1024]; /* Space for disklabel etc. */ > > + uint32_t version; > > + uint32_t last_page; > > + uint32_t nr_badpages; > > + unsigned char uuid[SWAP_UUID_LENGTH]; > > + char volume_name[SWAP_LABEL_LENGTH]; > > + uint32_t padding[117]; > > + uint32_t badpages[1]; > > +}; > > + > > +typedef union { > > + struct swap_header_v1_2 header; > > + struct { > > + uint8_t reserved[4096 - SWAP_SIGNATURE_SZ]; > > + char signature[SWAP_SIGNATURE_SZ]; > > + } tail; > > +} swhdr_t; > > + > > +#define sw_version header.version > > +#define sw_volume_name header.volume_name > > +#define sw_signature tail.signature > > + > > +int > > +is_linux_swap(const char *name) > > +{ > > + uint8_t buf[4096]; > > + swhdr_t *hdr =3D (swhdr_t *) buf; > > + int fd; > > + > > + fd =3D open(name, O_RDONLY); > > + if (fd =3D=3D -1) > > + return (-1); > > + > > + if (read(fd, buf, 4096) !=3D 4096) { > > + close(fd); > > + return (-1); > > + } > > + close(fd); > > + > > + return (hdr->sw_version =3D=3D SWAP_VERSION && > > + !memcmp(hdr->sw_signature, SWAP_SIGNATURE, SWAP_SIGNATURE_SZ)); > > +} > > diff --git a/sbin/swapon/swapon.c b/sbin/swapon/swapon.c > > index 26a7dc22654a..0f2ad1f5e213 100644 > > --- a/sbin/swapon/swapon.c > > +++ b/sbin/swapon/swapon.c > > @@ -54,10 +54,15 @@ > > #include > > #include > > > > +#include "extern.h" > > + > > +#define _PATH_GNOP "/sbin/gnop" > > + > > static void usage(void) __dead2; > > static const char *swap_on_off(const char *, int, char *); > > static const char *swap_on_off_gbde(const char *, int); > > static const char *swap_on_off_geli(const char *, char *, int); > > +static const char *swap_on_off_linux(const char *, int); > > static const char *swap_on_off_md(const char *, char *, int); > > static const char *swap_on_off_sfile(const char *, int); > > static void swaplist(int, int, int); > > @@ -250,8 +255,13 @@ swap_on_off(const char *name, int doingall, char > *mntops) > > return (swap_on_off_geli(name, mntops, doingall)); > > } > > > > - /* Swap on special file. */ > > free(basebuf); > > + > > + /* Linux swap */ > > + if (is_linux_swap(name)) > > + return (swap_on_off_linux(name, doingall)); > > + > > + /* Swap on special file. */ > > return (swap_on_off_sfile(name, doingall)); > > } > > > > @@ -466,6 +476,48 @@ swap_on_off_geli(const char *name, char *mntops, > int doingall) > > return (swap_on_off_sfile(name, doingall)); > > } > > > > +static const char * > > +swap_on_off_linux(const char *name, int doingall) > > +{ > > + const char *ret; > > + char *nopname; > > + size_t nopnamelen; > > + int error; > > + > > + if (which_prog =3D=3D SWAPON) { > > + /* Skip the header for Linux swap partitions */ > > + error =3D run_cmd(NULL, "%s create -o 4096 %s", _PATH_GNOP, > > + name); > > + if (error) { > > + warnx("gnop (create) error: %s", name); > > + return (NULL); > > + } > > + } > > + > > + /* Append ".nop" to name */ > > + nopnamelen =3D strlen(name) + sizeof(".nop"); > > + nopname =3D (char *) malloc(nopnamelen); > > + if (nopname =3D=3D NULL) > > + err(1, "malloc()"); > > + (void)strlcpy(nopname, name, nopnamelen); > > + (void)strlcat(nopname, ".nop", nopnamelen); > > + > > + ret =3D swap_on_off_sfile(nopname, doingall); > > + > > + if (which_prog =3D=3D SWAPOFF) { > > + error =3D run_cmd(NULL, "%s destroy %s", _PATH_GNOP, nopname); > > + if (error) { > > + warnx("gnop (destroy) error: %s", name); > > + free(nopname); > > + return (NULL); > > + } > > + } > > + > > + free(nopname); > > + > > + return (ret); > > +} > > + > > static const char * > > swap_on_off_md(const char *name, char *mntops, int doingall) > > { > > --000000000000de798a0616cddde4 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Tue, Apr 23, 2024, 6:16=E2=80=AFPM Jessica Clarke &= lt;jrtc27@freebsd.org> wrote:<= br>
On 23 Apr 2024, at 22:50, Warner Lo= sh <imp@FreeBSD.org> wrote:

> The branch main has been updated by imp:
>
> URL: https://cgit.FreeBSD.org/src/commit/?id=3Dcf04a7775a4e8ff6fd28c768be9daa= 3d83dd382e
>
> commit cf04a7775a4e8ff6fd28c768be9daa3d83dd382e
> Author:=C2=A0 =C2=A0 =C2=A0Ricardo Branco <rbranco@suse.de>
> AuthorDate: 2024-04-23 21:47:56 +0000
> Commit:=C2=A0 =C2=A0 =C2=A0Warner Losh <imp@FreeBSD.org>
> CommitDate: 2024-04-23 21:48:01 +0000
>
>=C2=A0 =C2=A0 swapon: Do not overwrite Linux swap header

I don=E2=80=99t think this is a sensible default. swapon should, by default= , do
exactly what you asked for, rather than magically do something more
convoluted and different, with no way to opt out of it. The fact that
it looks like some other random OS happened to touch the partition in
the past shouldn=E2=80=99t be relevant; we=E2=80=99re not Linux, we=E2=80= =99re FreeBSD.
Especially in the age of GPT where the OSes have different GUIDs for
their swap partitions. Having some user-friendly way to the gnop dance
seems fine, but it should be optional and off by default, not forced
upon you. As it stands the only way to avoid this silent behaviour
change is to zero out the first 4K of the partition, which is not
user-friendly (and starts to sound like going back to the old days when
that was a common thing to do when partitioning).

I also don=E2=80=99t believe the patch to be uncontested as you state in yo= ur
comment on the PR; kib@ had last commented on 1083 that it should be
documented and left up to the user to configure.

Yea. I'll back it out..= . i didn't realize the coupling of the issues until i re-read it... als= o armv7 is broken...

I&#= 39;m thinking we should just close those two PRs: they aren't ready and= the argument is whether to do it or not...

Warner

Jess

>=C2=A0 =C2=A0 Reviewed by: imp, jhb
>=C2=A0 =C2=A0 Pull Request: https://g= ithub.com/freebsd/freebsd-src/pull/1084
> ---
> sbin/swapon/Makefile=C2=A0 =C2=A0 |=C2=A0 2 ++
> sbin/swapon/extern.h=C2=A0 =C2=A0 |=C2=A0 5 ++++
> sbin/swapon/swaplinux.c | 66 +++++++++++++++++++++++++++++++++++++++++= ++++++++
> sbin/swapon/swapon.c=C2=A0 =C2=A0 | 54 +++++++++++++++++++++++++++++++= ++++++++-
> 4 files changed, 126 insertions(+), 1 deletion(-)
>
> diff --git a/sbin/swapon/Makefile b/sbin/swapon/Makefile
> index 27808aed5857..930e938e5b05 100644
> --- a/sbin/swapon/Makefile
> +++ b/sbin/swapon/Makefile
> @@ -6,6 +6,8 @@ LINKS=3D ${BINDIR}/swapon ${BINDIR}/swapoff
> LINKS+=3D ${BINDIR}/swapon ${BINDIR}/swapctl
> MLINKS=3D swapon.8 swapoff.8
> MLINKS+=3Dswapon.8 swapctl.8
> +SRCS=3D swapon.c swaplinux.c
> +HDRS+=3D extern.h
>
> LIBADD=3D util
>
> diff --git a/sbin/swapon/extern.h b/sbin/swapon/extern.h
> new file mode 100644
> index 000000000000..cb33dbbee953
> --- /dev/null
> +++ b/sbin/swapon/extern.h
> @@ -0,0 +1,5 @@
> +/*-
> + * SPDX-License-Identifier: BSD-3-Clause
> + */
> +
> +int is_linux_swap(const char *);
> diff --git a/sbin/swapon/swaplinux.c b/sbin/swapon/swaplinux.c
> new file mode 100644
> index 000000000000..d01a94345e68
> --- /dev/null
> +++ b/sbin/swapon/swaplinux.c
> @@ -0,0 +1,66 @@
> +/*-
> + * SPDX-License-Identifier: BSD-3-Clause
> + */
> +
> +#include <fcntl.h>
> +#include <err.h>
> +#include <stdint.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +#include "extern.h"
> +
> +/*
> + * Definitions and structure taken from
> + * https:/= /github.com/util-linux/util-linux/blob/master/include/swapheader.h
> + */
> +
> +#define SWAP_VERSION 1
> +#define SWAP_UUID_LENGTH 16
> +#define SWAP_LABEL_LENGTH 16
> +#define SWAP_SIGNATURE "SWAPSPACE2"
> +#define SWAP_SIGNATURE_SZ (sizeof(SWAP_SIGNATURE) - 1)
> +
> +struct swap_header_v1_2 {
> + char=C2=A0 =C2=A0 =C2=A0 bootbits[1024];=C2=A0 =C2=A0 /* Space for d= isklabel etc. */
> + uint32_t=C2=A0 =C2=A0 =C2=A0 version;
> + uint32_t=C2=A0 =C2=A0 =C2=A0 last_page;
> + uint32_t=C2=A0 =C2=A0 =C2=A0 nr_badpages;
> + unsigned char uuid[SWAP_UUID_LENGTH];
> + char=C2=A0 =C2=A0 =C2=A0 volume_name[SWAP_LABEL_LENGTH];
> + uint32_t=C2=A0 =C2=A0 =C2=A0 padding[117];
> + uint32_t=C2=A0 =C2=A0 =C2=A0 badpages[1];
> +};
> +
> +typedef union {
> + struct swap_header_v1_2 header;
> + struct {
> + uint8_t reserved[4096 - SWAP_SIGNATURE_SZ];
> + char signature[SWAP_SIGNATURE_SZ];
> + } tail;
> +} swhdr_t;
> +
> +#define sw_version header.version
> +#define sw_volume_name header.volume_name
> +#define sw_signature tail.signature
> +
> +int
> +is_linux_swap(const char *name)
> +{
> + uint8_t buf[4096];
> + swhdr_t *hdr =3D (swhdr_t *) buf;
> + int fd;
> +
> + fd =3D open(name, O_RDONLY);
> + if (fd =3D=3D -1)
> + return (-1);
> +
> + if (read(fd, buf, 4096) !=3D 4096) {
> + close(fd);
> + return (-1);
> + }
> + close(fd);
> +
> + return (hdr->sw_version =3D=3D SWAP_VERSION &&
> +=C2=A0 =C2=A0 !memcmp(hdr->sw_signature, SWAP_SIGNATURE, SWAP_SIGN= ATURE_SZ));
> +}
> diff --git a/sbin/swapon/swapon.c b/sbin/swapon/swapon.c
> index 26a7dc22654a..0f2ad1f5e213 100644
> --- a/sbin/swapon/swapon.c
> +++ b/sbin/swapon/swapon.c
> @@ -54,10 +54,15 @@
> #include <string.h>
> #include <unistd.h>
>
> +#include "extern.h"
> +
> +#define _PATH_GNOP "/sbin/gnop"
> +
> static void usage(void) __dead2;
> static const char *swap_on_off(const char *, int, char *);
> static const char *swap_on_off_gbde(const char *, int);
> static const char *swap_on_off_geli(const char *, char *, int);
> +static const char *swap_on_off_linux(const char *, int);
> static const char *swap_on_off_md(const char *, char *, int);
> static const char *swap_on_off_sfile(const char *, int);
> static void swaplist(int, int, int);
> @@ -250,8 +255,13 @@ swap_on_off(const char *name, int doingall, char = *mntops)
> return (swap_on_off_geli(name, mntops, doingall));
> }
>
> - /* Swap on special file. */
> free(basebuf);
> +
> + /* Linux swap */
> + if (is_linux_swap(name))
> + return (swap_on_off_linux(name, doingall));
> +
> + /* Swap on special file. */
> return (swap_on_off_sfile(name, doingall));
> }
>
> @@ -466,6 +476,48 @@ swap_on_off_geli(const char *name, char *mntops, = int doingall)
> return (swap_on_off_sfile(name, doingall));
> }
>
> +static const char *
> +swap_on_off_linux(const char *name, int doingall)
> +{
> + const char *ret;
> + char *nopname;
> + size_t nopnamelen;
> + int error;
> +
> + if (which_prog =3D=3D SWAPON) {
> + /* Skip the header for Linux swap partitions */
> + error =3D run_cmd(NULL, "%s create -o 4096 %s", _PATH_GNOP= ,
> +=C2=A0 =C2=A0 name);
> + if (error) {
> + warnx("gnop (create) error: %s", name);
> + return (NULL);
> + }
> + }
> +
> + /* Append ".nop" to name */
> + nopnamelen =3D strlen(name) + sizeof(".nop");
> + nopname =3D (char *) malloc(nopnamelen);
> + if (nopname =3D=3D NULL)
> + err(1, "malloc()");
> + (void)strlcpy(nopname, name, nopnamelen);
> + (void)strlcat(nopname, ".nop", nopnamelen);
> +
> + ret =3D swap_on_off_sfile(nopname, doingall);
> +
> + if (which_prog =3D=3D SWAPOFF) {
> + error =3D run_cmd(NULL, "%s destroy %s", _PATH_GNOP, nopna= me);
> + if (error) {
> + warnx("gnop (destroy) error: %s", name);
> + free(nopname);
> + return (NULL);
> + }
> + }
> +
> + free(nopname);
> +
> + return (ret);
> +}
> +
> static const char *
> swap_on_off_md(const char *name, char *mntops, int doingall)
> {

--000000000000de798a0616cddde4--