From: Ming Lei <ming.lei@redhat.com>
To: Eric Blake <eblake@redhat.com>
Cc: josef@toxicpanda.com, linux-block@vger.kernel.org,
nbd@other.debian.org, philipp.reisner@linbit.com,
lars.ellenberg@linbit.com, christoph.boehmwalder@linbit.com,
corbet@lwn.net, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, ming.lei@redhat.com
Subject: Re: [PATCH v2 2/5] block nbd: send handle in network order
Date: Wed, 22 Mar 2023 08:47:41 +0800 [thread overview]
Message-ID: <ZBpQLQtZP3Gj8MdS@ovpn-8-18.pek2.redhat.com> (raw)
In-Reply-To: <20230321135900.ni4w5ichvjba7s4u@redhat.com>
On Tue, Mar 21, 2023 at 08:59:00AM -0500, Eric Blake wrote:
> On Tue, Mar 21, 2023 at 07:20:33AM +0800, Ming Lei wrote:
> > On Fri, Mar 17, 2023 at 03:27:46PM -0500, Eric Blake wrote:
> > > The NBD spec says the client handle (or cookie) is opaque on the
> > > server, and therefore it really doesn't matter what endianness we use;
> > > to date, the use of memcpy() between u64 and a char[8] has exposed
> > > native endianness when treating the handle as a 64-bit number.
> >
> > No, memcpy() works fine for char[8], which doesn't break endianness.
>
> I didn't say memcpy() breaks endianness, I said it preserves it. By
> using memcpy(), you are exposing native endianness over the wire.
> Thus, even though a server should not be making any decisions based on
> the content of the handle (it is an opaque value handed back to the
> client unchanged), the current kernel client code DOES leak through
> information about whether the client is big- or little-endian;
How is the client cpu endianness leaked with handle defined as char[8]?
Suppose it is leaked, is it really one issue? Cause most of CPUs in
the world is little-endian.
> contrast to the NBD protocol saying that ALL data is
> network-byte-order.
That doesn't make sense for any data defined as char[] or byte which
needn't to be little or big endian.
>
> >
> > > However, since NBD protocol documents that everything else is in
> > > network order, and tools like Wireshark will dump even the contents of
> > > the handle as seen over the network, it's worth using a consistent
> > > ordering regardless of the native endianness.
> > >
> > > Plus, using a consistent endianness now allows an upcoming patch to
> > > simplify this to directly use integer assignment instead of memcpy().
> >
> > It isn't necessary, given ->handle is actually u64, which is handled by
> > nbd client only.
>
> No, re-read the whole series. ->handle is actually char[8]. Later in
> the series adds ->cookie as __be64 as an alias to ->handle, precisely
> so that we are converting the u64 'handle' in kernel code into a
> big-endian value on the wire, regardless of the host type, and making
> it impossible for a server to inspect the wire data and learn the
> kernel's endianness.
How does server learn the client cpu endianness in this way? Is it really
one issue?
>
> >
> > >
> > > Signed-off-by: Eric Blake <eblake@redhat.com>
> > >
> > > ---
> > > v2: new patch
> > > ---
> > > drivers/block/nbd.c | 10 +++++++---
> > > 1 file changed, 7 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> > > index 592cfa8b765a..8a9487e79f1c 100644
> > > --- a/drivers/block/nbd.c
> > > +++ b/drivers/block/nbd.c
> > > @@ -560,6 +560,7 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
> > > unsigned long size = blk_rq_bytes(req);
> > > struct bio *bio;
> > > u64 handle;
> > > + __be64 tmp;
> > > u32 type;
> > > u32 nbd_cmd_flags = 0;
> > > int sent = nsock->sent, skip = 0;
> > > @@ -606,7 +607,8 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
> > > request.len = htonl(size);
> > > }
> > > handle = nbd_cmd_handle(cmd);
> > > - memcpy(request.handle, &handle, sizeof(handle));
> > > + tmp = cpu_to_be64(handle);
> > > + memcpy(request.handle, &tmp, sizeof(tmp));
> >
> > This way copies handle two times, really not fun.
>
> Indeed. And as mentioned in the commit message, it is temporary; the
> second copy goes away later in the series once we can use direct
> integer assignment.
Then please merge with following patch, given it is hard to review
temporary change.
thanks,
Ming
next prev parent reply other threads:[~2023-03-22 0:48 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-17 20:27 [PATCH v2 0/5] nbd: s/handle/cookie/ Eric Blake
2023-03-17 20:27 ` [PATCH v2 1/5] uapi nbd: improve doc links to userspace spec Eric Blake
2023-03-17 20:27 ` [PATCH v2 2/5] block nbd: send handle in network order Eric Blake
2023-03-20 23:20 ` Ming Lei
2023-03-21 13:59 ` Eric Blake
2023-03-22 0:47 ` Ming Lei [this message]
2023-03-22 12:29 ` Eric Blake
2023-03-22 13:23 ` Ming Lei
2023-03-17 20:27 ` [PATCH v2 3/5] uapi nbd: add cookie alias to handle Eric Blake
2023-03-17 20:27 ` [PATCH v2 4/5] block nbd: use req.cookie instead of req.handle Eric Blake
2023-03-17 20:27 ` [PATCH v2 5/5] docs nbd: userspace NBD now favors github over sourceforge Eric Blake
2023-03-22 14:15 ` [PATCH v2 0/5] nbd: s/handle/cookie/ Josef Bacik
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=ZBpQLQtZP3Gj8MdS@ovpn-8-18.pek2.redhat.com \
--to=ming.lei@redhat.com \
--cc=christoph.boehmwalder@linbit.com \
--cc=corbet@lwn.net \
--cc=eblake@redhat.com \
--cc=josef@toxicpanda.com \
--cc=lars.ellenberg@linbit.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nbd@other.debian.org \
--cc=philipp.reisner@linbit.com \
/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).