Re: git: cf04a7775a4e - main - swapon: Do not overwrite Linux swap header

On Tue, Apr 23, 2024, 6:16 PM Jessica Clarke <jrtc27@freebsd.org> wrote:

> 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=cf04a7775a4e8ff6fd28c768be9daa3d83dd382e
> >
> > 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’t 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’t be relevant; we’re not Linux, we’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’t 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= ${BINDIR}/swapon ${BINDIR}/swapoff
> > LINKS+= ${BINDIR}/swapon ${BINDIR}/swapctl
> > MLINKS= swapon.8 swapoff.8
> > MLINKS+=swapon.8 swapctl.8
> > +SRCS= swapon.c swaplinux.c
> > +HDRS+= extern.h
> >
> > LIBADD= 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 = (swhdr_t *) buf;
> > + int fd;
> > +
> > + fd = open(name, O_RDONLY);
> > + if (fd == -1)
> > + return (-1);
> > +
> > + if (read(fd, buf, 4096) != 4096) {
> > + close(fd);
> > + return (-1);
> > + }
> > + close(fd);
> > +
> > + return (hdr->sw_version == 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 == SWAPON) {
> > + /* Skip the header for Linux swap partitions */
> > + error = 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 = strlen(name) + sizeof(".nop");
> > + nopname = (char *) malloc(nopnamelen);
> > + if (nopname == NULL)
> > + err(1, "malloc()");
> > + (void)strlcpy(nopname, name, nopnamelen);
> > + (void)strlcat(nopname, ".nop", nopnamelen);
> > +
> > + ret = swap_on_off_sfile(nopname, doingall);
> > +
> > + if (which_prog == SWAPOFF) {
> > + error = 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)
> > {
>
>