Date: Tue, 23 Apr 2024 19:48:14 -0600 From: Warner Losh <imp@bsdimp.com> To: Jessica Clarke <jrtc27@freebsd.org> Cc: Warner Losh <imp@freebsd.org>, src-committers <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: <CANCZdfpNSCchECQSLBFjzkM67uQYEZxfs9q%2BUQkaRRXp-kx7gA@mail.gmail.com> In-Reply-To: <C602A595-0A57-4854-9D49-CDFB4A3CC42F@freebsd.org> References: <202404232150.43NLoEsI087796@gitrepo.freebsd.org> <C602A595-0A57-4854-9D49-CDFB4A3CC42F@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <jrtc27@freebsd.org> w= rote: > On 23 Apr 2024, at 22:50, Warner Losh <imp@FreeBSD.org> wrote: > > > The branch main has been updated by imp: > > > > URL: > https://cgit.FreeBSD.org/src/commit/?id=3Dcf04a7775a4e8ff6fd28c768be9daa3= d83dd382e > > > > 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 > > > > 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 <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> > > > > +#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 <div dir=3D"auto"><div><br><br><div class=3D"gmail_quote"><div dir=3D"ltr" = class=3D"gmail_attr">On Tue, Apr 23, 2024, 6:16=E2=80=AFPM Jessica Clarke &= lt;<a href=3D"mailto:jrtc27@freebsd.org">jrtc27@freebsd.org</a>> wrote:<= br></div><blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;borde= r-left:1px #ccc solid;padding-left:1ex">On 23 Apr 2024, at 22:50, Warner Lo= sh <imp@FreeBSD.org> wrote:<br> <br> > The branch main has been updated by imp:<br> > <br> > URL: <a href=3D"https://cgit.FreeBSD.org/src/commit/?id=3Dcf04a7775a4e= 8ff6fd28c768be9daa3d83dd382e" rel=3D"noreferrer noreferrer" target=3D"_blan= k">https://cgit.FreeBSD.org/src/commit/?id=3Dcf04a7775a4e8ff6fd28c768be9daa= 3d83dd382e</a><br> > <br> > commit cf04a7775a4e8ff6fd28c768be9daa3d83dd382e<br> > Author:=C2=A0 =C2=A0 =C2=A0Ricardo Branco <<a href=3D"mailto:rbranc= o@suse.de" target=3D"_blank" rel=3D"noreferrer">rbranco@suse.de</a>><br> > AuthorDate: 2024-04-23 21:47:56 +0000<br> > Commit:=C2=A0 =C2=A0 =C2=A0Warner Losh <imp@FreeBSD.org><br> > CommitDate: 2024-04-23 21:48:01 +0000<br> > <br> >=C2=A0 =C2=A0 swapon: Do not overwrite Linux swap header<br> <br> I don=E2=80=99t think this is a sensible default. swapon should, by default= , do<br> exactly what you asked for, rather than magically do something more<br> convoluted and different, with no way to opt out of it. The fact that<br> it looks like some other random OS happened to touch the partition in<br> the past shouldn=E2=80=99t be relevant; we=E2=80=99re not Linux, we=E2=80= =99re FreeBSD.<br> Especially in the age of GPT where the OSes have different GUIDs for<br> their swap partitions. Having some user-friendly way to the gnop dance<br> seems fine, but it should be optional and off by default, not forced<br> upon you. As it stands the only way to avoid this silent behaviour<br> change is to zero out the first 4K of the partition, which is not<br> user-friendly (and starts to sound like going back to the old days when<br> that was a common thing to do when partitioning).<br> <br> I also don=E2=80=99t believe the patch to be uncontested as you state in yo= ur<br> comment on the PR; kib@ had last commented on 1083 that it should be<br> documented and left up to the user to configure.<br></blockquote></div></di= v><div dir=3D"auto"><br></div><div dir=3D"auto">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...</div><div dir=3D"auto"><br></div><div dir=3D"auto">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...</div><div dir=3D"auto"><br></di= v><div dir=3D"auto">Warner</div><div dir=3D"auto"><br></div><div dir=3D"aut= o"><div class=3D"gmail_quote"><blockquote class=3D"gmail_quote" style=3D"ma= rgin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Jess<br> <br> >=C2=A0 =C2=A0 Reviewed by: imp, jhb<br> >=C2=A0 =C2=A0 Pull Request: <a href=3D"https://github.com/freebsd/freeb= sd-src/pull/1084" rel=3D"noreferrer noreferrer" target=3D"_blank">https://g= ithub.com/freebsd/freebsd-src/pull/1084</a><br> > ---<br> > sbin/swapon/Makefile=C2=A0 =C2=A0 |=C2=A0 2 ++<br> > sbin/swapon/extern.h=C2=A0 =C2=A0 |=C2=A0 5 ++++<br> > sbin/swapon/swaplinux.c | 66 +++++++++++++++++++++++++++++++++++++++++= ++++++++<br> > sbin/swapon/swapon.c=C2=A0 =C2=A0 | 54 +++++++++++++++++++++++++++++++= ++++++++-<br> > 4 files changed, 126 insertions(+), 1 deletion(-)<br> > <br> > diff --git a/sbin/swapon/Makefile b/sbin/swapon/Makefile<br> > index 27808aed5857..930e938e5b05 100644<br> > --- a/sbin/swapon/Makefile<br> > +++ b/sbin/swapon/Makefile<br> > @@ -6,6 +6,8 @@ LINKS=3D ${BINDIR}/swapon ${BINDIR}/swapoff<br> > LINKS+=3D ${BINDIR}/swapon ${BINDIR}/swapctl<br> > MLINKS=3D swapon.8 swapoff.8<br> > MLINKS+=3Dswapon.8 swapctl.8<br> > +SRCS=3D swapon.c swaplinux.c<br> > +HDRS+=3D extern.h<br> > <br> > LIBADD=3D util<br> > <br> > diff --git a/sbin/swapon/extern.h b/sbin/swapon/extern.h<br> > new file mode 100644<br> > index 000000000000..cb33dbbee953<br> > --- /dev/null<br> > +++ b/sbin/swapon/extern.h<br> > @@ -0,0 +1,5 @@<br> > +/*-<br> > + * SPDX-License-Identifier: BSD-3-Clause<br> > + */<br> > +<br> > +int is_linux_swap(const char *);<br> > diff --git a/sbin/swapon/swaplinux.c b/sbin/swapon/swaplinux.c<br> > new file mode 100644<br> > index 000000000000..d01a94345e68<br> > --- /dev/null<br> > +++ b/sbin/swapon/swaplinux.c<br> > @@ -0,0 +1,66 @@<br> > +/*-<br> > + * SPDX-License-Identifier: BSD-3-Clause<br> > + */<br> > +<br> > +#include <fcntl.h><br> > +#include <err.h><br> > +#include <stdint.h><br> > +#include <string.h><br> > +#include <unistd.h><br> > +<br> > +#include "extern.h"<br> > +<br> > +/*<br> > + * Definitions and structure taken from<br> > + * <a href=3D"https://github.com/util-linux/util-linux/blob/master/in= clude/swapheader.h" rel=3D"noreferrer noreferrer" target=3D"_blank">https:/= /github.com/util-linux/util-linux/blob/master/include/swapheader.h</a><br> > + */<br> > +<br> > +#define SWAP_VERSION 1<br> > +#define SWAP_UUID_LENGTH 16<br> > +#define SWAP_LABEL_LENGTH 16<br> > +#define SWAP_SIGNATURE "SWAPSPACE2"<br> > +#define SWAP_SIGNATURE_SZ (sizeof(SWAP_SIGNATURE) - 1)<br> > +<br> > +struct swap_header_v1_2 {<br> > + char=C2=A0 =C2=A0 =C2=A0 bootbits[1024];=C2=A0 =C2=A0 /* Space for d= isklabel etc. */<br> > + uint32_t=C2=A0 =C2=A0 =C2=A0 version;<br> > + uint32_t=C2=A0 =C2=A0 =C2=A0 last_page;<br> > + uint32_t=C2=A0 =C2=A0 =C2=A0 nr_badpages;<br> > + unsigned char uuid[SWAP_UUID_LENGTH];<br> > + char=C2=A0 =C2=A0 =C2=A0 volume_name[SWAP_LABEL_LENGTH];<br> > + uint32_t=C2=A0 =C2=A0 =C2=A0 padding[117];<br> > + uint32_t=C2=A0 =C2=A0 =C2=A0 badpages[1];<br> > +};<br> > +<br> > +typedef union {<br> > + struct swap_header_v1_2 header;<br> > + struct {<br> > + uint8_t reserved[4096 - SWAP_SIGNATURE_SZ];<br> > + char signature[SWAP_SIGNATURE_SZ];<br> > + } tail;<br> > +} swhdr_t;<br> > +<br> > +#define sw_version header.version<br> > +#define sw_volume_name header.volume_name<br> > +#define sw_signature tail.signature<br> > +<br> > +int<br> > +is_linux_swap(const char *name)<br> > +{<br> > + uint8_t buf[4096];<br> > + swhdr_t *hdr =3D (swhdr_t *) buf;<br> > + int fd;<br> > +<br> > + fd =3D open(name, O_RDONLY);<br> > + if (fd =3D=3D -1)<br> > + return (-1);<br> > +<br> > + if (read(fd, buf, 4096) !=3D 4096) {<br> > + close(fd);<br> > + return (-1);<br> > + }<br> > + close(fd);<br> > +<br> > + return (hdr->sw_version =3D=3D SWAP_VERSION &&<br> > +=C2=A0 =C2=A0 !memcmp(hdr->sw_signature, SWAP_SIGNATURE, SWAP_SIGN= ATURE_SZ));<br> > +}<br> > diff --git a/sbin/swapon/swapon.c b/sbin/swapon/swapon.c<br> > index 26a7dc22654a..0f2ad1f5e213 100644<br> > --- a/sbin/swapon/swapon.c<br> > +++ b/sbin/swapon/swapon.c<br> > @@ -54,10 +54,15 @@<br> > #include <string.h><br> > #include <unistd.h><br> > <br> > +#include "extern.h"<br> > +<br> > +#define _PATH_GNOP "/sbin/gnop"<br> > +<br> > static void usage(void) __dead2;<br> > static const char *swap_on_off(const char *, int, char *);<br> > static const char *swap_on_off_gbde(const char *, int);<br> > static const char *swap_on_off_geli(const char *, char *, int);<br> > +static const char *swap_on_off_linux(const char *, int);<br> > static const char *swap_on_off_md(const char *, char *, int);<br> > static const char *swap_on_off_sfile(const char *, int);<br> > static void swaplist(int, int, int);<br> > @@ -250,8 +255,13 @@ swap_on_off(const char *name, int doingall, char = *mntops)<br> > return (swap_on_off_geli(name, mntops, doingall));<br> > }<br> > <br> > - /* Swap on special file. */<br> > free(basebuf);<br> > +<br> > + /* Linux swap */<br> > + if (is_linux_swap(name))<br> > + return (swap_on_off_linux(name, doingall));<br> > +<br> > + /* Swap on special file. */<br> > return (swap_on_off_sfile(name, doingall));<br> > }<br> > <br> > @@ -466,6 +476,48 @@ swap_on_off_geli(const char *name, char *mntops, = int doingall)<br> > return (swap_on_off_sfile(name, doingall));<br> > }<br> > <br> > +static const char *<br> > +swap_on_off_linux(const char *name, int doingall)<br> > +{<br> > + const char *ret;<br> > + char *nopname;<br> > + size_t nopnamelen;<br> > + int error;<br> > +<br> > + if (which_prog =3D=3D SWAPON) {<br> > + /* Skip the header for Linux swap partitions */<br> > + error =3D run_cmd(NULL, "%s create -o 4096 %s", _PATH_GNOP= ,<br> > +=C2=A0 =C2=A0 name);<br> > + if (error) {<br> > + warnx("gnop (create) error: %s", name);<br> > + return (NULL);<br> > + }<br> > + }<br> > +<br> > + /* Append ".nop" to name */<br> > + nopnamelen =3D strlen(name) + sizeof(".nop");<br> > + nopname =3D (char *) malloc(nopnamelen);<br> > + if (nopname =3D=3D NULL)<br> > + err(1, "malloc()");<br> > + (void)strlcpy(nopname, name, nopnamelen);<br> > + (void)strlcat(nopname, ".nop", nopnamelen);<br> > +<br> > + ret =3D swap_on_off_sfile(nopname, doingall);<br> > +<br> > + if (which_prog =3D=3D SWAPOFF) {<br> > + error =3D run_cmd(NULL, "%s destroy %s", _PATH_GNOP, nopna= me);<br> > + if (error) {<br> > + warnx("gnop (destroy) error: %s", name);<br> > + free(nopname);<br> > + return (NULL);<br> > + }<br> > + }<br> > +<br> > + free(nopname);<br> > +<br> > + return (ret);<br> > +}<br> > +<br> > static const char *<br> > swap_on_off_md(const char *name, char *mntops, int doingall)<br> > {<br> <br> </blockquote></div></div></div> --000000000000de798a0616cddde4--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfpNSCchECQSLBFjzkM67uQYEZxfs9q%2BUQkaRRXp-kx7gA>