linux-kernel - Re: [PATCH 2/3] uapi nbd: add cookie alias to handle

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]

Date:   Tue, 14 Mar 2023 14:50:23 -0500
From:   Eric Blake <eblake@...hat.com>
To:     Nir Soffer <nsoffer@...hat.com>
Cc:     josef@...icpanda.com, linux-block@...r.kernel.org,
        nbd@...er.debian.org, philipp.reisner@...bit.com,
        lars.ellenberg@...bit.com, christoph.boehmwalder@...bit.com,
        corbet@....net, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] uapi nbd: add cookie alias to handle

On Sat, Mar 11, 2023 at 02:30:39PM +0200, Nir Soffer wrote:
> On Fri, Mar 10, 2023 at 10:16 PM Eric Blake <eblake@...hat.com> wrote:
> >
> > The uapi <linux/nbd.h> header declares a 'char handle[8]' per request;
> > which is overloaded in English (are you referring to "handle" the
> > verb, such as handling a signal or writing a callback handler, or
> > "handle" the noun, the value used in a lookup table to correlate a
> > response back to the request).  Many client-side NBD implementations
> > (both servers and clients) have instead used 'u64 cookie' or similar,
> > as it is easier to directly assign an integer than to futz around with
> > memcpy.  In fact, upstream documentation is now encouraging this shift
> > in terminology: https://lists.debian.org/nbd/2023/03/msg00031.html
> >
> > Accomplish this by use of an anonymous union to provide the alias for
> > anyone getting the definition from the uapi; this does not break
> > existing clients, while exposing the nicer name for those who prefer
> > it.  Note that block/nbd.c still uses the term handle (in fact, it
> > actually combines a 32-bit cookie and a 32-bit tag into the 64-bit
> > handle), but that internal usage is not changed the public uapi, since
> > no compliant NBD server has any reason to inspect or alter the 64
> > bits sent over the socket.
> >
> > Signed-off-by: Eric Blake <eblake@...hat.com>
> > ---
> >  include/uapi/linux/nbd.h | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/uapi/linux/nbd.h b/include/uapi/linux/nbd.h
> > index 8797387caaf7..f58f2043f62e 100644
> > --- a/include/uapi/linux/nbd.h
> > +++ b/include/uapi/linux/nbd.h
> > @@ -81,7 +81,10 @@ enum {
> >  struct nbd_request {
> >         __be32 magic;   /* NBD_REQUEST_MAGIC    */
> >         __be32 type;    /* See NBD_CMD_*        */
> > -       char handle[8];
> > +       union {
> > +               char handle[8];
> > +               __be64 cookie;
> > +       };
> >         __be64 from;
> >         __be32 len;
> >  } __attribute__((packed));
> > @@ -93,6 +96,9 @@ struct nbd_request {
> >  struct nbd_reply {
> >         __be32 magic;           /* NBD_REPLY_MAGIC      */
> >         __be32 error;           /* 0 = ok, else error   */
> > -       char handle[8];         /* handle you got from request  */
> > +       union {
> > +               char handle[8]; /* handle you got from request  */
> > +               __be64 cookie;
> 
> Should we document like this?
> 
>     union {
>         __be64 cookie; /* cookie you got from request */
>         char handle[8]; /* older name */
> 
> I think we want future code to use the new term.

Sure, swapping the order to favor the preferred name first makes sense.

I'm still not sure on whether cookie should be u64 or __be64 (it's
opaque, so endianness over the wire doesn't matter; and previous code
was using memcpy() onto char[8] which may behave differently depending
on machine endianness).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org