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 <[email protected]>
---
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,
On 03/05/2012 10:36 AM, Chuck Lever wrote:
> 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 <[email protected]>
> ---
>
> 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));
They where not before, but these should be CPU ordered I think,
get_unaligned_be32(). Also in the other places. A be32 raw print is pretty
messy on le machines. Even with %x it can become ugly.
> }
> 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));
get_unaligned_be64, which takes a void * so you don't need any cast or
temps.
I'll test with the get_unaligned_be64 bits, later today
> +}
> +
> +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,
>
Let me test the obj-layout bits and come back to you
Thanks for doing this
Boaz