Re: [PATCH fstests v2] overlay: add a test for redirect_dir=followdata

[Giuseppe Scrivano <giuseppe@scrivano.org>]

Hi Amir,

Amir Goldstein <amir73il@gmail.com> writes:

> On Mon, Nov 9, 2026 at 2:40 PM Giuseppe Scrivano <giuseppe@scrivano.org> wrote:
>>
>> Test that data-only layers select redirect_dir=followdata, which follows
>> redirects from lower to lowerdata from any lower layer, with userxattr
>> and trusted xattrs, and that redirect_dir=nofollow is refused with
>> data-only layers.
>>
>> Also test following an absolute redirect past a lower layer that has the
>> parent directory of the target, which failed with EIO before the fix
>> "ovl: do not always clear absolute_redirect".
>>
>> Signed-off-by: Giuseppe Scrivano <giuseppe@scrivano.org>
>> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  common/overlay        |  15 +++++
>>  tests/overlay/090     | 149 ++++++++++++++++++++++++++++++++++++++++++
>>  tests/overlay/090.out |  43 ++++++++++++
>>  3 files changed, 207 insertions(+)
>>  create mode 100755 tests/overlay/090
>>  create mode 100644 tests/overlay/090.out
>>
>> diff --git a/common/overlay b/common/overlay
>> index 8e89d6e4..382960c3 100644
>> --- a/common/overlay
>> +++ b/common/overlay
>> @@ -286,6 +286,21 @@ _require_scratch_overlay_datadir_without_metacopy()
>>
>>  }
>>
>> +# Check kernel support for the "redirect_dir=followdata" mount option
>> +_require_scratch_overlay_redirect_followdata()
>> +{
>> +       local lowerdir="$OVL_BASE_SCRATCH_MNT/$OVL_UPPER"
>> +       local datadir="$OVL_BASE_SCRATCH_MNT/$OVL_LOWER"
>> +
>> +       _scratch_mkfs > /dev/null 2>&1
>> +       _overlay_scratch_mount_opts \
>> +               -o"lowerdir+=$lowerdir,datadir+=$datadir" \
>> +               -o"userxattr,redirect_dir=followdata" > /dev/null 2>&1 || \
>> +               _notrun "overlay redirect_dir=followdata not supported on ${SCRATCH_DEV}"
>> +
>> +       _scratch_unmount
>> +}
>> +
>>
>>  # Helper function to check underlying dirs of overlay filesystem
>>  _overlay_fsck_dirs()
>> diff --git a/tests/overlay/090 b/tests/overlay/090
>> new file mode 100755
>> index 00000000..6eeeacdf
>> --- /dev/null
>> +++ b/tests/overlay/090
>> @@ -0,0 +1,149 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2026 Giuseppe Scrivano.  All Rights Reserved.
>> +#
>> +# FS QA Test 090
>> +#
>> +# Test the redirect_dir=followdata mount option.
>> +#
>> +# Data-only layers select redirect_dir=followdata, which follows redirects
>> +# from any lower layer to a data-only layer, and cannot be used with
>> +# redirect_dir=nofollow.
>> +#
>> +# Also test following an absolute redirect past a lower layer that has the
>> +# parent directory of the redirect target, with metacopy=on.
>> +#
>> +. ./common/preamble
>> +_begin_fstest auto quick metacopy redirect attr
>> +
>> +# Import common functions.
>> +. ./common/filter
>> +. ./common/attr
>> +
>> +# We use non-default scratch underlying overlay dirs, we need to check
>> +# them explicitly after test.
>> +_require_scratch_nocheck
>> +_require_attrs user
>> +_require_attrs trusted
>> +_require_scratch_overlay_redirect_followdata
>> +_require_scratch_overlay_features redirect_dir metacopy
>> +_require_scratch_overlay_lowerdata_layers
>> +
>> +data=$OVL_BASE_SCRATCH_MNT/data
>> +l1=$OVL_BASE_SCRATCH_MNT/lower1
>> +l2=$OVL_BASE_SCRATCH_MNT/lower2
>> +l3=$OVL_BASE_SCRATCH_MNT/lower3
>> +upper=$OVL_BASE_SCRATCH_MNT/upper
>> +work=$OVL_BASE_SCRATCH_MNT/work
>> +layers="$l3:$l2:$l1::$data"
>> +
>> +# Make a metacopy file <file> with the redirect <redirect>, for both
>> +# userxattr and trusted xattr mounts
>> +make_stub()
>> +{
>> +       local file=$1 redirect=$2 x
>> +
>> +       touch $file
>> +       for x in user.overlay. ${OVL_XATTR_METACOPY%metacopy}; do
>> +               $SETFATTR_PROG -n ${x}metacopy $file
>> +               $SETFATTR_PROG -n ${x}redirect -v $redirect $file
>> +       done
>> +       truncate -s 4 $file
>> +}
>> +
>> +# Mount the layers $layers with the options <opts>, print redirect_dir=followdata
>> +# if the mount shows it and <opts> has userxattr or redirect_dir (otherwise it
>> +# depends on the kernel config), and the content of each of the following files,
>> +# or the error reading it
>> +check()
>> +{
>> +       local opts=$1 f
>> +
>> +       shift
>> +       echo "== $opts"
>> +       _overlay_scratch_mount_dirs "$layers" $upper $work -o $opts
>> +       case $opts in
>> +       *userxattr*|*redirect_dir=*)
>> +               _fs_options $OVL_BASE_SCRATCH_MNT | grep -o redirect_dir=followdata
>> +               ;;
>> +       esac
>> +       for f; do
>> +               echo "$f: $(cat $SCRATCH_MNT/$f 2>&1 | sed 's/^cat: .*: //')"
>> +       done
>> +       _unmount $SCRATCH_MNT
>> +}
>> +
>> +# Mounting the layers $layers with the options <opts> must fail
>> +check_fail()
>> +{
>> +       local opts=$1
>> +
>> +       echo "== $opts"
>> +       _overlay_scratch_mount_dirs "$layers" $upper $work -o $opts \
>> +               >> $seqres.full 2>&1 && echo "mount succeeded" && \
>> +               _unmount $SCRATCH_MNT
>> +}
>> +
>> +_scratch_mkfs
>> +mkdir -p $data/base $l1/base $l2/base $l3 $upper $work
>> +echo -n "data" > $data/base/file
>> +
>> +make_stub $l1/s1 /base/file
>> +make_stub $l2/s2 /base/file
>> +make_stub $l3/s3 /base/file
>> +# The redirect target in a lower layer below s3, which is not looked up
>> +echo -n "lowr" > $l2/base/file
>> +# Redirects that are not followed: relative, directory, upper
>> +echo -n "lowr" > $l2/reltarget
>> +make_stub $l3/rel reltarget
>> +mkdir $l3/dir
>> +$SETFATTR_PROG -n user.overlay.redirect -v /base $l3/dir
>> +$SETFATTR_PROG -n $OVL_XATTR_REDIRECT -v /base $l3/dir
>> +make_stub $upper/up /base/file
>> +
>> +# followdata is selected by default with userxattr and a data-only layer
>> +check userxattr s1 s2 s3
>> +check userxattr,redirect_dir=followdata s1 s2 s3 rel dir up
>> +# a data-only layer cannot be used without following redirects
>> +check_fail userxattr,redirect_dir=nofollow
>> +check_fail userxattr,redirect_dir=off
>
> The test looks good but this expectation of failure is problematic.
>
> First of all the test would fail with redirect_always_follow=N
> because that makes redirect_dir=off => followdata
>
> But I also think that redirect_dir=off should not fail with
> redirect_always_follow=Y.
>
> I will explain with the documentation.
> You documentation v2 patch says:
>
> :A data-only layer selects "redirect_dir=followdata", unless
> "redirect_dir=follow"
> :or "redirect_dir=on" is in effect.
>
> So it implies redirect_dir=off => followdata
>
> All that redirect_dir=off really means is "don't create redirects".
>
> I amended the document as follows to clarify that:
>
> - "redirect_dir=on":
>     Redirects are created and followed.
> - "redirect_dir=follow":
>     Redirects are not created, but followed.
> - "redirect_dir=followdata":
>     Redirects are not created.
>     Only redirects from lower to lowerdata are followed.
> - "redirect_dir=nofollow":
>     Redirects are not created and not followed.
> - "redirect_dir=off":
>     Redirects are not created.
>     Which redirects are followed depends on "redirect_always_follow" in the
>     kernel/module config, on data-only lower layers and on the "userxattr"
>     mount option.
>
> Made this off => followdata change in the code and pushed to ovl-fixes:
>
> -static int ovl_redirect_follow_mode(size_t nr_data)
> +int ovl_redirect_follow_mode(bool follow_lower, size_t nr_data)
>  {
> -       return ovl_redirect_always_follow ? OVL_REDIRECT_FOLLOW :
> -                                           ovl_redirect_nofollow_mode(nr_data);
> +       if (ovl_redirect_always_follow && follow_lower)
> +               return OVL_REDIRECT_FOLLOW;
> +
> +       /* Data-only layers cannot be used without following redirects
> to them */
> +       return nr_data ? OVL_REDIRECT_FOLLOWDATA : OVL_REDIRECT_NOFOLLOW;
>  }
>
>  /*
> @@ -137,7 +140,7 @@ static int ovl_redirect_follow_mode(size_t nr_data)
>  static int ovl_redirect_mode_def(void)
>  {
>         return ovl_redirect_dir_def ? OVL_REDIRECT_ON :
> -                                     ovl_redirect_follow_mode(0);
> +                                     ovl_redirect_follow_mode(true, 0);
>  }
>
>  static const struct constant_table ovl_parameter_verity[] = {
> @@ -896,6 +899,9 @@ int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
>         struct ovl_opt_set set = ctx->set;
>         /* The requested redirect_dir=, for reporting conflicts with it */
>         int redirect_opt = config->redirect_mode;
> +       /* The redirect follow mode for redirect_dir=off and for the default */
> +       int follow_mode = ovl_redirect_follow_mode(!config->userxattr,
> +                                                  ctx->nr_data);
>
>         /* Workdir/index are useless in non-upper mount */
>         if (!config->upperdir) {
> @@ -922,9 +928,12 @@ int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
>                 config->uuid = OVL_UUID_NULL;
>         }
>
> -       /* Resolve redirect_dir=off to the default redirect follow mode */
> +       /*
> +        * Resolve redirect_dir=off to the default redirect follow mode.
> +        * With userxattr, redirects are not followed to lower by default.
> +        */
>         if (config->redirect_mode == OVL_REDIRECT_OFF)
> -               config->redirect_mode = ovl_redirect_follow_mode(ctx->nr_data);
> +               config->redirect_mode = follow_mode;
>
>         /*
>          * This is to make the logic below simpler.  It doesn't make any other
> @@ -1038,7 +1047,7 @@ int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
>                  * userxattr.
>                  */
>                 if (!set.redirect)
> -                       config->redirect_mode =
> ovl_redirect_nofollow_mode(ctx->nr_data);
> +                       config->redirect_mode = follow_mode;
>                 config->metacopy = false;
>         }
>
>
> Please amend the test and test with the latest ovl-fixes:
> d0d57557b76de ovl: follow lower to lowerdata redirects from any lower layer

I've tested this modified version and it works fine.  Do you want me to
send a v3 or will you amend it?

Thanks,
Giuseppe