[PATCH 0/4] ceph: endianness fixes and sparse warning fixes

[PATCH 0/4] ceph: endianness fixes and sparse warning fixes

[Jeff Layton]

Just a small set of patches to fix some stuff that sparse was
complaining about. Some of these look like real bugs however for
anyone trying to use ceph on a big endian arch.

Jeff Layton (4):
  ceph: fix endianness of getattr mask in ceph_d_revalidate
  ceph: fix endianness bug in frag_tree_split_cmp
  ceph: fix bogus endianness change in ceph_ioctl_set_layout
  ceph: fix bad endianness handling in parse_reply_info_extra

 fs/ceph/dir.c        | 5 +++--
 fs/ceph/inode.c      | 3 ++-
 fs/ceph/ioctl.c      | 4 ++--
 fs/ceph/mds_client.c | 9 +++++----
 4 files changed, 12 insertions(+), 9 deletions(-)

-- 
2.7.4

[PATCH 1/4] ceph: fix endianness of getattr mask in ceph_d_revalidate

[Jeff Layton]

    fs/ceph/dir.c:1248:50: warning: incorrect type in assignment (different base types)
    fs/ceph/dir.c:1248:50:    expected restricted __le32 [usertype] mask
    fs/ceph/dir.c:1248:50:    got int [signed] [assigned] mask

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/ceph/dir.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index cd99b26de4da..d4385563b70a 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -1230,7 +1230,8 @@ static int ceph_d_revalidate(struct dentry *dentry, unsigned int flags)
 		struct ceph_mds_client *mdsc =
 			ceph_sb_to_client(dir->i_sb)->mdsc;
 		struct ceph_mds_request *req;
-		int op, mask, err;
+		int op, err;
+		u32 mask;
 
 		if (flags & LOOKUP_RCU)
 			return -ECHILD;
@@ -1245,7 +1246,7 @@ static int ceph_d_revalidate(struct dentry *dentry, unsigned int flags)
 			mask = CEPH_STAT_CAP_INODE | CEPH_CAP_AUTH_SHARED;
 			if (ceph_security_xattr_wanted(dir))
 				mask |= CEPH_CAP_XATTR_SHARED;
-			req->r_args.getattr.mask = mask;
+			req->r_args.getattr.mask = cpu_to_le32(mask);
 
 			err = ceph_mdsc_do_request(mdsc, NULL, req);
 			switch (err) {
-- 
2.7.4

Re: [PATCH 1/4] ceph: fix endianness of getattr mask in ceph_d_revalidate

[Ilya Dryomov]

On Thu, Jan 12, 2017 at 5:12 PM, Jeff Layton <jlayton@redhat.com> wrote:
>     fs/ceph/dir.c:1248:50: warning: incorrect type in assignment (different base types)
>     fs/ceph/dir.c:1248:50:    expected restricted __le32 [usertype] mask
>     fs/ceph/dir.c:1248:50:    got int [signed] [assigned] mask

This one is missing "sparse says" ;)

Thanks,

                Ilya

Re: [PATCH 1/4] ceph: fix endianness of getattr mask in ceph_d_revalidate

[Jeff Layton]

On Thu, 2017-01-12 at 17:43 +0100, Ilya Dryomov wrote:
> On Thu, Jan 12, 2017 at 5:12 PM, Jeff Layton <jlayton@redhat.com> wrote:
> >     fs/ceph/dir.c:1248:50: warning: incorrect type in assignment (different base types)
> >     fs/ceph/dir.c:1248:50:    expected restricted __le32 [usertype] mask
> >     fs/ceph/dir.c:1248:50:    got int [signed] [assigned] mask
> 
> This one is missing "sparse says" ;)
> 
> Thanks,

Oops! Fixed in my tree, which is here, btw:

    git://git.samba.org/jlayton/linux.git

...in my ceph-4.10 branch.
-- 
Jeff Layton <jlayton@redhat.com>

[PATCH 2/4] ceph: fix endianness bug in frag_tree_split_cmp

[Jeff Layton]

sparse says:

