[PATCH v2] IB/core: Fix unaligned accesses

[PATCH v2] IB/core: Fix unaligned accesses

[David Ahern]

Addresses the following kernel logs seen during boot of sparc systems:

Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm]
Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm]
Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm]
Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm]
Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm]

Changed signature of cm_mask_copy from u8 to u32 as suggested by Jason
Gunthorpe.

Signed-off-by: David Ahern <david.ahern-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
---
v2
- updated per Jason's comments

 drivers/infiniband/core/cm.c | 17 ++++++++---------
 include/rdma/ib_cm.h         |  4 ++--
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index e28a494e2a3a..cbbf741987b3 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -437,20 +437,19 @@ static struct cm_id_private * cm_acquire_id(__be32 local_id, __be32 remote_id)
 	return cm_id_priv;
 }
 
-static void cm_mask_copy(u8 *dst, u8 *src, u8 *mask)
+static void cm_mask_copy(u32 *dst, u32 *src, u32 *mask)
 {
 	int i;
 
-	for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(unsigned long); i++)
-		((unsigned long *) dst)[i] = ((unsigned long *) src)[i] &
-					     ((unsigned long *) mask)[i];
+	for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(u32); i++)
+		dst[i] =  src[i] & mask[i];
 }
 
 static int cm_compare_data(struct ib_cm_compare_data *src_data,
 			   struct ib_cm_compare_data *dst_data)
 {
-	u8 src[IB_CM_COMPARE_SIZE];
-	u8 dst[IB_CM_COMPARE_SIZE];
+	u32 src[IB_CM_COMPARE_SIZE];
+	u32 dst[IB_CM_COMPARE_SIZE];
 
 	if (!src_data || !dst_data)
 		return 0;
@@ -460,10 +459,10 @@ static int cm_compare_data(struct ib_cm_compare_data *src_data,
 	return memcmp(src, dst, IB_CM_COMPARE_SIZE);
 }
 
-static int cm_compare_private_data(u8 *private_data,
+static int cm_compare_private_data(u32 *private_data,
 				   struct ib_cm_compare_data *dst_data)
 {
-	u8 src[IB_CM_COMPARE_SIZE];
+	u32 src[IB_CM_COMPARE_SIZE];
 
 	if (!dst_data)
 		return 0;
@@ -546,7 +545,7 @@ static struct cm_id_private * cm_find_listen(struct ib_device *device,
 
 	while (node) {
 		cm_id_priv = rb_entry(node, struct cm_id_private, service_node);
-		data_cmp = cm_compare_private_data(private_data,
+		data_cmp = cm_compare_private_data((u32 *) private_data,
 						   cm_id_priv->compare_data);
 		if ((cm_id_priv->id.service_mask & service_id) ==
 		     cm_id_priv->id.service_id &&
diff --git a/include/rdma/ib_cm.h b/include/rdma/ib_cm.h
index 0e3ff30647d5..0dd576f40c83 100644
--- a/include/rdma/ib_cm.h
+++ b/include/rdma/ib_cm.h
@@ -337,8 +337,8 @@ void ib_destroy_cm_id(struct ib_cm_id *cm_id);
 #define IB_SDP_SERVICE_ID_MASK	cpu_to_be64(0xFFFFFFFFFFFF0000ULL)
 
 struct ib_cm_compare_data {
-	u8  data[IB_CM_COMPARE_SIZE];
-	u8  mask[IB_CM_COMPARE_SIZE];
+	u32  data[IB_CM_COMPARE_SIZE];
+	u32  mask[IB_CM_COMPARE_SIZE];
 };
 
 /**
-- 
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] IB/core: Fix unaligned accesses

[Yann Droneaud]

Hi,

Le jeudi 30 avril 2015 à 22:42 -0400, David Ahern a écrit :
> Addresses the following kernel logs seen during boot of sparc systems:
> 
> Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm]
> Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm]
> Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm]
> Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm]
> Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm]
> 
> Changed signature of cm_mask_copy from u8 to u32 as suggested by Jason
> Gunthorpe.

> Signed-off-by: David Ahern <david.ahern-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> ---
> v2
> - updated per Jason's comments
> 
>  drivers/infiniband/core/cm.c | 17 ++++++++---------
>  include/rdma/ib_cm.h         |  4 ++--
>  2 files changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> index e28a494e2a3a..cbbf741987b3 100644
> --- a/drivers/infiniband/core/cm.c
> +++ b/drivers/infiniband/core/cm.c
> @@ -437,20 +437,19 @@ static struct cm_id_private * cm_acquire_id(__be32 local_id, __be32 remote_id)
>  	return cm_id_priv;
>  }
>  
> -static void cm_mask_copy(u8 *dst, u8 *src, u8 *mask)
> +static void cm_mask_copy(u32 *dst, u32 *src, u32 *mask)
>  {
>  	int i;
>  
> -	for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(unsigned long); i++)
> -		((unsigned long *) dst)[i] = ((unsigned long *) src)[i] &
> -					     ((unsigned long *) mask)[i];
> +	for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(u32); i++)
> +		dst[i] =  src[i] & mask[i];
>  }
>  
>  static int cm_compare_data(struct ib_cm_compare_data *src_data,
>  			   struct ib_cm_compare_data *dst_data)
>  {
> -	u8 src[IB_CM_COMPARE_SIZE];
> -	u8 dst[IB_CM_COMPARE_SIZE];
> +	u32 src[IB_CM_COMPARE_SIZE];
> +	u32 dst[IB_CM_COMPARE_SIZE];
>  

You have to change IB_CM_COMPARE_SIZE, as it's going to allocate more
bytes than necessary.

Perhaps renaming IB_CM_COMPARE_SIZE to IB_CM_COMPARE_COUNT would be
good, then remove the division by sizeof(u32).

>  	if (!src_data || !dst_data)
>  		return 0;
> @@ -460,10 +459,10 @@ static int cm_compare_data(struct ib_cm_compare_data *src_data,
>  	return memcmp(src, dst, IB_CM_COMPARE_SIZE);
>  }
>  
> -static int cm_compare_private_data(u8 *private_data,
> +static int cm_compare_private_data(u32 *private_data,
>  				   struct ib_cm_compare_data *dst_data)
>  {
> -	u8 src[IB_CM_COMPARE_SIZE];
> +	u32 src[IB_CM_COMPARE_SIZE];
>  
>  	if (!dst_data)
>  		return 0;
> @@ -546,7 +545,7 @@ static struct cm_id_private * cm_find_listen(struct ib_device *device,
>  
>  	while (node) {
>  		cm_id_priv = rb_entry(node, struct cm_id_private, service_node);
> -		data_cmp = cm_compare_private_data(private_data,
> +		data_cmp = cm_compare_private_data((u32 *) private_data,
>  						   cm_id_priv->compare_data);
>  		if ((cm_id_priv->id.service_mask & service_id) ==
>  		     cm_id_priv->id.service_id &&
> diff --git a/include/rdma/ib_cm.h b/include/rdma/ib_cm.h
> index 0e3ff30647d5..0dd576f40c83 100644
> --- a/include/rdma/ib_cm.h
> +++ b/include/rdma/ib_cm.h
> @@ -337,8 +337,8 @@ void ib_destroy_cm_id(struct ib_cm_id *cm_id);
>  #define IB_SDP_SERVICE_ID_MASK	cpu_to_be64(0xFFFFFFFFFFFF0000ULL)
>  
>  struct ib_cm_compare_data {
> -	u8  data[IB_CM_COMPARE_SIZE];
> -	u8  mask[IB_CM_COMPARE_SIZE];
> +	u32  data[IB_CM_COMPARE_SIZE];
> +	u32  mask[IB_CM_COMPARE_SIZE];
>  };
>  
>  /**

Anyway, you might want to compile this part of the kernel with
-Wcast-align and ignore the false positive (in particular
every other use of container_of()).

Regards.

-- 
Yann Droneaud
OPTEYA


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] IB/core: Fix unaligned accesses

[David Ahern]

On 5/1/15 2:45 AM, Yann Droneaud wrote:
>>   static int cm_compare_data(struct ib_cm_compare_data *src_data,
>>   			   struct ib_cm_compare_data *dst_data)
>>   {
>> -	u8 src[IB_CM_COMPARE_SIZE];
>> -	u8 dst[IB_CM_COMPARE_SIZE];
>> +	u32 src[IB_CM_COMPARE_SIZE];
>> +	u32 dst[IB_CM_COMPARE_SIZE];
>>
>
> You have to change IB_CM_COMPARE_SIZE, as it's going to allocate more
> bytes than necessary.

d'oh. yes, oversight. Thanks for pointing out.

>
> Perhaps renaming IB_CM_COMPARE_SIZE to IB_CM_COMPARE_COUNT would be
> good, then remove the division by sizeof(u32).

I feel like I stepped into a tar pit... It's a bit odd that 
IB_CM_COMPARE_SIZE is part of an enum rather than a #define.

It seems like the number of bytes being compared is what is relevant 
here; comparing by u32's or unsigned long's is the optimization. So, I'd 
prefer to leave it as SIZE.

If there are no objections I think the better thing to do is pull it out as:

/* compare's are done u32 at a time */
#define IB_CM_COMPARE_SIZE 64/sizeof(u32)



>> diff --git a/include/rdma/ib_cm.h b/include/rdma/ib_cm.h
>> index 0e3ff30647d5..0dd576f40c83 100644
>> --- a/include/rdma/ib_cm.h
>> +++ b/include/rdma/ib_cm.h
>> @@ -337,8 +337,8 @@ void ib_destroy_cm_id(struct ib_cm_id *cm_id);
>>   #define IB_SDP_SERVICE_ID_MASK	cpu_to_be64(0xFFFFFFFFFFFF0000ULL)
>>
>>   struct ib_cm_compare_data {
>> -	u8  data[IB_CM_COMPARE_SIZE];
>> -	u8  mask[IB_CM_COMPARE_SIZE];
>> +	u32  data[IB_CM_COMPARE_SIZE];
>> +	u32  mask[IB_CM_COMPARE_SIZE];
>>   };
>>
>>   /**
>
> Anyway, you might want to compile this part of the kernel with
> -Wcast-align and ignore the false positive (in particular
> every other use of container_of()).

Why?

David
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] IB/core: Fix unaligned accesses

[Bart Van Assche]

In addition to Yann's comments:

On 05/01/15 04:42, David Ahern wrote:
> -static void cm_mask_copy(u8 *dst, u8 *src, u8 *mask)
> +static void cm_mask_copy(u32 *dst, u32 *src, u32 *mask)

Please consider to constify the src and mask arguments.

> -	for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(unsigned long); i++)
> -		((unsigned long *) dst)[i] = ((unsigned long *) src)[i] &
> -					     ((unsigned long *) mask)[i];
> +	for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(u32); i++)
> +		dst[i] =  src[i] & mask[i];

A single space after the equals sign is sufficient.

Thanks,

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] IB/core: Fix unaligned accesses

[David Ahern]

On 5/1/15 3:50 AM, Bart Van Assche wrote:
> In addition to Yann's comments:
>
> On 05/01/15 04:42, David Ahern wrote:
>> -static void cm_mask_copy(u8 *dst, u8 *src, u8 *mask)
>> +static void cm_mask_copy(u32 *dst, u32 *src, u32 *mask)
>
> Please consider to constify the src and mask arguments.
>
>> -    for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(unsigned long); i++)
>> -        ((unsigned long *) dst)[i] = ((unsigned long *) src)[i] &
>> -                         ((unsigned long *) mask)[i];
>> +    for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(u32); i++)
>> +        dst[i] =  src[i] & mask[i];
>
> A single space after the equals sign is sufficient.
>

ack

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] IB/core: Fix unaligned accesses

[Jason Gunthorpe]

On Thu, Apr 30, 2015 at 10:42:48PM -0400, David Ahern wrote:
> Addresses the following kernel logs seen during boot of sparc systems:
> 
> Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm]
> Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm]
> Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm]
> Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm]
> Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm]
> 
> Changed signature of cm_mask_copy from u8 to u32 as suggested by Jason
> Gunthorpe.

+1 to everyone elses comments..

> -static void cm_mask_copy(u8 *dst, u8 *src, u8 *mask)
> +static void cm_mask_copy(u32 *dst, u32 *src, u32 *mask)

masy as well add the const while here

> -	for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(unsigned long); i++)
> -		((unsigned long *) dst)[i] = ((unsigned long *) src)[i] &
> -					     ((unsigned long *) mask)[i];
> +	for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(u32); i++)
> +		dst[i] =  src[i] & mask[i];

for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(*dst); i++)

Is a little more idiomatic

> -		data_cmp = cm_compare_private_data(private_data,
> +		data_cmp = cm_compare_private_data((u32 *) private_data,
>  						   cm_id_priv->compare_data);

I'd be happier without this cast, at the worst move it up one more
layer, at the best, fix the two impacted structures as well.

> I feel like I stepped into a tar pit... It's a bit odd that
> IB_CM_COMPARE_SIZE is part of an enum rather than a #define.

This is, again, idiomatic. '#define CONSTANT' is often frowned upon,
the semantics of enum are much cleaner/safer, eg what you trivially
posted:

> #define IB_CM_COMPARE_SIZE 64/sizeof(u32)

Is wrong, missing brackets.

>> Anyway, you might want to compile this part of the kernel with
>> -Wcast-align and ignore the false positive (in particular
>> every other use of container_of()).

> Why?

To look for more sketchy things that might impact SPARC, ARM, etc.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] IB/core: Fix unaligned accesses

[David Ahern]

On 5/1/15 10:24 AM, Jason Gunthorpe wrote:
>> -static void cm_mask_copy(u8 *dst, u8 *src, u8 *mask)
>> +static void cm_mask_copy(u32 *dst, u32 *src, u32 *mask)
>
> masy as well add the const while here

noted by Bart.

>
>> -	for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(unsigned long); i++)
>> -		((unsigned long *) dst)[i] = ((unsigned long *) src)[i] &
>> -					     ((unsigned long *) mask)[i];
>> +	for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(u32); i++)
>> +		dst[i] =  src[i] & mask[i];
>
> for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(*dst); i++)
>
> Is a little more idiomatic

Sizing of the array using some macro / enum that accounts for u32 
walking versus u8 walking means the sizeof goes away.

>
>> -		data_cmp = cm_compare_private_data(private_data,
>> +		data_cmp = cm_compare_private_data((u32 *) private_data,
>>   						   cm_id_priv->compare_data);
>
> I'd be happier without this cast, at the worst move it up one more
> layer, at the best, fix the two impacted structures as well.

I tried to fix private_data and it blows the patch up.

>
>> I feel like I stepped into a tar pit... It's a bit odd that
>> IB_CM_COMPARE_SIZE is part of an enum rather than a #define.
>
> This is, again, idiomatic. '#define CONSTANT' is often frowned upon,
> the semantics of enum are much cleaner/safer,

'#define NAME VALUE' is used throughout the kernel. Why does ib_rdma 
need to be different?

>>> Anyway, you might want to compile this part of the kernel with
>>> -Wcast-align and ignore the false positive (in particular
>>> every other use of container_of()).
>
>> Why?
>
> To look for more sketchy things that might impact SPARC, ARM, etc.

Seems like a topic for another patch.

David

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] IB/core: Fix unaligned accesses

[Jason Gunthorpe]

On Fri, May 01, 2015 at 10:33:19AM -0600, David Ahern wrote:
> On 5/1/15 10:24 AM, Jason Gunthorpe wrote:
> >>-		data_cmp = cm_compare_private_data(private_data,
> >>+		data_cmp = cm_compare_private_data((u32 *) private_data,
> >>  						   cm_id_priv->compare_data);
> >
> >I'd be happier without this cast, at the worst move it up one more
> >layer, at the best, fix the two impacted structures as well.
> 
> I tried to fix private_data and it blows the patch up.

Add this:

diff --git a/drivers/infiniband/core/cm_msgs.h b/drivers/infiniband/core/cm_msgs.h
index be068f47e47e..a1e8cbf0a234 100644
--- a/drivers/infiniband/core/cm_msgs.h
+++ b/drivers/infiniband/core/cm_msgs.h
@@ -103,7 +103,7 @@ struct cm_req_msg {
        /* local ACK timeout:5, rsvd:3 */
        u8 alt_offset139;
 
-       u8 private_data[IB_CM_REQ_PRIVATE_DATA_SIZE];
+       u32 private_data[IB_CM_REQ_PRIVATE_DATA_SIZE/4];
 
 } __attribute__ ((packed));
 
@@ -801,7 +801,7 @@ struct cm_sidr_req_msg {
        __be16 rsvd;
        __be64 service_id;
 
-       u8 private_data[IB_CM_SIDR_REQ_PRIVATE_DATA_SIZE];
+       u32 private_data[IB_CM_SIDR_REQ_PRIVATE_DATA_SIZE/4];
 } __attribute__ ((packed));
 
 struct cm_sidr_rep_msg {
 
> >>I feel like I stepped into a tar pit... It's a bit odd that
> >>IB_CM_COMPARE_SIZE is part of an enum rather than a #define.
> >
> >This is, again, idiomatic. '#define CONSTANT' is often frowned upon,
> >the semantics of enum are much cleaner/safer,
> 
> '#define NAME VALUE' is used throughout the kernel. Why does ib_rdma
> need to be different?

There is a mix, the style guide endorses both options.
 
> >To look for more sketchy things that might impact SPARC, ARM, etc.
> 
> Seems like a topic for another patch.

Yep

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html