Skip to contentSkip to conversation

osd_req_encode_op() breakage?

PATCH New Alex Elder 3 participants 4 messages Apr 18, 2012

Subsystems (2)
VFS Ceph common code (libceph)
lore
Share & tools
lore

Alex Elder

author
#0 lore raw

I got dropped off the kernel.org lists for a bit and didn't see this at first. I don't know whether Sage responded to you or not. I'm getting back to you, just to be sure.

Quoted 16 lines

Date: Sat, 14 Apr 2012 03:34:20 +0100
From: Al Viro <[email protected]>
To: Sage Weil <[email protected]>
Cc: [email protected], [email protected]
Subject: osd_req_encode_op() breakage?

static void osd_req_encode_op(struct ceph_osd_request *req,
struct ceph_osd_op *dst, struct ceph_osd_req_op *src)

{
        dst->op = cpu_to_le16(src->op);

        switch (dst->op) {
        case CEPH_OSD_OP_READ:
        case CEPH_OSD_OP_WRITE:
Quoted 2 lines

is an interesting thing to say, seeing that CEPH_OSD_OP_READ et.al. are all host-endian... Should that be "switch (src->op)" instead?

Yes, you are absolutely correct.

We have obviously not done endianness checks in this code for some time.

I will commit a fix and credit you for it. I'll also make sure we are doing proper testing for this sort of thing on a regular basis.

-Alex

Quoted 13 lines

AFAICS, that sucker had appeared in that form back in
commit 68b4476b0bc13fef18266b4140309a30e86739d2

Author: Yehuda Sadeh <[email protected]>
Date:   Tue Apr 6 15:01:27 2010 -0700

ceph: messenger and osdc changes for rbd

and it seems to be broken on big-endian hosts. Doesn't look like a misspelled le16_to_cpu() either, since dst->op ends up going on the wire...

I'm really mystified by that - it looks like it must've shown up immediately on big-endian hosts; it's not like it was an obscure codepath, after all... Comments?

author
#1 lore raw

PATCH 1/1 ceph: osd_client: fix endianness bug in osd_req_encode_op()

From Al Viro <[email protected]>

Al Viro noticed that we were using a non-cpu-encoded value in a switch statement in osd_req_encode_op(). The result would clearly not work correctly on a big-endian machine.

Signed-off-by Alex Elder <[email protected]>
---
 net/ceph/osd_client.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/ceph/osd_client.c +1 -1
Index: b/net/ceph/osd_client.c
===================================================================
@@ -278,7 +278,7 @@ static void osd_req_encode_op(struct cep
 {
 	dst->op = cpu_to_le16(src->op);
 
-	switch (dst->op) {
+	switch (src->op) {
 	case CEPH_OSD_OP_READ:
 	case CEPH_OSD_OP_WRITE:
 		dst->extent.offset =
maintainer
#2 lore raw
In reply to Alex Elder
Quoted 8 lines · Alex Elder

Yes, you are absolutely correct.

We have obviously not done endianness checks in this code for some time.

I will commit a fix and credit you for it. I'll also make sure we are doing proper testing for this sort of thing on a regular basis.

FWIW, that got caught by sparse, but I would really recommend _testing_ on big-endian - getting an emulated headless e.g. mips box is fairly easy with qemu; I'd done that with debian big-endian mips userland on emulated malta and it didn't take much work to set up.

Speaking of sparse, I think we simply ought to add -D__CHECK_ENDIAN__ to CHECKFLAGS in top-level Makefile. It's not _that_ much noise these days... Not sure which tree should that go through, though...

#3 lore raw
Quoted 12 lines · Al Viro
Quoted 8 lines · Alex Elder

Yes, you are absolutely correct.

We have obviously not done endianness checks in this code for some time.

I will commit a fix and credit you for it.  I'll also make sure we are doing proper testing for this sort of thing on a regular basis.

Quoted 8 lines

FWIW, that got caught by sparse, but I would really recommend _testing_ on big-endian - getting an emulated headless e.g. mips box is fairly easy with qemu; I'd done that with debian big-endian mips userland on emulated malta and it didn't take much work to set up.

Speaking of sparse, I think we simply ought to add -D__CHECK_ENDIAN__ to CHECKFLAGS in top-level Makefile.  It's not _that_ much noise these days...  Not sure which tree should that go through, though...

Definitely. I've run sparse on that code from time to time, but completely forgot about this flag and assumed everything was ok.

Yehuda
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html