| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 6 Jul 2022 12:06:38 +0800
From: Zorro Lang <zlang@...hat.com>
To: "Darrick J. Wong" <djwong@...nel.org>
Cc: Jeremy Bongio <bongiojp@...il.com>, Ted Tso <tytso@....edu>,
linux-ext4@...r.kernel.org, fstests@...r.kernel.org
Subject: Re: [PATCH v2] ext4/056: add a check to make sure ext4 uuid ioctls
get/set during fsstress.
On Tue, Jul 05, 2022 at 03:19:21PM -0700, Darrick J. Wong wrote:
> On Tue, Jul 05, 2022 at 11:25:07PM +0800, Zorro Lang wrote:
> > On Fri, Jul 01, 2022 at 01:13:32PM -0700, Jeremy Bongio wrote:
> > > Adds a utility to set/get uuid through ext4 ioctl. Executes the ioctls
> > > while running fsstress. These ioctls are used by tune2fs to safely change
> > > the uuid without racing other filesystem modifications.
> > >
> > > Signed-off-by: Jeremy Bongio <bongiojp@...il.com>
> > > ---
> > > .gitignore | 1 +
> > > src/Makefile | 4 +-
> > > src/uuid_ioctl.c | 106 +++++++++++++++++++++++++++++++++++++++++++++
> > > tests/ext4/056 | 55 +++++++++++++++++++++++
> > > tests/ext4/056.out | 2 +
> > > 5 files changed, 166 insertions(+), 2 deletions(-)
> > > create mode 100644 src/uuid_ioctl.c
> > > create mode 100755 tests/ext4/056
> > > create mode 100644 tests/ext4/056.out
> > >
> > > diff --git a/.gitignore b/.gitignore
> > > index ba0c572b..dab24d68 100644
> > > --- a/.gitignore
> > > +++ b/.gitignore
> > > @@ -169,6 +169,7 @@ tags
> > > /src/unwritten_mmap
> > > /src/unwritten_sync
> > > /src/usemem
> > > +/src/uuid_ioctl
> > > /src/writemod
> > > /src/writev_on_pagefault
> > > /src/xfsctl
> > > diff --git a/src/Makefile b/src/Makefile
> > > index 111ce1d9..e33e04de 100644
> > > --- a/src/Makefile
> > > +++ b/src/Makefile
> > > @@ -31,14 +31,14 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
> > > dio-invalidate-cache stat_test t_encrypted_d_revalidate \
> > > attr_replace_test swapon mkswap t_attr_corruption t_open_tmpfiles \
> > > fscrypt-crypt-util bulkstat_null_ocount splice-test chprojid_fail \
> > > - detached_mounts_propagation ext4_resize
> > > + detached_mounts_propagation ext4_resize uuid_ioctl
> > >
> > > EXTRA_EXECS = dmerror fill2attr fill2fs fill2fs_check scaleread.sh \
> > > btrfs_crc32c_forged_name.py
> > >
> > > SUBDIRS = log-writes perf
> > >
> > > -LLDLIBS = $(LIBHANDLE) $(LIBACL) -lpthread -lrt
> > > +LLDLIBS = $(LIBHANDLE) $(LIBACL) -lpthread -lrt -luuid
> > >
> > > ifeq ($(HAVE_XLOG_ASSIGN_LSN), true)
> > > LINUX_TARGETS += loggen
> > > diff --git a/src/uuid_ioctl.c b/src/uuid_ioctl.c
> > > new file mode 100644
> > > index 00000000..a4937478
> > > --- /dev/null
> > > +++ b/src/uuid_ioctl.c
> > > @@ -0,0 +1,106 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +
> > > +/*
> > > + * Test program which uses the raw ext4 set_fsuuid ioctl directly.
> > > + * SYNOPSIS:
> > > + * $0 COMMAND MOUNT_POINT [UUID]
> > > + *
> > > + * COMMAND must be either "get" or "set".
> > > + * The UUID must be a 16 octet represented as 32 hexadecimal digits in canonical
> > > + * textual representation, e.g. output from `uuidgen`.
> > > + *
> > > + */
> > > +
> > > +#include <stdio.h>
> > > +#include <fcntl.h>
> > > +#include <errno.h>
> > > +#include <unistd.h>
> > > +#include <stdint.h>
> > > +#include <stdlib.h>
> > > +#include <string.h>
> > > +#include <sys/ioctl.h>
> > > +#include <uuid/uuid.h>
> > > +#include <linux/fs.h>
> > > +
> > > +struct fsuuid {
> > > + __u32 fu_len;
> > > + __u32 fu_flags;
> > > + __u8 fu_uuid[];
> > > +};
> > > +
> > > +#define EXT4_IOC_SETFSUUID_FLAG_BLOCKING 0x1
> > > +
> > > +#ifndef EXT4_IOC_GETFSUUID
> > > +#define EXT4_IOC_GETFSUUID _IOR('f', 44, struct fsuuid)
> > > +#endif
> > > +
> > > +#ifndef EXT4_IOC_SETFSUUID
> > > +#define EXT4_IOC_SETFSUUID _IOW('f', 44, struct fsuuid)
> > > +#endif
> >
> > Will this be ext4 only ioctl? If this C program is only used for ext4 ioctl, I'd
> > like to rename it as "ext4_uuid_ioctl.c" (or something else make it not likes a
> > common test program). I think XFS doesn't has this ioctl, for now at least, it
> > get/set uuid by another way.
>
> Right now XFS only supports offline changes to UUIDs, but in principle
> we could implement this pair of ioctls some day.
Oh, good to know that, so this program might become a common/shared test program
later? If so, we can keep current file name.
>
> > > +
> > > +int main(int argc, char **argv)
> > > +{
> > > + int error, fd;
> > > + struct fsuuid *fsuuid = NULL;
> > > +
> > > + if (argc < 3) {
> > > + fprintf(stderr, "Invalid arguments\n");
> > > + return 1;
> > > + }
> > > +
> > > + fd = open(argv[2], O_RDONLY);
> > > + if (!fd) {
> > > + perror(argv[2]);
> > > + return 1;
> > > + }
> > > +
> > > + fsuuid = malloc(sizeof(*fsuuid) + sizeof(uuid_t));
> > > + if (!fsuuid) {
> > > + perror("malloc");
> > > + return 1;
> > > + }
> > > + fsuuid->fu_len = sizeof(uuid_t);
> > > +
> > > + if (strcmp(argv[1], "get") == 0) {
> > > + uuid_t uuid;
> > > + char uuid_str[37];
> > > +
> > > + if (ioctl(fd, EXT4_IOC_GETFSUUID, fsuuid)) {
> > > + fprintf(stderr, "%s while trying to get fs uuid\n",
> > > + strerror(errno));
> > > + return 1;
> > > + }
> > > +
> > > + memcpy(&uuid, fsuuid->fu_uuid, sizeof(uuid));
> > > + uuid_unparse(uuid, uuid_str);
> > > + printf("%s", uuid_str);
> > > + } else if (strcmp(argv[1], "set") == 0) {
> > > + uuid_t uuid;
> > > +
> > > + if (argc != 4) {
> > > + fprintf(stderr, "UUID argument missing.\n");
> > > + return 1;
> > > + }
> > > +
> > > + error = uuid_parse(argv[3], uuid);
> > > + if (error < 0) {
> > > + fprintf(stderr, "Invalid UUID. The UUID should be in "
> > > + "canonical format. Example: "
> > > + "8c628557-6987-42b2-ba16-b7cc79ddfb43\n");
> > > + return 1;
> > > + }
> > > +
> > > + memcpy(&fsuuid->fu_uuid, uuid, sizeof(uuid));
> > > + fsuuid->fu_flags = EXT4_IOC_SETFSUUID_FLAG_BLOCKING;
> > > + if (ioctl(fd, EXT4_IOC_SETFSUUID, fsuuid)) {
> > > + fprintf(stderr, "%s while trying to set fs uuid\n",
> > > + strerror(errno));
> > > + return 1;
> > > + }
> > > + } else {
> > > + fprintf(stderr, "Invalid command\n");
> > > + return 1;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > diff --git a/tests/ext4/056 b/tests/ext4/056
> > > new file mode 100755
> > > index 00000000..ebefb136
> > > --- /dev/null
> > > +++ b/tests/ext4/056
> > > @@ -0,0 +1,55 @@
> > > +#! /bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +#
> > > +# Test the set/get UUID ioctl.
> > > +#
> > > +
> > > +. ./common/preamble
> > > +_begin_fstest auto ioctl
> > > +
> > > +# Import common functions.
> > > +. ./common/filter
> > > +
> > > +# real QA test starts here
> > > +_supported_fs ext4
> > > +_require_scratch
> > > +_require_test_program uuid_ioctl
> > > +_require_command $UUIDGEN_PROG uuidgen
> > > +
> > > +UUID_IOCTL=$here/src/uuid_ioctl
> > > +
> > > +# if the ioctl is not supported by the kernel, then skip test.
> > > +current_uuid=$($UUID_IOCTL get $SCRATCH_MNT 2>&1)
> > > +if [[ "$current_uuid" =~ ^Inappropriate[[:space:]]ioctl ]]; then
> > > + _notrun "UUID ioctls are not supported by kernel."
> > > +fi
> > > +
> > > +# Create filesystem and mount
> >
> > Rather than this such obvious comment description, explain why the below
> > "metadata_csum_seed" is necessary?
> >
> > > +_scratch_mkfs_ext4 -O metadata_csum_seed >> $seqres.full 2>&1
> > > +_scratch_mount >> $seqres.full
> > > +
> > > +# Begin fsstress while modifying UUID
> > > +fsstress_args=$(_scale_fsstress_args -d $SCRATCH_MNT -p 15 -n 999999)
> > > +$FSSTRESS_PROG $fsstress_args > /dev/null 2>&1 &
> > > +fsstress_pid=$!
> > > +
> > > +for n in $(seq 1 20); do
> > > + new_uuid=$($UUIDGEN_PROG)
> > > +
> > > + echo "Setting UUID to ${new_uuid}" >> $seqres.full 2>&1
> > > + $UUID_IOCTL set $SCRATCH_MNT $new_uuid
> > > +
> > > + current_uuid=$($UUID_IOCTL get $SCRATCH_MNT)
> > > + echo "$UUID_IOCTL get $SCARTCH_MNT: $current_uuid" >> $seqres.full 2>&1
> > > + if [[ "$current_uuid" != "$new_uuid" ]]; then
> > > + echo "Current UUID ($current_uuid) does not equal what "
> > > + "was sent with the ioctl ($new_uuid)"
> >
> > Can these two lines really be printed?
>
> Let's hope not.
>
> > > + fi
> > > +done
> > > +
> > > +# success, all done
> > > +echo "Silence is golden"
> > > +kill $fsstress_pid >/dev/null 2>&1
> > > +wait
> >
> > We can't make sure the testing will exit normally, so better to make sure the
> > fsstress processes are killed in _cleanup(). You can refer to generic/390.
>
> Might be time to turn that (run fsstress/fsx in the background and kill it
> later) into a set of common helper functions?
>
> (Or just implement this fix now and we'll revisit it whenever I get
> around to submitting the "fsstress vs. online fsck" series[1].)
I remember Dave tried to clean up the _cleanup() things in a patchset named:
[RFC PATCH 0/8] fstests: _cleanup() overrides are a mess
it contains a patch named:
[PATCH 5/8] fstests: use a common fsstress cleanup function
I think Dave is busy recently, I don't know if he's still working on that.
(JFYI, to avoid you might take time to do reduplicate work)
Thanks,
Zorro
>
> --D
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfstests-dev.git/log/?h=refactor-fsmap-stress
>
> > Thanks,
> > Zorro
> >
> > > +status=0
> > > +exit
> > > diff --git a/tests/ext4/056.out b/tests/ext4/056.out
> > > new file mode 100644
> > > index 00000000..6142fcd2
> > > --- /dev/null
> > > +++ b/tests/ext4/056.out
> > > @@ -0,0 +1,2 @@
> > > +QA output created by 056
> > > +Silence is golden
> > > --
> > > 2.37.0.rc0.161.g10f37bed90-goog
> > >
> >
>
Powered by blists - more mailing lists