Re: [RFC net-next 3/7] net/ism: Use uuid_t for ISM GID

[Simon Horman <horms@kernel.org>]

On Wed, Jan 15, 2025 at 08:55:23PM +0100, Alexandra Winter wrote:
> SMC uses 64 Bit and 128 Bit Global Identifiers (GIDs)
> that need to be sent via the SMC protocol.
> When integers are used network endianness and host endianness
> need to be considered.
> 
> Avoid this in the ISM layer by using uuid_t byte arrays.
> Follow on patches could do the same change for SMC, for now
> conversion helper functions are introduced.
> 
> ISM-vPCI devices provide 64 Bit GIDs. Map them to ISM uuid_t GIDs
> like this:
>  _________________________________________
> | 64 Bit ISM-vPCI GID | 00000000_00000000 |
>  -----------------------------------------
> If interpreted as UUID, this would be interpreted as th UIID variant,
> that is reserved for NCS backward compatibility. So it will not collide
> with UUIDs that were generated according to the standard.
> 
> Future ISM devices, shall use real UUIDs as 128 Bit GIDs.
> 
> Note:
> - In this RFC patch smcd_gid is now moved back to smc.h,
>   future patchset should avoid that.
> - ism_dmb and ism_event structs still contain 64 Bit rgid and info
>   fields. A future patch could change them to uuid_t gids. This
>   does not break anything, because ism_loopback does not use them.
> 
> Signed-off-by: Alexandra Winter <wintera@linux.ibm.com>

...

> diff --git a/net/smc/smc_ism.h b/net/smc/smc_ism.h
> index 6763133dd8d0..d041e5a7c459 100644
> --- a/net/smc/smc_ism.h
> +++ b/net/smc/smc_ism.h
> @@ -12,6 +12,7 @@
>  #include <linux/uio.h>
>  #include <linux/types.h>
>  #include <linux/mutex.h>
> +#include <linux/ism.h>
>  
>  #include "smc.h"
>  
> @@ -94,4 +95,24 @@ static inline bool smc_ism_is_loopback(struct smcd_dev *smcd)
>  	return (smcd->ops->get_chid(smcd) == 0xFFFF);
>  }
>  
> +static inline void copy_to_smcdgid(struct smcd_gid *sgid, uuid_t *igid)
> +{
> +	__be64 temp;
> +
> +	memcpy(&temp, igid, sizeof(sgid->gid));
> +	sgid->gid = ntohll(temp);
> +	memcpy(&temp, igid + sizeof(sgid->gid), sizeof(sgid->gid_ext));

Hi Alexandra,

The stride of the pointer arithmetic is the width of igid
so this write will be at an offset of:

   sizeof(igid) + sizeof(sgid->gid) = 128 bytes

Which is beyond the end of *igid.

I think the desired operation is to write at an offset of 8 bytes, so
perhaps this is a way to achieve that, as the bi field is a
16 byte array of u8:

	memcpy(&temp, igid->b + sizeof(sgid->gid), sizeof(sgid->gid_ext));


Flagged by W=1 builds with gcc-14 and clang-19, and by Smatch.

> +	sgid->gid_ext = ntohll(temp);
> +}
> +
> +static inline void copy_to_ismgid(uuid_t *igid, struct smcd_gid *sgid)
> +{
> +	__be64 temp;
> +
> +	temp = htonll(sgid->gid);
> +	memcpy(igid, &temp, sizeof(sgid->gid));
> +	temp = htonll(sgid->gid_ext);
> +	memcpy(igid + sizeof(sgid->gid), &temp, sizeof(sgid->gid_ext));

I believe there is a similar problem here too.

> +}
> +
>  #endif
> -- 
> 2.45.2
>