fs/ceph/inode.c:308:36: warning: incorrect type in argument 1 (different base types)
fs/ceph/inode.c:308:36:    expected unsigned int [unsigned] [usertype] a
fs/ceph/inode.c:308:36:    got restricted __le32 [usertype] frag
fs/ceph/inode.c:308:46: warning: incorrect type in argument 2 (different base types)
fs/ceph/inode.c:308:46:    expected unsigned int [unsigned] [usertype] b
fs/ceph/inode.c:308:46:    got restricted __le32 [usertype] frag

I think we need to convert these values to host-endian before calling
the comparator.

Fixes: a407846ef7c6e (ceph: don't assume frag tree splits in mds reply are sorted)
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/ceph/inode.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 1476f54ed118..4926265f4223 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -305,7 +305,8 @@ static int frag_tree_split_cmp(const void *l, const void *r)
 {
 	struct ceph_frag_tree_split *ls = (struct ceph_frag_tree_split*)l;
 	struct ceph_frag_tree_split *rs = (struct ceph_frag_tree_split*)r;
-	return ceph_frag_compare(ls->frag, rs->frag);
+	return ceph_frag_compare(le32_to_cpu(ls->frag),
+				 le32_to_cpu(rs->frag));
 }
 
 static bool is_frag_child(u32 f, struct ceph_inode_frag *frag)
-- 
2.7.4

[PATCH 3/4] ceph: fix bogus endianness change in ceph_ioctl_set_layout

[Jeff Layton]

sparse says:

    fs/ceph/ioctl.c:100:28: warning: cast to restricted __le64

preferred_osd is a __s64 so we don't need to do any conversion. Also,
just remove the cast in ceph_ioctl_get_layout as it's not needed.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/ceph/ioctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ceph/ioctl.c b/fs/ceph/ioctl.c
index 7d752d53353a..4c9c72f26eb9 100644
--- a/fs/ceph/ioctl.c
+++ b/fs/ceph/ioctl.c
@@ -25,7 +25,7 @@ static long ceph_ioctl_get_layout(struct file *file, void __user *arg)
 		l.stripe_count = ci->i_layout.stripe_count;
 		l.object_size = ci->i_layout.object_size;
 		l.data_pool = ci->i_layout.pool_id;
-		l.preferred_osd = (s32)-1;
+		l.preferred_osd = -1;
 		if (copy_to_user(arg, &l, sizeof(l)))
 			return -EFAULT;
 	}
@@ -97,7 +97,7 @@ static long ceph_ioctl_set_layout(struct file *file, void __user *arg)
 		nl.data_pool = ci->i_layout.pool_id;
 
 	/* this is obsolete, and always -1 */
-	nl.preferred_osd = le64_to_cpu(-1);
+	nl.preferred_osd = -1;
 
 	err = __validate_layout(mdsc, &nl);
 	if (err)
-- 
2.7.4

[PATCH 4/4] ceph: fix bad endianness handling in parse_reply_info_extra

[Jeff Layton]

sparse says:

    fs/ceph/mds_client.c:291:23: warning: restricted __le32 degrades to integer
    fs/ceph/mds_client.c:293:28: warning: restricted __le32 degrades to integer
    fs/ceph/mds_client.c:294:28: warning: restricted __le32 degrades to integer
    fs/ceph/mds_client.c:296:28: warning: restricted __le32 degrades to integer

