Clean up due to code review.
The struct nfs4_deviceid's data field is not guaranteed to be u64-
aligned. Casting an array of chars to a u32 * or u64 * is considered
generally hazardous.
Since the pointer casts are done just to print these objects, use
get_unaligned() to ensure the access is legal no matter what.
Make sure code always uses the .data field when comparing or copying
these structures.
Also, sizeof(struct nfs4_deviceid) is the size of the in-core device
ID data structure, but NFS4_DEVICEID4_SIZE is the number of octets in
an XDR'd device ID. The two are not interchangeable, even if they
happen to have the same value.
Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
---
Hi Boaz-
I'm thinking something like this. Compile-tested only.
fs/nfs/blocklayout/blocklayoutdev.c | 2 +-
fs/nfs/blocklayout/extents.c | 2 +-
fs/nfs/nfs4filelayout.c | 3 +--
fs/nfs/nfs4filelayoutdev.c | 6 ++++--
fs/nfs/objlayout/pnfs_osd_xdr_cli.c | 5 +++--
fs/nfs/pnfs_dev.c | 8 +++++---
include/linux/pnfs_osd_xdr.h | 16 +++++++++++-----
7 files changed, 26 insertions(+), 16 deletions(-)
diff --git a/fs/nfs/blocklayout/blocklayoutdev.c b/fs/nfs/blocklayout/blocklayoutdev.c
index b48f782..bae2e7b 100644
--- a/fs/nfs/blocklayout/blocklayoutdev.c
+++ b/fs/nfs/blocklayout/blocklayoutdev.c
@@ -323,7 +323,7 @@ nfs4_blk_process_layoutget(struct pnfs_layout_hdr *lo,
status = -ENOMEM;
goto out_err;
}
- memcpy(&be->be_devid, p, NFS4_DEVICEID4_SIZE);
+ memcpy(&be->be_devid.data, p, NFS4_DEVICEID4_SIZE);
p += XDR_QUADLEN(NFS4_DEVICEID4_SIZE);
be->be_mdev = translate_devid(lo, &be->be_devid);
if (!be->be_mdev)
diff --git a/fs/nfs/blocklayout/extents.c b/fs/nfs/blocklayout/extents.c
index 1f9a603..c8f54f5 100644
--- a/fs/nfs/blocklayout/extents.c
+++ b/fs/nfs/blocklayout/extents.c
@@ -675,7 +675,7 @@ encode_pnfs_block_layoutupdate(struct pnfs_block_layout *bl,
if (!xdr_start)
goto out;
list_for_each_entry_safe(lce, save, &bl->bl_commit, bse_node) {
- p = xdr_reserve_space(xdr, 7 * 4 + sizeof(lce->bse_devid.data));
+ p = xdr_reserve_space(xdr, 7 * 4 + NFS4_DEVICEID4_SIZE);
if (!p)
break;
p = xdr_encode_opaque_fixed(p, lce->bse_devid.data, NFS4_DEVICEID4_SIZE);
diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
index 47e8f34..5d77819 100644
--- a/fs/nfs/nfs4filelayout.c
+++ b/fs/nfs/nfs4filelayout.c
@@ -550,8 +550,7 @@ filelayout_decode_layout(struct pnfs_layout_hdr *flo,
if (unlikely(!p))
goto out_err;
- memcpy(id, p, sizeof(*id));
- p += XDR_QUADLEN(NFS4_DEVICEID4_SIZE);
+ p = xdr_decode_opaque_fixed(p, id->data, NFS4_DEVICEID4_SIZE);
nfs4_print_deviceid(id);
nfl_util = be32_to_cpup(p++);
diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c
index 41677f0..905c335 100644
--- a/fs/nfs/nfs4filelayoutdev.c
+++ b/fs/nfs/nfs4filelayoutdev.c
@@ -795,11 +795,13 @@ static void
filelayout_mark_devid_negative(struct nfs4_file_layout_dsaddr *dsaddr,
int err, const char *ds_remotestr)
{
- u32 *p = (u32 *)&dsaddr->id_node.deviceid;
+ u32 *p = (u32 *)dsaddr->id_node.deviceid.data;
printk(KERN_ERR "NFS: data server %s connection error %d."
" Deviceid [%x%x%x%x] marked out of use.\n",
- ds_remotestr, err, p[0], p[1], p[2], p[3]);
+ ds_remotestr, err,
+ get_unaligned(p), get_unaligned(p + 1),
+ get_unaligned(p + 2), get_unaligned(p + 3));
spin_lock(&nfs4_ds_cache_lock);
dsaddr->flags |= NFS4_DEVICE_ID_NEG_ENTRY;
diff --git a/fs/nfs/objlayout/pnfs_osd_xdr_cli.c b/fs/nfs/objlayout/pnfs_osd_xdr_cli.c
index b3918f7..e14b938 100644
--- a/fs/nfs/objlayout/pnfs_osd_xdr_cli.c
+++ b/fs/nfs/objlayout/pnfs_osd_xdr_cli.c
@@ -56,12 +56,13 @@ static __be32 *
_osd_xdr_decode_objid(__be32 *p, struct pnfs_osd_objid *objid)
{
p = xdr_decode_opaque_fixed(p, objid->oid_device_id.data,
- sizeof(objid->oid_device_id.data));
+ NFS4_DEVICEID4_SIZE);
p = xdr_decode_hyper(p, &objid->oid_partition_id);
p = xdr_decode_hyper(p, &objid->oid_object_id);
return p;
}
+
/*
* struct pnfs_osd_opaque_cred {
* u32 cred_len;
@@ -378,7 +379,7 @@ static inline __be32 *
pnfs_osd_xdr_encode_objid(__be32 *p, struct pnfs_osd_objid *object_id)
{
p = xdr_encode_opaque_fixed(p, &object_id->oid_device_id.data,
- sizeof(object_id->oid_device_id.data));
+ NFS4_DEVICEID4_SIZE);
p = xdr_encode_hyper(p, object_id->oid_partition_id);
p = xdr_encode_hyper(p, object_id->oid_object_id);
diff --git a/fs/nfs/pnfs_dev.c b/fs/nfs/pnfs_dev.c
index 4f359d2..3bf5ce1 100644
--- a/fs/nfs/pnfs_dev.c
+++ b/fs/nfs/pnfs_dev.c
@@ -46,10 +46,11 @@ static DEFINE_SPINLOCK(nfs4_deviceid_lock);
void
nfs4_print_deviceid(const struct nfs4_deviceid *id)
{
- u32 *p = (u32 *)id;
+ u32 *p = (u32 *)id->data;
dprintk("%s: device id= [%x%x%x%x]\n", __func__,
- p[0], p[1], p[2], p[3]);
+ get_unaligned(p), get_unaligned(p + 1),
+ get_unaligned(p + 2), get_unaligned(p + 3));
}
EXPORT_SYMBOL_GPL(nfs4_print_deviceid);
@@ -77,7 +78,8 @@ _lookup_deviceid(const struct pnfs_layoutdriver_type *ld,
hlist_for_each_entry_rcu(d, n, &nfs4_deviceid_cache[hash], node)
if (d->ld == ld && d->nfs_client == clp &&
- !memcmp(&d->deviceid, id, sizeof(*id))) {
+ !memcmp(&d->deviceid.data, id->data,
+ NFS4_DEVICEID4_SIZE)) {
if (atomic_read(&d->ref))
return d;
else
diff --git a/include/linux/pnfs_osd_xdr.h b/include/linux/pnfs_osd_xdr.h
index 435dd5f..e0557c1 100644
--- a/include/linux/pnfs_osd_xdr.h
+++ b/include/linux/pnfs_osd_xdr.h
@@ -90,11 +90,17 @@ struct pnfs_osd_objid {
* kprint("dev(%llx:%llx)", _DEVID_LO(pointer), _DEVID_HI(pointer));
* BE style
*/
-#define _DEVID_LO(oid_device_id) \
- (unsigned long long)be64_to_cpup((__be64 *)(oid_device_id)->data)
-
-#define _DEVID_HI(oid_device_id) \
- (unsigned long long)be64_to_cpup(((__be64 *)(oid_device_id)->data) + 1)
+static inline unsigned long long _DEVID_LO(struct nfs4_deviceid *id)
+{
+ __be64 *p = (__be64 *)id->data;
+ return be64_to_cpu(get_unaligned(p));
+}
+
+static inline unsigned long long _DEVID_HI(struct nfs4_deviceid *id)
+{
+ __be64 *p = (__be64 *)id->data;
+ return be64_to_cpu(get_unaligned(p + 1));
+}
enum pnfs_osd_version {
PNFS_OSD_MISSING = 0,
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
