nbd: Use uint64_t instead of char[8] for cookie

Type-punning *(uint64_t*)(char[8]) is unsafe if the pointer is not
aligned per the requirements of the hardware (x86_64 has 1-byte
alignment, but other hardware exists that will give a SIGBUS).  In
practice, it didn't matter - 'struct nbd_request' was already 64-bit
aligned for 'from' (even before the recent change in commit 739f712
to pack it), and 'struct nbd_reply' being packed means the compiler
emits code to deal with 1-byte alignment despite hardware.  But since
the cookie is already opaque on the server side, we might as well
treat it as an 8-byte integer instead of a character array, with no
visible semantic changes to the client.

This patch does NOT perform endian swapping on the value.  As long as
we are consistent (no swap when reading from the client, no swap when
writing back to the client), the client sees the same value.  If we
were to log the cookie at any point in our local handling, then it
would be nice to obey the spec that all NBD values sent over the wire
are in network byte order (big-endian), so that what we log appears as
a 64-bit value with endianness matching whatever wireshark or the
client might be logging as well.  But since we aren't logging the
cookie, skipping the byte-swaps doesn't hurt.

Signed-off-by: Eric Blake <[email protected]>
Message-Id: <[email protected]>

Author: ebblake

nbd-server.c

@@ -1274,7 +1274,7 @@ int rawexpwrite_fully(off_t a, char *buf, size_t len, CLIENT *client, int fua) {
 static void setup_reply(struct nbd_reply* rep, struct nbd_request* req) {
 	rep->magic = htonl(NBD_REPLY_MAGIC);
 	rep->error = 0;
-	memcpy(&(rep->cookie), &(req->cookie), sizeof(req->cookie));
+	rep->cookie = req->cookie;
 }
 
 static void log_reply(CLIENT *client, struct nbd_reply *prply) {
@@ -1298,7 +1298,7 @@ void send_structured_chunk(CLIENT *client, struct nbd_request *req, uint16_t fla
 	rep.magic = htonl(NBD_STRUCTURED_REPLY_MAGIC);
 	rep.flags = htons(flags);
 	rep.type = htons(type);
-	memcpy(&(rep.cookie), req->cookie, sizeof(rep.cookie));
+	rep.cookie = req->cookie;
 	rep.paylen = htonl(length);
 	pthread_mutex_lock(&(client->lock));
 	socket_write(client, &rep, sizeof rep);
@@ -1315,7 +1315,7 @@ void send_structured_chunk_v(CLIENT *client, struct nbd_request *req, uint16_t f
 	rep.magic = htonl(NBD_STRUCTURED_REPLY_MAGIC);
 	rep.flags = htons(flags);
 	rep.type = htons(type);
-	memcpy(&(rep.cookie), req->cookie, sizeof(rep.cookie));
+	rep.cookie = req->cookie;
 	rep.paylen = htonl(length);
 	va_start(ap, bufcount);
 	pthread_mutex_lock(&(client->lock));
@@ -2223,7 +2223,7 @@ static void setup_transactionlog(CLIENT *client) {
 
 		req.magic = htonl(NBD_TRACELOG_MAGIC);
 		req.type = htonl(NBD_TRACELOG_SET_DATALOG);
-		memset(req.cookie, 0, sizeof(req.cookie));
+		req.cookie = 0;
 		req.from = htonll(NBD_TRACELOG_FROM_MAGIC);
 		req.len = htonl(TRUE);
 

nbd-trdump.c

@@ -70,7 +70,7 @@ int main(int argc, char**argv) {
 		switch (magic) {
 		case NBD_REQUEST_MAGIC:
 			doread(readfd, sizeof(magic)+(char *)(&req), sizeof(struct nbd_request)-sizeof(magic));
-			cookie = ntohll(*((long long int *)(req.cookie)));
+			cookie = ntohll(req.cookie);
 			offset = ntohll(req.from);
 			len = ntohl(req.len);
 			command = ntohl(req.type);
@@ -99,7 +99,7 @@ int main(int argc, char**argv) {
 			break;
 		case NBD_REPLY_MAGIC:
 			doread(readfd, sizeof(magic)+(char *)(&rep), sizeof(struct nbd_reply)-sizeof(magic));
-			cookie = ntohll(*((long long int *)(rep.cookie)));
+			cookie = ntohll(rep.cookie);
 			error = ntohl(rep.error);
 
 			printf("< H=%016llx E=0x%08x\n",
@@ -109,7 +109,7 @@ int main(int argc, char**argv) {
 
 		case NBD_TRACELOG_MAGIC:
 			doread(readfd, sizeof(magic)+(char *)(&req), sizeof(struct nbd_request)-sizeof(magic));
-			cookie = ntohll(*((long long int *)(req.cookie)));
+			cookie = ntohll(req.cookie);
 			offset = ntohll(req.from);
 			len = ntohl(req.len);
 			command = ntohl(req.type);

nbd-trplay.c

@@ -163,7 +163,7 @@ int main_loop(int logfd, int imagefd) {
 		switch (magic) {
 		case NBD_REQUEST_MAGIC:
 			doread(logfd, sizeof(magic)+(char *)(&req), sizeof(struct nbd_request)-sizeof(magic));
-			cookie = ntohll(*((long long int *)(req.cookie)));
+			cookie = ntohll(req.cookie);
 			offset = ntohll(req.from);
 			len = ntohl(req.len);
 			command = ntohl(req.type);
@@ -185,7 +185,7 @@ int main_loop(int logfd, int imagefd) {
 
 		case NBD_REPLY_MAGIC:
 			doread(logfd, sizeof(magic)+(char *)(&rep), sizeof(struct nbd_reply)-sizeof(magic));
-			cookie = ntohll(*((long long int *)(rep.cookie)));
+			cookie = ntohll(rep.cookie);
 			error = ntohl(rep.error);
 
 			if (g_verbose >= VERBOSE_NORMAL) {
@@ -197,7 +197,7 @@ int main_loop(int logfd, int imagefd) {
 
 		case NBD_TRACELOG_MAGIC:
 			doread(logfd, sizeof(magic)+(char *)(&req), sizeof(struct nbd_request)-sizeof(magic));
-			cookie = ntohll(*((long long int *)(req.cookie)));
+			cookie = ntohll(req.cookie);
 			offset = ntohll(req.from);
 			len = ntohl(req.len);
 			command = ntohl(req.type);

nbd.h

@@ -90,7 +90,7 @@ enum {
 struct nbd_request {
 	uint32_t magic;
 	uint32_t type;	/* == READ || == WRITE 	*/
-	char cookie[8];
+	uint64_t cookie;
 	uint64_t from;
 	uint32_t len;
 } __attribute__ ((packed));
@@ -102,7 +102,7 @@ struct nbd_request {
 struct nbd_reply {
 	uint32_t magic;
 	uint32_t error;		/* 0 = ok, else error	*/
-	char cookie[8];		/* cookie you got from request	*/
+	uint64_t cookie;	/* cookie you got from request	*/
 } __attribute__ ((packed));
 
 /*

tests/run/nbd-tester-client.c

@@ -74,7 +74,7 @@ typedef enum {
 
 struct reqcontext {
 	uint64_t seq;
-	char origcookie[8];
+	uint64_t origcookie;
 	struct nbd_request req;
 	struct reqcontext *next;
 	struct reqcontext *prev;
@@ -680,7 +680,7 @@ int close_connection(int sock, CLOSE_TYPE type)
 	case CONNECTION_CLOSE_PROPERLY:
 		req.magic = htonl(NBD_REQUEST_MAGIC);
 		req.type = htonl(NBD_CMD_DISC);
-		memcpy(&(req.cookie), &(counter), sizeof(counter));
+		req.cookie = htonll(counter);
 		counter++;
 		req.from = 0;
 		req.len = 0;
@@ -720,8 +720,8 @@ int read_packet_check_header(int sock, size_t datasize, long long int curcookie)
 			 "Received package with incorrect reply_magic. Index of sent packages is %lld (0x%llX), received cookie is %lld (0x%llX). Received magic 0x%lX, expected 0x%lX",
 			 (long long int)curcookie,
 			 (long long unsigned int)curcookie,
-			 (long long int)*((u64 *) rep.cookie),
-			 (long long unsigned int)*((u64 *) rep.cookie),
+			 (long long int)rep.cookie,
+			 (long long unsigned int)rep.cookie,
 			 (long unsigned int)rep.magic,
 			 (long unsigned int)NBD_REPLY_MAGIC);
 		retval = -1;
@@ -731,8 +731,8 @@ int read_packet_check_header(int sock, size_t datasize, long long int curcookie)
 		snprintf(errstr, errstr_len,
 			 "Received error from server: %ld (0x%lX). Cookie is %lld (0x%llX).",
 			 (long int)rep.error, (long unsigned int)rep.error,
-			 (long long int)(*((u64 *) rep.cookie)),
-			 (long long unsigned int)*((u64 *) rep.cookie));
+			 (long long int)rep.cookie,
+			 (long long unsigned int)rep.cookie);
 		retval = -2;
 		goto end;
 	}
@@ -767,7 +767,7 @@ int oversize_test(char *name, int sock, char close_sock, int testflags)
 	req.magic = htonl(NBD_REQUEST_MAGIC);
 	req.type = htonl(NBD_CMD_READ);
 	req.len = htonl(1024 * 1024);
-	memcpy(&(req.cookie), &i, sizeof(i));
+	req.cookie = htonll(i);
 	req.from = htonll(i);
 	WRITE_ALL_ERR_RT(sock, &req, sizeof(req), err, -1,
 			 "Could not write request: %s", strerror(errno));
@@ -971,7 +971,7 @@ int throughput_test(char *name, int sock, char close_sock, int testflags)
 			if (sendfua)
 				req.type =
 				    htonl(NBD_CMD_WRITE | NBD_CMD_FLAG_FUA);
-			memcpy(&(req.cookie), &i, sizeof(i));
+			req.cookie = htonll(i);
 			req.from = htonll(i);
 			if (write_all(sock, &req, sizeof(req)) < 0) {
 				retval = -1;
@@ -987,7 +987,7 @@ int throughput_test(char *name, int sock, char close_sock, int testflags)
 			if (sendflush) {
 				long long int j = i ^ (1LL << 63);
 				req.type = htonl(NBD_CMD_FLUSH);
-				memcpy(&(req.cookie), &j, sizeof(j));
+				req.cookie = htonll(j);
 				req.from = 0;
 				req.len = 0;
 				if (write_all(sock, &req, sizeof(req)) < 0) {
@@ -1334,7 +1334,7 @@ int integrity_test(char *name, int sock, char close_sock, int testflags)
 						"Could not read transaction log: %s",
 						strerror(errno));
 				prc->req.magic = htonl(NBD_REQUEST_MAGIC);
-				memcpy(prc->origcookie, prc->req.cookie, 8);
+				prc->origcookie = htonll(prc->req.cookie);
 				prc->seq = seq++;
 				if ((ntohl(prc->req.type) &
 				     NBD_CMD_MASK_COMMAND) == NBD_CMD_DISC) {
@@ -1439,9 +1439,8 @@ int integrity_test(char *name, int sock, char close_sock, int testflags)
 
 				dumpcommand("Sending command", prc->req.type);
 				/* we rewrite the cookie as they otherwise may not be unique */
-				*((uint64_t *) (prc->req.cookie)) =
-				    getrandomcookie(cookiehash);
-				g_hash_table_insert(cookiehash, prc->req.cookie,
+				prc->req.cookie = getrandomcookie(cookiehash);
+				g_hash_table_insert(cookiehash, &prc->req.cookie,
 						    prc);
 				addbuffer(&txbuf, &(prc->req),
 					  sizeof(struct nbd_request));
@@ -1538,20 +1537,18 @@ int integrity_test(char *name, int sock, char close_sock, int testflags)
 			}
 
 			uint64_t cookie;
-			memcpy(&cookie, rep.cookie, 8);
+			cookie = rep.cookie;
 			prc = g_hash_table_lookup(cookiehash, &cookie);
 			if (!prc) {
 				snprintf(errstr, errstr_len,
 					 "Unrecognised cookie in reply: 0x%llX",
-					 *(long long unsigned int *)(rep.
-								     cookie));
+					 (long long unsigned int)rep.cookie);
 				goto err_open;
 			}
 			if (!g_hash_table_remove(cookiehash, &cookie)) {
 				snprintf(errstr, errstr_len,
 					 "Could not remove cookie from hash: 0x%llX",
-					 *(long long unsigned int *)(rep.
-								     cookie));
+					 (long long unsigned int)rep.cookie);
 				goto err_open;
 			}