Re: [PATCH 1/3] grub-fs-tester: add luks1 and luks2 support

[Glenn Washburn]

On Thu,  3 Feb 2022 18:21:03 +0100
Pierre-Louis Bonicoli <pierre-louis.bonicoli@libregerbil.fr> wrote:

> The logical sector size used by LUKS1 is 512 bytes. LUKS2 uses 512 to
> 4069 bytes.

I like that this has been added here. However there's no test that
actually runs this new code. Please add another two fs tests one for
LUKS and one for LUKS2.

> 
> Signed-off-by: Pierre-Louis Bonicoli <pierre-louis.bonicoli@libregerbil.fr>
> ---
>  tests/util/grub-fs-tester.in | 58 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 55 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in
> index aa72b2174..7ed275fd0 100644
> --- a/tests/util/grub-fs-tester.in
> +++ b/tests/util/grub-fs-tester.in
> @@ -15,7 +15,12 @@ XORRISOFS_CHARSET="-input-charset UTF-8 -output-charset 
> UTF-8"
>  
>  # This wrapper is to ease insertion of valgrind or time statistics
>  run_it () {
> -    LC_ALL=C "$GRUBFSTEST" "$@"
> +    case x"$fs" in
> +    xluks*)
> +     LC_ALL=C echo -n pass | "$GRUBFSTEST" -C "$@";;

Why do this here instead of leaving this function alone and doing the
pipe and -C arg below when calling run_grubfstest?

