linux-kernel - Re: [PATCH 04/20] ublk: add integrity UAPI

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADUfDZp5Or_Q+7HKtdi97n5kBpQ=zpOFAtqfatR3nu+=yGLb_Q@mail.gmail.com>
Date: Mon, 22 Dec 2025 10:09:50 -0500
From: Caleb Sander Mateos <csander@...estorage.com>
To: Ming Lei <ming.lei@...hat.com>
Cc: Jens Axboe <axboe@...nel.dk>, Shuah Khan <shuah@...nel.org>, linux-block@...r.kernel.org, 
	linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Stanley Zhang <stazhang@...estorage.com>, Uday Shankar <ushankar@...estorage.com>
Subject: Re: [PATCH 04/20] ublk: add integrity UAPI

On Mon, Dec 22, 2025 at 9:26 AM Ming Lei <ming.lei@...hat.com> wrote:
>
> On Tue, Dec 16, 2025 at 10:34:38PM -0700, Caleb Sander Mateos wrote:
> > From: Stanley Zhang <stazhang@...estorage.com>
> >
> > Add UAPI definitions for metadata/integrity support in ublk.
> > UBLK_PARAM_TYPE_INTEGRITY and struct ublk_param_integrity allow a ublk
> > server to specify the integrity params of a ublk device.
> > The ublk driver will set UBLK_IO_F_INTEGRITY in the op_flags field of
> > struct ublksrv_io_desc for requests with integrity data.
> > The ublk server uses user copy with UBLKSRV_IO_INTEGRITY_FLAG set in the
> > offset parameter to access a request's integrity buffer.
> >
> > Signed-off-by: Stanley Zhang <stazhang@...estorage.com>
> > [csander: drop feature flag and redundant pi_tuple_size field,
> >  add io_desc flag, use block metadata UAPI constants]
> > Signed-off-by: Caleb Sander Mateos <csander@...estorage.com>
> > ---
> >  include/uapi/linux/ublk_cmd.h | 20 +++++++++++++++++++-
> >  1 file changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
> > index ec77dabba45b..5bfb9a0521c3 100644
> > --- a/include/uapi/linux/ublk_cmd.h
> > +++ b/include/uapi/linux/ublk_cmd.h
> > @@ -129,11 +129,15 @@
> >  #define UBLK_QID_BITS                12
> >  #define UBLK_QID_BITS_MASK   ((1ULL << UBLK_QID_BITS) - 1)
> >
> >  #define UBLK_MAX_NR_QUEUES   (1U << UBLK_QID_BITS)
> >
> > -#define UBLKSRV_IO_BUF_TOTAL_BITS    (UBLK_QID_OFF + UBLK_QID_BITS)
> > +/* Copy to/from request integrity buffer instead of data buffer */
> > +#define UBLK_INTEGRITY_FLAG_OFF              (UBLK_QID_OFF + UBLK_QID_BITS)
> > +#define UBLKSRV_IO_INTEGRITY_FLAG    (1ULL << UBLK_INTEGRITY_FLAG_OFF)
> > +
>
> I feel it is more readable to move the definition into the patch which uses
> them.

Sure, I can do that.

>
> > +#define UBLKSRV_IO_BUF_TOTAL_BITS    (UBLK_INTEGRITY_FLAG_OFF + 1)
>
> It is UAPI, UBLKSRV_IO_BUF_TOTAL_BITS shouldn't be changed, or can you
> explain this way is safe?

It's not clear to me how userspace is expected to use
UBLKSRV_IO_BUF_TOTAL_BITS. (Our ublk server, for one, doesn't use it.)
Can you provide an example? It looks to me like the purpose is to
communicate the number of bits needed to represent a user copy offset
value, in which case it makes sense to include the integrity flag now
that that bit is being used.

>
> >  #define UBLKSRV_IO_BUF_TOTAL_SIZE    (1ULL << UBLKSRV_IO_BUF_TOTAL_BITS)
> >
> >  /*
> >   * ublk server can register data buffers for incoming I/O requests with a sparse
> >   * io_uring buffer table. The request buffer can then be used as the data buffer
> > @@ -406,10 +410,12 @@ struct ublksrv_ctrl_dev_info {
> >   *
> >   * ublk server has to check this flag if UBLK_AUTO_BUF_REG_FALLBACK is
> >   * passed in.
> >   */
> >  #define              UBLK_IO_F_NEED_REG_BUF          (1U << 17)
> > +/* Request has an integrity data buffer */
> > +#define              UBLK_IO_F_INTEGRITY             (1U << 18)
> >
> >  /*
> >   * io cmd is described by this structure, and stored in share memory, indexed
> >   * by request tag.
> >   *
> > @@ -598,10 +604,20 @@ struct ublk_param_segment {
> >       __u32   max_segment_size;
> >       __u16   max_segments;
> >       __u8    pad[2];
> >  };
> >
> > +struct ublk_param_integrity {
> > +     __u32   flags; /* LBMD_PI_CAP_* from linux/fs.h */
> > +     __u8    interval_exp;
> > +     __u8    metadata_size;
> > +     __u8    pi_offset;
> > +     __u8    csum_type; /* LBMD_PI_CSUM_* from linux/fs.h */
> > +     __u8    tag_size;
> > +     __u8    pad[7];
> > +};
> > +
>
> Just be curious, `pi_tuple_size` isn't defined, instead it is hard-coded in
> ublk_integrity_pi_tuple_size().
>
> However, both scsi and nvme sets `pi_tuple_size`, so it means that ublk PI
> supports one `subset` or scsi/nvme `pi_tuple_size` can be removed too?

blk_validate_integrity_limits() validates that pi_tuple_size matches
the expected PI size for each csum_type value. So it looks like these
fields are redundant. Yes, pi_tuple_size could probably be removed
from the scsi/nvme block drivers too. But maybe there's value in
having the drivers explicitly specify both values?

Thanks,
Caleb