| 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
| ||
|
Message-ID: <CADUfDZpX3RTu4m5WZ1LrjnFRxg96qpeM0fMtw1-c=7Qn_5gKQQ@mail.gmail.com>
Date: Thu, 11 Dec 2025 10:45:36 -0800
From: Caleb Sander Mateos <csander@...estorage.com>
To: Ming Lei <ming.lei@...hat.com>
Cc: Shuah Khan <shuah@...nel.org>, linux-block@...r.kernel.org,
linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/8] selftests: ublk: forbid multiple data copy modes
On Thu, Dec 11, 2025 at 1:09 AM Ming Lei <ming.lei@...hat.com> wrote:
>
> On Wed, Dec 10, 2025 at 10:16:01PM -0700, Caleb Sander Mateos wrote:
> > The kublk mock ublk server allows multiple data copy mode arguments to
> > be passed on the command line (--zero_copy, --get_data, and --auto_zc).
> > The ublk device will be created with all the requested feature flags,
> > however kublk will only use one of the modes to interact with request
> > data (arbitrarily preferring auto_zc over zero_copy over get_data). To
> > clarify the intent of the test, don't allow multiple data copy modes to
> > be specified. Don't set UBLK_F_USER_COPY for zero_copy, as it's an
> > independent feature. Don't require zero_copy for auto_zc_fallback, as
> > only auto_zc is needed. Fix the test cases passing multiple data copy
> > mode arguments.
> >
> > Signed-off-by: Caleb Sander Mateos <csander@...estorage.com>
> > ---
> > tools/testing/selftests/ublk/kublk.c | 21 ++++++++++++-------
> > .../testing/selftests/ublk/test_generic_09.sh | 2 +-
> > .../testing/selftests/ublk/test_stress_03.sh | 4 ++--
> > .../testing/selftests/ublk/test_stress_04.sh | 2 +-
> > .../testing/selftests/ublk/test_stress_05.sh | 10 ++++-----
> > 5 files changed, 22 insertions(+), 17 deletions(-)
> >
> > diff --git a/tools/testing/selftests/ublk/kublk.c b/tools/testing/selftests/ublk/kublk.c
> > index f8fa102a627f..1765c4806523 100644
> > --- a/tools/testing/selftests/ublk/kublk.c
> > +++ b/tools/testing/selftests/ublk/kublk.c
> > @@ -1611,11 +1611,11 @@ int main(int argc, char *argv[])
> > break;
> > case 'd':
> > ctx.queue_depth = strtol(optarg, NULL, 10);
> > break;
> > case 'z':
> > - ctx.flags |= UBLK_F_SUPPORT_ZERO_COPY | UBLK_F_USER_COPY;
> > + ctx.flags |= UBLK_F_SUPPORT_ZERO_COPY;
> > break;
> > case 'r':
> > value = strtol(optarg, NULL, 10);
> > if (value)
> > ctx.flags |= UBLK_F_USER_RECOVERY;
> > @@ -1674,17 +1674,22 @@ int main(int argc, char *argv[])
> > optind += 1;
> > break;
> > }
> > }
> >
> > - /* auto_zc_fallback depends on F_AUTO_BUF_REG & F_SUPPORT_ZERO_COPY */
> > - if (ctx.auto_zc_fallback &&
> > - !((ctx.flags & UBLK_F_AUTO_BUF_REG) &&
> > - (ctx.flags & UBLK_F_SUPPORT_ZERO_COPY))) {
> > - ublk_err("%s: auto_zc_fallback is set but neither "
> > - "F_AUTO_BUF_REG nor F_SUPPORT_ZERO_COPY is enabled\n",
> > - __func__);
> > + /* auto_zc_fallback depends on F_AUTO_BUF_REG */
> > + if (ctx.auto_zc_fallback && !(ctx.flags & UBLK_F_AUTO_BUF_REG)) {
> > + ublk_err("%s: auto_zc_fallback is set but F_AUTO_BUF_REG is disabled\n",
> > + __func__);
> > + return -EINVAL;
> > + }
> > +
> > + if (!!(ctx.flags & UBLK_F_SUPPORT_ZERO_COPY) +
> > + !!(ctx.flags & UBLK_F_NEED_GET_DATA) +
> > + !!(ctx.flags & UBLK_F_USER_COPY) +
> > + !!(ctx.flags & UBLK_F_AUTO_BUF_REG) > 1) {
> > + fprintf(stderr, "too many data copy modes specified\n");
> > return -EINVAL;
> > }
>
> Actually most of them are allowed to co-exist, such as -z/--auto_zc/-u.
Yes, I know the ublk driver allows multiple copy mode flags to be set
(though it will clear UBLK_F_NEED_GET_DATA if any of the others are
set). However, kublk will only actually use one of the modes. For
example, --get_data --zero_copy will use zero copy for the data
transfer, not get data. And --zero_copy --auto_zc will only use auto
buffer registration. So I think it's confusing to allow multiple of
these parameters to be passed to kublk. Or do you think there is value
in testing ublk device creation with multiple data copy mode flags
set, but only one of the modes actually used?
>
> >
> > i = optind;
> > while (i < argc && ctx.nr_files < MAX_BACK_FILES) {
> > diff --git a/tools/testing/selftests/ublk/test_generic_09.sh b/tools/testing/selftests/ublk/test_generic_09.sh
> > index bb6f77ca5522..145e17b3d2b0 100755
> > --- a/tools/testing/selftests/ublk/test_generic_09.sh
> > +++ b/tools/testing/selftests/ublk/test_generic_09.sh
> > @@ -14,11 +14,11 @@ if ! _have_program fio; then
> > exit "$UBLK_SKIP_CODE"
> > fi
> >
> > _prep_test "null" "basic IO test"
> >
> > -dev_id=$(_add_ublk_dev -t null -z --auto_zc --auto_zc_fallback)
> > +dev_id=$(_add_ublk_dev -t null --auto_zc --auto_zc_fallback)
> > _check_add_dev $TID $?
> >
> > # run fio over the two disks
> > fio --name=job1 --filename=/dev/ublkb"${dev_id}" --ioengine=libaio --rw=readwrite --iodepth=32 --size=256M > /dev/null 2>&1
> > ERR_CODE=$?
> > diff --git a/tools/testing/selftests/ublk/test_stress_03.sh b/tools/testing/selftests/ublk/test_stress_03.sh
> > index 3ed4c9b2d8c0..8e9f2786ef9c 100755
> > --- a/tools/testing/selftests/ublk/test_stress_03.sh
> > +++ b/tools/testing/selftests/ublk/test_stress_03.sh
> > @@ -36,19 +36,19 @@ wait
> >
> > if _have_feature "AUTO_BUF_REG"; then
> > ublk_io_and_remove 8G -t null -q 4 --auto_zc &
> > ublk_io_and_remove 256M -t loop -q 4 --auto_zc "${UBLK_BACKFILES[0]}" &
> > ublk_io_and_remove 256M -t stripe -q 4 --auto_zc "${UBLK_BACKFILES[1]}" "${UBLK_BACKFILES[2]}" &
> > - ublk_io_and_remove 8G -t null -q 4 -z --auto_zc --auto_zc_fallback &
> > + ublk_io_and_remove 8G -t null -q 4 --auto_zc --auto_zc_fallback &
> > wait
> > fi
> >
> > if _have_feature "PER_IO_DAEMON"; then
> > ublk_io_and_remove 8G -t null -q 4 --auto_zc --nthreads 8 --per_io_tasks &
> > ublk_io_and_remove 256M -t loop -q 4 --auto_zc --nthreads 8 --per_io_tasks "${UBLK_BACKFILES[0]}" &
> > ublk_io_and_remove 256M -t stripe -q 4 --auto_zc --nthreads 8 --per_io_tasks "${UBLK_BACKFILES[1]}" "${UBLK_BACKFILES[2]}" &
> > - ublk_io_and_remove 8G -t null -q 4 -z --auto_zc --auto_zc_fallback --nthreads 8 --per_io_tasks &
> > + ublk_io_and_remove 8G -t null -q 4 --auto_zc --auto_zc_fallback --nthreads 8 --per_io_tasks &
> > wait
> > fi
> >
> > _cleanup_test "stress"
> > _show_result $TID $ERR_CODE
> > diff --git a/tools/testing/selftests/ublk/test_stress_04.sh b/tools/testing/selftests/ublk/test_stress_04.sh
> > index c7220723b537..6e165a1f90b4 100755
> > --- a/tools/testing/selftests/ublk/test_stress_04.sh
> > +++ b/tools/testing/selftests/ublk/test_stress_04.sh
> > @@ -35,11 +35,11 @@ wait
> >
> > if _have_feature "AUTO_BUF_REG"; then
> > ublk_io_and_kill_daemon 8G -t null -q 4 --auto_zc &
> > ublk_io_and_kill_daemon 256M -t loop -q 4 --auto_zc "${UBLK_BACKFILES[0]}" &
> > ublk_io_and_kill_daemon 256M -t stripe -q 4 --auto_zc --no_ublk_fixed_fd "${UBLK_BACKFILES[1]}" "${UBLK_BACKFILES[2]}" &
> > - ublk_io_and_kill_daemon 8G -t null -q 4 -z --auto_zc --auto_zc_fallback &
> > + ublk_io_and_kill_daemon 8G -t null -q 4 --auto_zc --auto_zc_fallback &
> > wait
> > fi
> >
> > if _have_feature "PER_IO_DAEMON"; then
> > ublk_io_and_kill_daemon 8G -t null -q 4 --auto_zc --nthreads 8 --per_io_tasks &
> > diff --git a/tools/testing/selftests/ublk/test_stress_05.sh b/tools/testing/selftests/ublk/test_stress_05.sh
> > index 274295061042..09b94c36f2ba 100755
> > --- a/tools/testing/selftests/ublk/test_stress_05.sh
> > +++ b/tools/testing/selftests/ublk/test_stress_05.sh
> > @@ -56,21 +56,21 @@ for reissue in $(seq 0 1); do
> > wait
> > done
> >
> > if _have_feature "ZERO_COPY"; then
> > for reissue in $(seq 0 1); do
> > - ublk_io_and_remove 8G -t null -q 4 -g -z -r 1 -i "$reissue" &
> > - ublk_io_and_remove 256M -t loop -q 4 -g -z -r 1 -i "$reissue" "${UBLK_BACKFILES[1]}" &
> > + ublk_io_and_remove 8G -t null -q 4 -z -r 1 -i "$reissue" &
> > + ublk_io_and_remove 256M -t loop -q 4 -z -r 1 -i "$reissue" "${UBLK_BACKFILES[1]}" &
> > wait
> > done
> > fi
> >
> > if _have_feature "AUTO_BUF_REG"; then
> > for reissue in $(seq 0 1); do
> > - ublk_io_and_remove 8G -t null -q 4 -g --auto_zc -r 1 -i "$reissue" &
> > - ublk_io_and_remove 256M -t loop -q 4 -g --auto_zc -r 1 -i "$reissue" "${UBLK_BACKFILES[1]}" &
> > - ublk_io_and_remove 8G -t null -q 4 -g -z --auto_zc --auto_zc_fallback -r 1 -i "$reissue" &
> > + ublk_io_and_remove 8G -t null -q 4 --auto_zc -r 1 -i "$reissue" &
> > + ublk_io_and_remove 256M -t loop -q 4 --auto_zc -r 1 -i "$reissue" "${UBLK_BACKFILES[1]}" &
> > + ublk_io_and_remove 8G -t null -q 4 --auto_zc --auto_zc_fallback -r 1 -i "$reissue" &
>
> --auto_zc_fallback requires both -z and --auto_zc.
Ah, right, I forgot that the fallback path relies on normal zero copy
buffer registration. I guess we are missing coverage of that, then,
since the tests still passed with --zero_copy disabled.
Thanks,
Caleb
Powered by blists - more mailing lists