From: Ming Lei <ming.lei@redhat.com>
To: Caleb Sander Mateos <csander@purestorage.com>
Cc: Shuah Khan <shuah@kernel.org>,
linux-block@vger.kernel.org, linux-kselftest@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 6/8] selftests: ublk: forbid multiple data copy modes
Date: Fri, 12 Dec 2025 19:30:06 +0800 [thread overview]
Message-ID: <aTv8vpxAapZCc6yq@fedora> (raw)
In-Reply-To: <CADUfDZp3oyFSwLhuAOi24quNFYK2CxLB0dPm5dRiB-AjmHbG=Q@mail.gmail.com>
On Thu, Dec 11, 2025 at 09:21:37PM -0800, Caleb Sander Mateos wrote:
> On Thu, Dec 11, 2025 at 9:14 PM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > On Thu, Dec 11, 2025 at 08:59:00PM -0800, Caleb Sander Mateos wrote:
> > > On Thu, Dec 11, 2025 at 6:38 PM Ming Lei <ming.lei@redhat.com> wrote:
> > > >
> > > > On Thu, Dec 11, 2025 at 06:06:51PM -0800, Caleb Sander Mateos wrote:
> > > > > On Thu, Dec 11, 2025 at 3:05 PM Ming Lei <ming.lei@redhat.com> wrote:
> > > > > >
> > > > > > On Thu, Dec 11, 2025 at 10:45:36AM -0800, Caleb Sander Mateos wrote:
> > > > > > > On Thu, Dec 11, 2025 at 1:09 AM Ming Lei <ming.lei@redhat.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@purestorage.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.
> > > > > >
> > > > > > Looks one regression from commit 0a9beafa7c63 ("ublk: refactor auto buffer register in ublk_dispatch_req()")
> > > > >
> > > > > Is there a particular issue you see in that commit? I think the issue
> > > >
> > > > Looks I overlooked.
> > > >
> > > > > is that if UBLK_IO_F_NEED_REG_BUF is set in the ublksrv_io_desc but zc
> > > > > isn't enabled, the null ublk server will just complete the I/O
> > > > > immediately. And --auto_zc_fallback isn't supported by any of the
> > > > > other ublk servers.
> > > > >
> > > > > if (auto_zc && !ublk_io_auto_zc_fallback(iod))
> > > > > queued = null_queue_auto_zc_io(t, q, tag);
> > > > > else if (zc)
> > > > > queued = null_queue_zc_io(t, q, tag);
> > > > > else {
> > > > > ublk_complete_io(t, q, tag, iod->nr_sectors << 9);
> > > > > return 0;
> > > > > }
> > > > >
> > > > > So it looks to me to just be an issue with my kublk change.
> > > >
> > > > But ublk_queue_auto_zc_fallback() is broken, so the auto_zc_fallback code
> > > > path isn't examined actually.
> > >
> > > How is it broken?
> >
> > typeof(q->flags) is u64, but the return type is i32, so it is overflowed.
>
> Ah, good catch. Yeah, these ublk_queue flag query functions should
> probably return bool. Are you going to send out a patch?
I guess you may have to send V3 for fixing patch 4, I appreciate you may include the
following fix in your V3, otherwise it may conflict with the added ublk_queue_use_user_copy().
From 62da81c7a9883dc251cc0daa2a3081226abc5da1 Mon Sep 17 00:00:00 2001
From: Ming Lei <ming.lei@redhat.com>
Date: Fri, 12 Dec 2025 11:23:56 +0000
Subject: [PATCH] selftests: ublk: fix overflow in
ublk_queue_auto_zc_fallback()
The functions ublk_queue_use_zc(), ublk_queue_use_auto_zc(), and
ublk_queue_auto_zc_fallback() were returning int, but performing
bitwise AND on q->flags which is __u64.
When a flag bit is set in the upper 32 bits (beyond INT_MAX), the
result of the bitwise AND operation could overflow when cast to int,
leading to incorrect boolean evaluation.
For example, if UBLKS_Q_AUTO_BUF_REG_FALLBACK is 0x8000000000000000:
- (u64)flags & 0x8000000000000000 = 0x8000000000000000
- Cast to int: undefined behavior / incorrect value
- Used in if(): may evaluate incorrectly
Fix by:
1. Changing return type from int to bool for semantic correctness
2. Using !! to explicitly convert to boolean (0 or 1)
This ensures the functions return proper boolean values regardless
of which bit position the flags occupy in the 64-bit field.
Fixes: c3a6d48f86da ("selftests: ublk: remove ublk queue self-defined flags")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
tools/testing/selftests/ublk/kublk.h | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/ublk/kublk.h b/tools/testing/selftests/ublk/kublk.h
index fda72e19ef09..8a83b90ec603 100644
--- a/tools/testing/selftests/ublk/kublk.h
+++ b/tools/testing/selftests/ublk/kublk.h
@@ -396,19 +396,19 @@ static inline int ublk_completed_tgt_io(struct ublk_thread *t,
return --io->tgt_ios == 0;
}
-static inline int ublk_queue_use_zc(const struct ublk_queue *q)
+static inline bool ublk_queue_use_zc(const struct ublk_queue *q)
{
- return q->flags & UBLK_F_SUPPORT_ZERO_COPY;
+ return !!(q->flags & UBLK_F_SUPPORT_ZERO_COPY);
}
-static inline int ublk_queue_use_auto_zc(const struct ublk_queue *q)
+static inline bool ublk_queue_use_auto_zc(const struct ublk_queue *q)
{
- return q->flags & UBLK_F_AUTO_BUF_REG;
+ return !!(q->flags & UBLK_F_AUTO_BUF_REG);
}
-static inline int ublk_queue_auto_zc_fallback(const struct ublk_queue *q)
+static inline bool ublk_queue_auto_zc_fallback(const struct ublk_queue *q)
{
- return q->flags & UBLKS_Q_AUTO_BUF_REG_FALLBACK;
+ return !!(q->flags & UBLKS_Q_AUTO_BUF_REG_FALLBACK);
}
static inline bool ublk_queue_use_user_copy(const struct ublk_queue *q)
--
2.47.1
Thanks,
Ming
next prev parent reply other threads:[~2025-12-12 11:30 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-12-11 5:15 [PATCH 0/8] selftests: ublk: add user copy test cases Caleb Sander Mateos
2025-12-11 5:15 ` [PATCH 1/8] selftests: ublk: correct last_rw map type in seq_io.bt Caleb Sander Mateos
2025-12-11 8:58 ` Ming Lei
2025-12-11 5:15 ` [PATCH 2/8] selftests: ublk: remove unused ios map " Caleb Sander Mateos
2025-12-11 8:58 ` Ming Lei
2025-12-11 5:15 ` [PATCH 3/8] selftests: ublk: fix fio arguments in run_io_and_recover() Caleb Sander Mateos
2025-12-11 9:03 ` Ming Lei
2025-12-11 5:15 ` [PATCH 4/8] selftests: ublk: use auto_zc for PER_IO_DAEMON tests in stress_04 Caleb Sander Mateos
2025-12-11 9:06 ` Ming Lei
2025-12-11 18:33 ` Caleb Sander Mateos
2025-12-11 22:30 ` Ming Lei
2025-12-12 2:57 ` Caleb Sander Mateos
2025-12-12 3:06 ` Ming Lei
2025-12-11 5:16 ` [PATCH 5/8] selftests: ublk: don't share backing files between ublk servers Caleb Sander Mateos
2025-12-11 9:07 ` Ming Lei
2025-12-11 5:16 ` [PATCH 6/8] selftests: ublk: forbid multiple data copy modes Caleb Sander Mateos
2025-12-11 9:09 ` Ming Lei
2025-12-11 18:45 ` Caleb Sander Mateos
2025-12-11 23:04 ` Ming Lei
2025-12-12 2:06 ` Caleb Sander Mateos
2025-12-12 2:38 ` Ming Lei
2025-12-12 4:59 ` Caleb Sander Mateos
2025-12-12 5:13 ` Ming Lei
2025-12-12 5:21 ` Caleb Sander Mateos
2025-12-12 11:30 ` Ming Lei [this message]
2025-12-12 17:02 ` Caleb Sander Mateos
2025-12-11 5:16 ` [PATCH 7/8] selftests: ublk: add support for user copy to kublk Caleb Sander Mateos
2025-12-11 5:16 ` [PATCH 8/8] selftests: ublk: add user copy test cases Caleb Sander Mateos
2025-12-11 9:17 ` Ming Lei
2025-12-11 18:46 ` Caleb Sander Mateos
2025-12-11 22:35 ` Ming Lei
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aTv8vpxAapZCc6yq@fedora \
--to=ming.lei@redhat.com \
--cc=csander@purestorage.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=shuah@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).