Re: osd_req_encode_op() breakage?

Re: osd_req_encode_op() breakage?

[Alex Elder]

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.

 > Date: Sat, 14 Apr 2012 03:34:20 +0100
 > From: Al Viro <viro@ZenIV.linux.org.uk>
 > To: Sage Weil <sage@newdream.net>
 > Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
 > 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:
 >
 > 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

 >  AFAICS, that sucker had appeared in that form back in
 > commit 68b4476b0bc13fef18266b4140309a30e86739d2
 > Author: Yehuda Sadeh <yehuda@hq.newdream.net>
 > 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?

[PATCH] ceph: osd_client: fix endianness bug in osd_req_encode_op()

[Alex Elder]

 From Al Viro <viro@zeniv.linux.org.uk>

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 <elder@dreamhost.com>

---
  net/ceph/osd_client.c |    2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

Index: b/net/ceph/osd_client.c
===================================================================
--- a/net/ceph/osd_client.c
+++ 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 =

Re: osd_req_encode_op() breakage?

[Al Viro]

On Wed, Apr 18, 2012 at 10:09:05AM -0500, Alex Elder wrote:

> 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...

Re: osd_req_encode_op() breakage?

[Yehuda Sadeh Weinraub]

On Wed, Apr 18, 2012 at 8:32 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Wed, Apr 18, 2012 at 10:09:05AM -0500, Alex Elder wrote:
>
> > 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...

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html