The op value is __le32, so we need to convert it before comparing it.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/ceph/mds_client.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 4568f80308f8..176512960b14 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -288,12 +288,13 @@ static int parse_reply_info_extra(void **p, void *end,
 				  struct ceph_mds_reply_info_parsed *info,
 				  u64 features)
 {
-	if (info->head->op == CEPH_MDS_OP_GETFILELOCK)
+	u32 op = le32_to_cpu(info->head->op);
+
+	if (op == CEPH_MDS_OP_GETFILELOCK)
 		return parse_reply_info_filelock(p, end, info, features);
-	else if (info->head->op == CEPH_MDS_OP_READDIR ||
-		 info->head->op == CEPH_MDS_OP_LSSNAP)
+	else if (op == CEPH_MDS_OP_READDIR || op == CEPH_MDS_OP_LSSNAP)
 		return parse_reply_info_dir(p, end, info, features);
-	else if (info->head->op == CEPH_MDS_OP_CREATE)
+	else if (op == CEPH_MDS_OP_CREATE)
 		return parse_reply_info_create(p, end, info, features);
 	else
 		return -EIO;
-- 
2.7.4

Re: [PATCH 0/4] ceph: endianness fixes and sparse warning fixes

[Sage Weil]

On Thu, 12 Jan 2017, Jeff Layton wrote:
> Just a small set of patches to fix some stuff that sparse was
> complaining about. Some of these look like real bugs however for
> anyone trying to use ceph on a big endian arch.
> 
> Jeff Layton (4):
>   ceph: fix endianness of getattr mask in ceph_d_revalidate
>   ceph: fix endianness bug in frag_tree_split_cmp
>   ceph: fix bogus endianness change in ceph_ioctl_set_layout
>   ceph: fix bad endianness handling in parse_reply_info_extra
> 
>  fs/ceph/dir.c        | 5 +++--
>  fs/ceph/inode.c      | 3 ++-
>  fs/ceph/ioctl.c      | 4 ++--
>  fs/ceph/mds_client.c | 9 +++++----
>  4 files changed, 12 insertions(+), 9 deletions(-)

Reviewed-by: Sage Weil <sage@redhat.com>

Re: [PATCH 0/4] ceph: endianness fixes and sparse warning fixes

[Jeff Layton]

On Thu, 2017-01-12 at 16:23 +0000, Sage Weil wrote:
> On Thu, 12 Jan 2017, Jeff Layton wrote:
> > Just a small set of patches to fix some stuff that sparse was
> > complaining about. Some of these look like real bugs however for
> > anyone trying to use ceph on a big endian arch.
> > 
> > Jeff Layton (4):
> >   ceph: fix endianness of getattr mask in ceph_d_revalidate
> >   ceph: fix endianness bug in frag_tree_split_cmp
> >   ceph: fix bogus endianness change in ceph_ioctl_set_layout
> >   ceph: fix bad endianness handling in parse_reply_info_extra
> > 
> >  fs/ceph/dir.c        | 5 +++--
> >  fs/ceph/inode.c      | 3 ++-
> >  fs/ceph/ioctl.c      | 4 ++--
> >  fs/ceph/mds_client.c | 9 +++++----
> >  4 files changed, 12 insertions(+), 9 deletions(-)
> 
> Reviewed-by: Sage Weil <sage@redhat.com>

Thanks. Should we send this series to stable? I'm not sure how common
kcephfs mounts are on BE arches, but these bugs would likely prevent it
from working.

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 0/4] ceph: endianness fixes and sparse warning fixes

[David Disseldorp]

On Fri, 13 Jan 2017 07:36:05 -0500, Jeff Layton wrote:

> Thanks. Should we send this series to stable? I'm not sure how common
> kcephfs mounts are on BE arches, but these bugs would likely prevent it
> from working.

Please do, Jeff. I hit the [PATCH 4/4] bug on first CephFS I/O with
s390x (BE):
[ 2691.805255] ceph: mds parse_reply err -5
[ 2691.805256] ceph: mdsc_handle_reply got corrupt reply mds0(tid:5)

Cheers, David

Re: [PATCH 0/4] ceph: endianness fixes and sparse warning fixes

[Ilya Dryomov]

On Tue, Jan 17, 2017 at 7:26 PM, David Disseldorp <ddiss@suse.de> wrote:
> On Fri, 13 Jan 2017 07:36:05 -0500, Jeff Layton wrote:
>
>> Thanks. Should we send this series to stable? I'm not sure how common
>> kcephfs mounts are on BE arches, but these bugs would likely prevent it
>> from working.
>
> Please do, Jeff. I hit the [PATCH 4/4] bug on first CephFS I/O with
> s390x (BE):
> [ 2691.805255] ceph: mds parse_reply err -5
> [ 2691.805256] ceph: mdsc_handle_reply got corrupt reply mds0(tid:5)

I'll pick these up.  Thanks for confirming,

                Ilya