diff mbox

[V9fs-developer] TREADDIR: issue when cookie has 64th bit set to 1

Message ID CAFkjPTmSru+o7_NNBDeSna5i8NQDFLU_b2MUpzwBuf2AgWNYJg@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Eric Van Hensbergen July 9, 2012, 2:31 p.m. UTC 
On Fri, Jul 6, 2012 at 8:47 AM, DENIEL Philippe <philippe.deniel@cea.fr> wrote:
> Hi,
>
> I faced a weird issue when running cthon04's test suite. Client was
> kernel 3.4.0 and server was my latest version of ganesha with 9p.2000L
> support.

Hi.  Thanks for the report, looking into this today.

> You have to know that ganesha uses AVL tree to store directory content.
> This means that readdir cookies are in fact keys to access the data in
> the AVL tree. This key is computed using the murmur3 algorith, which is
> pretty efficient but produces very "big" 64 bits integer (it uses of all
> the 64 available bits, including the last one). I use the AVL key as a
> readdir cookie in my implementation of TREADDIR. I first had an
> unexpected issue: the "telldir" step for the cthon04's special tests
> failed. While debugging, I realized that it happened when a TREADDIR was
> called with a starting cookie whose value had its 64th bit set to 1.

I think I'm having a bit of terminology mismatch in my head.  When you
say cookie, can you map that to the 9p protocol element?  It sounds like
maybe you are "overloading" the offset to be this cookie?

> Other cookies were OK. So I forced the 64 bits of my cookies to be 0 (it
> does not break the internal logic in ganesha) and everything was fixed.
> It seems to me that the kernel's 9P implementation does not like 64 bits
> long integer whose "sign bit" is set. I suspect something like a
> uint64_t managed as a int64_t. Since ganesha has very strange cookies, I
> guess I am probably the first one to use v9fs with cookie whose 64bit is
> set. I am afraid this is a bug.

Looking through the code, the neo-protocol code which I copied from
Anthony's original qemu 9p server uses int8_t, int16_t, int32_t, and
int64_t for all the protocol conversions.  Looking at the Plan 9 code,
it seems like most things are handled as uchar, ushort, u32int, and
vlongs (so only 64 bit is signed?).

A quick search/replace of relevant pieces of client and protocol to
convert everything to u8, u16, u32, u64 seems to pass my rudimentary
regressions.  I'm including the patch here for Daniel to test and
copying Anthony to find out what I probably just broke and jmk to get
the official Plan 9 read on if there is a good reason why the 64-bit
quantities are signed in struct Fcall, but all the other bits are
unsigned.

And of course I'll blame any stupidity on my part on it being early
Monday morning before coffee.

       -eric

----
Original protocol specs from Plan 9 show most protocol operations
as being unsigned.  Problems exposed when running with ganesha server
may be linked to signed coversion issues on the client, swap all protocol
ops back to unsigned.