> +    *)
> +     LC_ALL=C "$GRUBFSTEST" "$@";;
> +    esac
>  }
>  
>  range() {
> @@ -48,6 +53,8 @@ run_grubfstest () {
>  MINLOGSECSIZE=9
>  MAXLOGSECSIZE=9
>  case x"$fs" in
> +    xluks2*)
> +     MAXLOGSECSIZE=12;;
>      xntfs*)
>       MINLOGSECSIZE=8
>       MAXLOGSECSIZE=12;;
> @@ -335,7 +342,7 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" 
> "$MAXLOGSECSIZE" 1); do
>                   #FSLABEL="g;/_é莭莽😁кит u"
>                   ;;
>           # FS LIMITATION: reiserfs, extN and jfs label is at most 16 UTF-8 
> characters
> -             x"reiserfs_old" | x"reiserfs" | x"ext"* | x"lvm"* | x"mdraid"* 
> | x"jfs" | x"jfs_caseins")
> +             x"reiserfs_old" | x"reiserfs" | x"ext"* | x"lvm"* | x"luks"* | 
> x"mdraid"* | x"jfs" | x"jfs_caseins")
>                   FSLABEL="g;/éт 莭😁";;
>              # FS LIMITATION: No underscore, space, semicolon, slash or 
> international characters in UFS* in label. Limited to 32 UTF-8 characters
>               x"ufs1" | x"ufs1_sun" | x"ufs2")
> @@ -804,6 +811,12 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" 
> "$MAXLOGSECSIZE" 1); do
>                   MOUNTDEVICE="/dev/mapper/grub_test-testvol"
>                   MOUNTFS=ext2
>                   "mkfs.ext2" -L "$FSLABEL" -q "${MOUNTDEVICE}"  ;;
> +             x"luks"*)
> +                 echo -n pass | cryptsetup luksFormat --type $fs 
> --sector-size $SECSIZE --pbkdf pbkdf2 $LODEVICE
> +                 echo -n pass | cryptsetup open $LODEVICE grub_luks_test
> +                 MOUNTDEVICE="/dev/mapper/grub_luks_test"
> +                 MOUNTFS=ext2
> +                 "mkfs.ext2" -L "$FSLABEL" -q "${MOUNTDEVICE}"  ;;
>               xf2fs)
>                   "mkfs.f2fs" -l "$FSLABEL" -q "${MOUNTDEVICE}" ;;
>               xnilfs2)
> @@ -916,6 +929,20 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" 
> "$MAXLOGSECSIZE" 1); do
>                   GRUBDEVICE="mduuid/`mdadm --detail --export $MOUNTDEVICE | 
> grep MD_UUID=|sed 's,MD_UUID=,,g;s,:,,g'`";;
>               xlvm*)
>                   GRUBDEVICE="lvm/grub_test-testvol";;
> +             xluks*)
> +                 if test x"$fs" = xluks2 && ! (cryptsetup luksDump 
> --dump-json-metadata $LODEVICE | grep "\"sector_size\":$SECSIZE"); then
> +                         echo "Unexpected sector size for $LODEVICE 
> (expected: $SECSIZE)"
> +                         exit 1
> +                 fi
> +
> +                 uuid=$(cryptsetup luksUUID $LODEVICE | tr -d '-')
> +                 probe_uuid=$(@builddir@/grub-probe --device $MOUNTDEVICE 
> --target=cryptodisk_uuid)
> +                 if [ x"$uuid" != x"$probe_uuid" ]; then
> +                     echo "$fs probe command FAIL"
> +                     exit 1
> +                 fi
> +                 GRUBDEVICE="cryptouuid/${uuid}"
> +                 ;;
>           esac
>           GRUBDIR="($GRUBDEVICE)"
>           case x"$fs" in
> @@ -1071,6 +1098,18 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" 
> "$MAXLOGSECSIZE" 1); do
>                   sleep 1
>                   vgchange -a n grub_test
>                   ;;
> +             xluks*)
> +                     sleep 1

Bad indention. Why is this sleep desirable if there's a sleep in the
loop below?

> +                 for try in $(range 0 20 1); do
> +                     if umount "$MNTPOINTRW" ; then
> +                         break;
> +                     fi
> +                     sleep 1;
> +                 done
> +                 UMOUNT_TIME=$(date -u "+%Y-%m-%d %H:%M:%S")
> +                 sleep 1

Why sleeping here again? We already slept in the loop above. We need
more time?

> +                 cryptsetup close grub_luks_test
> +                 ;;
>               xmdraid*)
>                   sleep 1
>                   for try in $(range 0 20 1); do
> @@ -1117,6 +1156,10 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" 
> "$MAXLOGSECSIZE" 1); do
>                   vgchange -a y grub_test
>                   sleep 1
>                   mount -t "$MOUNTFS" "${MOUNTDEVICE}" "$MNTPOINTRO" -o 
> ${MOUNTOPTS}${SELINUXOPTS}ro ;;
> +             xluks*)
> +                 echo -n pass | cryptsetup open $LODEVICE grub_luks_test
> +                 sleep 1

Is this sleep really needed? Seems like you're trying to avoid a race,
but is the dm device ever never fully setup after cryptsetup returns?

> +                 mount -t "$MOUNTFS" "${MOUNTDEVICE}" "$MNTPOINTRO" -o 
> ${MOUNTOPTS}${SELINUXOPTS}ro ;;
>               xmdraid*)
>                   mdadm --assemble /dev/md/"${fs}_$NDEVICES" $LODEVICES
>                   sleep 1
> @@ -1281,7 +1324,12 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" 
> "$MAXLOGSECSIZE" 1); do
>               exit 1
>           fi
>  
> -         LSOUT=`run_grubfstest ls -- -l "($GRUBDEVICE)"`
> +         case x"$fs" in
> +             x"luks"*)
> +                 LSOUT=`run_grubfstest ls -- -l "($GRUBDEVICE)"|tail -n 1`;;

Here is where I'm think if would be better to do the pipe and add -C.
And why is this tail needed?

> +             *)
> +                 LSOUT=`run_grubfstest ls -- -l "($GRUBDEVICE)"`;;
> +         esac
>           if [ x"$NOFSLABEL" = xy ]; then
>               :
>           elif echo "$LSOUT" | grep -F "Label \`$FSLABEL'" > /dev/null; then
> @@ -1558,6 +1606,10 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" 
> "$MAXLOGSECSIZE" 1); do
>                   vgchange -a n grub_test
>                   sleep 1
>                   ;;
> +             xluks*)
> +                 cryptsetup close grub_luks_test
> +                 sleep 1
> +                 ;;
>           esac
>           case x"$fs" in
>               x"tarfs" | x"cpio_"* | x"iso9660" | xrockridge | xjoliet | 
> xrockridge_joliet | x"ziso9660" | x"romfs" | x"squash4_"* | x"iso9660_1999" | 
> xrockridge_1999 | xjoliet_1999 | xrockridge_joliet_1999) ;;

Glenn