Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 22 Mar 2023 08:47:41 +0800
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
Message-ID: <ZBpQLQtZP3Gj8MdS@ovpn-8-18.pek2.redhat.com>
References: <20230317202749.419094-1-eblake@redhat.com>
 <20230317202749.419094-3-eblake@redhat.com>
 <ZBjqQckL7d5EJPlh@ovpn-8-29.pek2.redhat.com>
 <20230321135900.ni4w5ichvjba7s4u@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230321135900.ni4w5ichvjba7s4u@redhat.com>
Precedence: bulk

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