Date: Wed, 24 Apr 2024 01:15:48 +0100 From: Jessica Clarke <jrtc27@freebsd.org> To: Warner Losh <imp@FreeBSD.org> Cc: "src-committers@freebsd.org" <src-committers@FreeBSD.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@FreeBSD.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@FreeBSD.org> Subject: Re: git: cf04a7775a4e - main - swapon: Do not overwrite Linux swap header Message-ID: <C602A595-0A57-4854-9D49-CDFB4A3CC42F@freebsd.org> In-Reply-To: <202404232150.43NLoEsI087796@gitrepo.freebsd.org> References: <202404232150.43NLoEsI087796@gitrepo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 23 Apr 2024, at 22:50, Warner Losh <imp@FreeBSD.org> wrote: > The branch main has been updated by imp: >=20 > URL: = https://cgit.FreeBSD.org/src/commit/?id=3Dcf04a7775a4e8ff6fd28c768be9daa3d= 83dd382e >=20 > commit cf04a7775a4e8ff6fd28c768be9daa3d83dd382e > Author: Ricardo Branco <rbranco@suse.de> > AuthorDate: 2024-04-23 21:47:56 +0000 > Commit: Warner Losh <imp@FreeBSD.org> > CommitDate: 2024-04-23 21:48:01 +0000 >=20 > 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=99= re 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. 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(-) >=20 > 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 >=20 > LIBADD=3D util >=20 > 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 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 <string.h> > #include <unistd.h> >=20 > +#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)); > } >=20 > - /* 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)); > } >=20 > @@ -466,6 +476,48 @@ swap_on_off_geli(const char *name, char *mntops, = int doingall) > return (swap_on_off_sfile(name, doingall)); > } >=20 > +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) > {
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?C602A595-0A57-4854-9D49-CDFB4A3CC42F>