Signed-off-by: Eric Van Hensbergen <ericvh@gmail.com>
---
 include/net/9p/client.h |    4 ++--
 net/9p/client.c         |   30 +++++++++++++--------------
 net/9p/protocol.c       |   52 +++++++++++++++++++++++------------------------
 net/9p/protocol.h       |    2 +-
 net/9p/trans_rdma.c     |    2 +-
 5 files changed, 45 insertions(+), 45 deletions(-)

 	ib_dma_unmap_single(rdma->cm_id->device, c->busa, client->msize,

Comments

DENIEL Philippe July 9, 2012, 2:45 p.m. UTC | #1 
Hi Eric,
>
> I think I'm having a bit of terminology mismatch in my head.  When you
> say cookie, can you map that to the 9p protocol element?  It sounds like
> maybe you are "overloading" the offset to be this cookie?
>   
Sorry for the terminology mismatch. What I call "cookie" is the 
equivalent of the NFS readdir cookie in my server's md cache. I use this 
value as "offset" in 9p.2000L's RREADIR.

>
> Looking through the code, the neo-protocol code which I copied from
> Anthony's original qemu 9p server uses int8_t, int16_t, int32_t, and
> int64_t for all the protocol conversions.  Looking at the Plan 9 code,
> it seems like most things are handled as uchar, ushort, u32int, and
> vlongs (so only 64 bit is signed?).
>   
A "sign mismatch" may explain what I saw.

> A quick search/replace of relevant pieces of client and protocol to
> convert everything to u8, u16, u32, u64 seems to pass my rudimentary
> regressions.  I'm including the patch here for Daniel to test and
> copying Anthony to find out what I probably just broke and jmk to get
> the official Plan 9 read on if there is a good reason why the 64-bit
> quantities are signed in struct Fcall, but all the other bits are
> unsigned.
>
> And of course I'll blame any stupidity on my part on it being early
> Monday morning before coffee.
>   
Once you have a fix to give to me for testing, I am ready to proceed. I 
currently use kernel 3.4.0 from kernel.org.

    Regards

       Philippe

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
diff mbox

Patch

diff --git a/include/net/9p/client.h b/include/net/9p/client.h
index fc9b90b..16b1084 100644
--- a/include/net/9p/client.h
+++ b/include/net/9p/client.h
@@ -221,7 +221,7 @@  void p9_client_disconnect(struct p9_client *clnt);
 void p9_client_begin_disconnect(struct p9_client *clnt);
 struct p9_fid *p9_client_attach(struct p9_client *clnt, struct p9_fid *afid,
 					char *uname, u32 n_uname, char *aname);
-struct p9_fid *p9_client_walk(struct p9_fid *oldfid, uint16_t nwname,
+struct p9_fid *p9_client_walk(struct p9_fid *oldfid, u16 nwname,
 		char **wnames, int clone);
 int p9_client_open(struct p9_fid *fid, int mode);
 int p9_client_fcreate(struct p9_fid *fid, char *name, u32 perm, int mode,
@@ -258,7 +258,7 @@  int p9_client_getlock_dotl(struct p9_fid *fid,
struct p9_getlock *fl);
 struct p9_req_t *p9_tag_lookup(struct p9_client *, u16);
 void p9_client_cb(struct p9_client *c, struct p9_req_t *req);

-int p9_parse_header(struct p9_fcall *, int32_t *, int8_t *, int16_t *, int);
+int p9_parse_header(struct p9_fcall *, u32 *, u8 *, u16 *, int);
 int p9stat_read(struct p9_client *, char *, int, struct p9_wstat *);
 void p9stat_free(struct p9_wstat *);

diff --git a/net/9p/client.c b/net/9p/client.c
index a170893..93b76c1 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -411,12 +411,12 @@  EXPORT_SYMBOL(p9_client_cb);
  */

 int
-p9_parse_header(struct p9_fcall *pdu, int32_t *size, int8_t *type,
int16_t *tag,
+p9_parse_header(struct p9_fcall *pdu, u32 *size, u8 *type, u16 *tag,
 								int rewind)
 {
-	int8_t r_type;
-	int16_t r_tag;
-	int32_t r_size;
+	u8 r_type;
+	u16 r_tag;
+	u32 r_size;
 	int offset = pdu->offset;
 	int err;

@@ -463,9 +463,9 @@  EXPORT_SYMBOL(p9_parse_header);

 static int p9_check_errors(struct p9_client *c, struct p9_req_t *req)
 {
-	int8_t type;
+	u8 type;
 	int err;
-	int ecode;
+	uint ecode;

 	err = p9_parse_header(req->rc, NULL, &type, NULL, 0);
 	/*
@@ -529,7 +529,7 @@  static int p9_check_zc_errors(struct p9_client *c,
struct p9_req_t *req,
 {
 	int err;
 	int ecode;
-	int8_t type;
+	u8 type;
 	char *ename = NULL;

 	err = p9_parse_header(req->rc, NULL, &type, NULL, 0);
@@ -548,7 +548,7 @@  static int p9_check_zc_errors(struct p9_client *c,
struct p9_req_t *req,

 	if (!p9_is_proto_dotl(c)) {
 		/* Error is reported in string format */
-		uint16_t len;
+		u16 len;
 		/* 7 = header size for RERROR, 2 is the size of string len; */
 		int inline_len = in_hdrlen - (7 + 2);

@@ -622,7 +622,7 @@  out_err:
 }

 static struct p9_req_t *
-p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...);
+p9_client_rpc(struct p9_client *c, u8 type, const char *fmt, ...);

 /**
  * p9_client_flush - flush (cancel) a request
@@ -639,7 +639,7 @@  p9_client_rpc(struct p9_client *c, int8_t type,
const char *fmt, ...);
 static int p9_client_flush(struct p9_client *c, struct p9_req_t *oldreq)
 {
 	struct p9_req_t *req;
-	int16_t oldtag;
+	u16 oldtag;
 	int err;

 	err = p9_parse_header(oldreq->tc, NULL, NULL, &oldtag, 1);
@@ -665,7 +665,7 @@  static int p9_client_flush(struct p9_client *c,
struct p9_req_t *oldreq)
 }

 static struct p9_req_t *p9_client_prepare_req(struct p9_client *c,
-					      int8_t type, int req_size,
+					      u8 type, int req_size,
 					      const char *fmt, va_list ap)
 {
 	int tag, err;
@@ -715,7 +715,7 @@  reterr:
  */

 static struct p9_req_t *
-p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...)
+p9_client_rpc(struct p9_client *c, u8 type, const char *fmt, ...)
 {
 	va_list ap;
 	int sigpending, err;
@@ -798,7 +798,7 @@  reterr:
  *
  * Returns request structure (which client must free using p9_free_req)
  */
-static struct p9_req_t *p9_client_zc_rpc(struct p9_client *c, int8_t type,
+static struct p9_req_t *p9_client_zc_rpc(struct p9_client *c, u8 type,
 					 char *uidata, char *uodata,
 					 int inlen, int olen, int in_hdrlen,
 					 int kern_buf, const char *fmt, ...)
@@ -1132,7 +1132,7 @@  error:
 }
 EXPORT_SYMBOL(p9_client_attach);

-struct p9_fid *p9_client_walk(struct p9_fid *oldfid, uint16_t nwname,
+struct p9_fid *p9_client_walk(struct p9_fid *oldfid, u16 nwname,
 		char **wnames, int clone)
 {
 	int err;
@@ -1140,7 +1140,7 @@  struct p9_fid *p9_client_walk(struct p9_fid
*oldfid, uint16_t nwname,
 	struct p9_fid *fid;
 	struct p9_qid *wqids;
 	struct p9_req_t *req;
-	uint16_t nwqids, count;
+	u16 nwqids, count;

 	err = 0;
 	wqids = NULL;
diff --git a/net/9p/protocol.c b/net/9p/protocol.c
index 9ee48cb..18d9b25 100644
--- a/net/9p/protocol.c
+++ b/net/9p/protocol.c
@@ -80,16 +80,16 @@  pdu_write_u(struct p9_fcall *pdu, const char
__user *udata, size_t size)
 }

 /*
-	b - int8_t
-	w - int16_t
-	d - int32_t
-	q - int64_t
+	b - uu8
+	w - u16
+	d - u32
+	q - u64
 	s - string
 	S - stat
 	Q - qid
-	D - data blob (int32_t size followed by void *, results are not freed)
-	T - array of strings (int16_t count, followed by strings)
-	R - array of qids (int16_t count, followed by qids)
+	D - data blob (u32 size followed by void *, results are not freed)
+	T - array of strings (u16 count, followed by strings)
+	R - array of qids (u16 count, followed by qids)
 	A - stat for 9p2000.L (p9_stat_dotl)
 	? - if optional = 1, continue parsing
 */
@@ -104,7 +104,7 @@  p9pdu_vreadf(struct p9_fcall *pdu, int
proto_version, const char *fmt,
 	for (ptr = fmt; *ptr; ptr++) {
 		switch (*ptr) {
 		case 'b':{
-				int8_t *val = va_arg(ap, int8_t *);
+				u8 *val = va_arg(ap, u8 *);
 				if (pdu_read(pdu, val, sizeof(*val))) {
 					errcode = -EFAULT;
 					break;
@@ -112,7 +112,7 @@  p9pdu_vreadf(struct p9_fcall *pdu, int
proto_version, const char *fmt,
 			}
 			break;
 		case 'w':{
-				int16_t *val = va_arg(ap, int16_t *);
+				u16 *val = va_arg(ap, u16 *);
 				__le16 le_val;
 				if (pdu_read(pdu, &le_val, sizeof(le_val))) {
 					errcode = -EFAULT;
@@ -122,7 +122,7 @@  p9pdu_vreadf(struct p9_fcall *pdu, int
proto_version, const char *fmt,
 			}
 			break;
 		case 'd':{
-				int32_t *val = va_arg(ap, int32_t *);
+				u32 *val = va_arg(ap, u32 *);
 				__le32 le_val;
 				if (pdu_read(pdu, &le_val, sizeof(le_val))) {
 					errcode = -EFAULT;
@@ -132,7 +132,7 @@  p9pdu_vreadf(struct p9_fcall *pdu, int
proto_version, const char *fmt,
 			}
 			break;
 		case 'q':{
-				int64_t *val = va_arg(ap, int64_t *);
+				u64 *val = va_arg(ap, u64 *);
 				__le64 le_val;
 				if (pdu_read(pdu, &le_val, sizeof(le_val))) {
 					errcode = -EFAULT;
@@ -143,7 +143,7 @@  p9pdu_vreadf(struct p9_fcall *pdu, int
proto_version, const char *fmt,
 			break;
 		case 's':{
 				char **sptr = va_arg(ap, char **);
-				uint16_t len;
+				u16 len;

 				errcode = p9pdu_readf(pdu, proto_version,
 								"w", &len);
@@ -196,21 +196,21 @@  p9pdu_vreadf(struct p9_fcall *pdu, int
proto_version, const char *fmt,
 			}
 			break;
 		case 'D':{
-				uint32_t *count = va_arg(ap, uint32_t *);
+				u32 *count = va_arg(ap, u32 *);
 				void **data = va_arg(ap, void **);

 				errcode =
 				    p9pdu_readf(pdu, proto_version, "d", count);
 				if (!errcode) {
 					*count =
-					    min_t(uint32_t, *count,
+					    min_t(u32, *count,
 						  pdu->size - pdu->offset);
 					*data = &pdu->sdata[pdu->offset];
 				}
 			}
 			break;
 		case 'T':{
-				uint16_t *nwname = va_arg(ap, uint16_t *);
+				u16 *nwname = va_arg(ap, u16 *);
 				char ***wnames = va_arg(ap, char ***);

 				errcode = p9pdu_readf(pdu, proto_version,
@@ -250,7 +250,7 @@  p9pdu_vreadf(struct p9_fcall *pdu, int
proto_version, const char *fmt,
 			}
 			break;
 		case 'R':{
-				int16_t *nwqid = va_arg(ap, int16_t *);
+				u16 *nwqid = va_arg(ap, u16 *);
 				struct p9_qid **wqids =
 				    va_arg(ap, struct p9_qid **);

@@ -341,7 +341,7 @@  p9pdu_vwritef(struct p9_fcall *pdu, int
proto_version, const char *fmt,
 	for (ptr = fmt; *ptr; ptr++) {
 		switch (*ptr) {
 		case 'b':{
-				int8_t val = va_arg(ap, int);
+				u8 val = va_arg(ap, int);
 				if (pdu_write(pdu, &val, sizeof(val)))
 					errcode = -EFAULT;
 			}
@@ -353,22 +353,22 @@  p9pdu_vwritef(struct p9_fcall *pdu, int
proto_version, const char *fmt,
 			}
 			break;
 		case 'd':{
-				__le32 val = cpu_to_le32(va_arg(ap, int32_t));
+				__le32 val = cpu_to_le32(va_arg(ap, u32));
 				if (pdu_write(pdu, &val, sizeof(val)))
 					errcode = -EFAULT;
 			}
 			break;
 		case 'q':{
-				__le64 val = cpu_to_le64(va_arg(ap, int64_t));
+				__le64 val = cpu_to_le64(va_arg(ap, u64));
 				if (pdu_write(pdu, &val, sizeof(val)))
 					errcode = -EFAULT;
 			}
 			break;
 		case 's':{
 				const char *sptr = va_arg(ap, const char *);
-				uint16_t len = 0;
+				u16 len = 0;
 				if (sptr)
-					len = min_t(uint16_t, strlen(sptr),
+					len = min_t(u16, strlen(sptr),
 								USHRT_MAX);

 				errcode = p9pdu_writef(pdu, proto_version,
@@ -401,7 +401,7 @@  p9pdu_vwritef(struct p9_fcall *pdu, int
proto_version, const char *fmt,
 						 stbuf->n_gid, stbuf->n_muid);
 			} break;
 		case 'D':{
-				uint32_t count = va_arg(ap, uint32_t);
+				u32 count = va_arg(ap, u32);
 				const void *data = va_arg(ap, const void *);

 				errcode = p9pdu_writef(pdu, proto_version, "d",
@@ -411,7 +411,7 @@  p9pdu_vwritef(struct p9_fcall *pdu, int
proto_version, const char *fmt,
 			}
 			break;
 		case 'U':{
-				int32_t count = va_arg(ap, int32_t);
+				u32 count = va_arg(ap, u32);
 				const char __user *udata =
 						va_arg(ap, const void __user *);
 				errcode = p9pdu_writef(pdu, proto_version, "d",
@@ -421,7 +421,7 @@  p9pdu_vwritef(struct p9_fcall *pdu, int
proto_version, const char *fmt,
 			}
 			break;
 		case 'T':{
-				uint16_t nwname = va_arg(ap, int);
+				u16 nwname = va_arg(ap, int);
 				const char **wnames = va_arg(ap, const char **);

 				errcode = p9pdu_writef(pdu, proto_version, "w",
@@ -442,7 +442,7 @@  p9pdu_vwritef(struct p9_fcall *pdu, int
proto_version, const char *fmt,
 			}
 			break;
 		case 'R':{
-				int16_t nwqid = va_arg(ap, int);
+				u16 nwqid = va_arg(ap, int);
 				struct p9_qid *wqids =
 				    va_arg(ap, struct p9_qid *);

@@ -542,7 +542,7 @@  int p9stat_read(struct p9_client *clnt, char *buf,
int len, struct p9_wstat *st)
 }
 EXPORT_SYMBOL(p9stat_read);

-int p9pdu_prepare(struct p9_fcall *pdu, int16_t tag, int8_t type)
+int p9pdu_prepare(struct p9_fcall *pdu, u16 tag, u8 type)
 {
 	pdu->id = type;
 	return p9pdu_writef(pdu, 0, "dbw", 0, type, tag);
diff --git a/net/9p/protocol.h b/net/9p/protocol.h
index 2cc525fa..d4aa0a4 100644
--- a/net/9p/protocol.h
+++ b/net/9p/protocol.h
@@ -28,7 +28,7 @@ 
 int p9pdu_vwritef(struct p9_fcall *pdu, int proto_version, const char *fmt,
 								va_list ap);
 int p9pdu_readf(struct p9_fcall *pdu, int proto_version, const char *fmt, ...);
-int p9pdu_prepare(struct p9_fcall *pdu, int16_t tag, int8_t type);
+int p9pdu_prepare(struct p9_fcall *pdu, u16 tag, u8 type);
 int p9pdu_finalize(struct p9_client *clnt, struct p9_fcall *pdu);
 void p9pdu_reset(struct p9_fcall *pdu);
 size_t pdu_read(struct p9_fcall *pdu, void *data, size_t size);
diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c
index 2c69ddd..3f41a63 100644
--- a/net/9p/trans_rdma.c
+++ b/net/9p/trans_rdma.c
@@ -279,7 +279,7 @@  handle_recv(struct p9_client *client, struct
p9_trans_rdma *rdma,
 {
 	struct p9_req_t *req;
 	int err = 0;
-	int16_t tag;
+	u16 tag;

 	req = NULL;