linux-kernel - Re: [RFC PATCH net-next v4 03/11] net/smc: implement ID-related operations of loopback-ism

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 21 Mar 2024 09:12:02 +0100
From: Jan Karcher <jaka@...ux.ibm.com>
To: Wen Gu <guwen@...ux.alibaba.com>, wintera@...ux.ibm.com,
        twinkler@...ux.ibm.com, hca@...ux.ibm.com, gor@...ux.ibm.com,
        agordeev@...ux.ibm.com, davem@...emloft.net, edumazet@...gle.com,
        kuba@...nel.org, pabeni@...hat.com, wenjia@...ux.ibm.com
Cc: borntraeger@...ux.ibm.com, svens@...ux.ibm.com, alibuda@...ux.alibaba.com,
        tonylu@...ux.alibaba.com, linux-kernel@...r.kernel.org,
        linux-s390@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [RFC PATCH net-next v4 03/11] net/smc: implement ID-related
 operations of loopback-ism



On 17/03/2024 11:05, Wen Gu wrote:
> This implements GID and CHID related operations of loopback-ism device.
> loopback-ism acts as an ISMv2. It's GID is generated randomly by UUIDv4
> algorithm and CHID is reserved 0xFFFF.

Hi Wen Gu,

Please add that loopback-ism uses an extended GID instead of a GID here 
aswell.

> 
> Signed-off-by: Wen Gu <guwen@...ux.alibaba.com>
> ---
>   net/smc/smc_loopback.c | 62 ++++++++++++++++++++++++++++++++++++++----
>   net/smc/smc_loopback.h |  3 ++
>   2 files changed, 60 insertions(+), 5 deletions(-)
> 
> diff --git a/net/smc/smc_loopback.c b/net/smc/smc_loopback.c
> index e9170d86e58f..7656a2474500 100644
> --- a/net/smc/smc_loopback.c
> +++ b/net/smc/smc_loopback.c
> @@ -19,11 +19,62 @@
>   #include "smc_loopback.h"
>   
>   #if IS_ENABLED(CONFIG_SMC_LO)
> +#define SMC_LO_V2_CAPABLE	0x1 /* loopback-ism acts as ISMv2 */
> +
>   static const char smc_lo_dev_name[] = "loopback-ism";
>   static struct smc_lo_dev *lo_dev;
>   
> +static void smc_lo_generate_id(struct smc_lo_dev *ldev)
> +{
> +	struct smcd_gid *lgid = &ldev->local_gid;
> +	uuid_t uuid;
> +
> +	uuid_gen(&uuid);
> +	memcpy(&lgid->gid, &uuid, sizeof(lgid->gid));
> +	memcpy(&lgid->gid_ext, (u8 *)&uuid + sizeof(lgid->gid),
> +	       sizeof(lgid->gid_ext));
> +
> +	ldev->chid = SMC_LO_CHID;
> +}

Minor comment. The function name implies that there is one id set whle
there are two different ones. The chid assignment can be easily looked 
over. Maybe changing the function name to `smc_lo_generate_ids` would 
prevent this. What do you think?

Thanks
- Jan

> +
> +static int smc_lo_query_rgid(struct smcd_dev *smcd, struct smcd_gid *rgid,
> +			     u32 vid_valid, u32 vid)
> +{
> +	struct smc_lo_dev *ldev = smcd->priv;
> +
> +	/* rgid should be the same as lgid */
> +	if (!ldev || rgid->gid != ldev->local_gid.gid ||
> +	    rgid->gid_ext != ldev->local_gid.gid_ext)
> +		return -ENETUNREACH;
> +	return 0;
> +}
> +
> +static int smc_lo_supports_v2(void)
> +{
> +	return SMC_LO_V2_CAPABLE;
> +}
> +
> +static void smc_lo_get_local_gid(struct smcd_dev *smcd,
> +				 struct smcd_gid *smcd_gid)
> +{
> +	struct smc_lo_dev *ldev = smcd->priv;
> +
> +	smcd_gid->gid = ldev->local_gid.gid;
> +	smcd_gid->gid_ext = ldev->local_gid.gid_ext;
> +}
> +
> +static u16 smc_lo_get_chid(struct smcd_dev *smcd)
> +{
> +	return ((struct smc_lo_dev *)smcd->priv)->chid;
> +}
> +
> +static struct device *smc_lo_get_dev(struct smcd_dev *smcd)
> +{
> +	return &((struct smc_lo_dev *)smcd->priv)->dev;
> +}
> +
>   static const struct smcd_ops lo_ops = {
> -	.query_remote_gid	= NULL,
> +	.query_remote_gid = smc_lo_query_rgid,
>   	.register_dmb		= NULL,
>   	.unregister_dmb		= NULL,
>   	.add_vlan_id		= NULL,
> @@ -32,10 +83,10 @@ static const struct smcd_ops lo_ops = {
>   	.reset_vlan_required	= NULL,
>   	.signal_event		= NULL,
>   	.move_data		= NULL,
> -	.supports_v2		= NULL,
> -	.get_local_gid		= NULL,
> -	.get_chid		= NULL,
> -	.get_dev		= NULL,
> +	.supports_v2 = smc_lo_supports_v2,
> +	.get_local_gid = smc_lo_get_local_gid,
> +	.get_chid = smc_lo_get_chid,
> +	.get_dev = smc_lo_get_dev,
>   };
>   
>   static struct smcd_dev *smcd_lo_alloc_dev(const struct smcd_ops *ops,
> @@ -95,6 +146,7 @@ static void smcd_lo_unregister_dev(struct smc_lo_dev *ldev)
>   
>   static int smc_lo_dev_init(struct smc_lo_dev *ldev)
>   {
> +	smc_lo_generate_id(ldev);
>   	return smcd_lo_register_dev(ldev);
>   }
>   
> diff --git a/net/smc/smc_loopback.h b/net/smc/smc_loopback.h
> index 9dd44d4c0ca3..55b41133a97f 100644
> --- a/net/smc/smc_loopback.h
> +++ b/net/smc/smc_loopback.h
> @@ -20,10 +20,13 @@
>   
>   #if IS_ENABLED(CONFIG_SMC_LO)
>   #define SMC_LO_MAX_DMBS		5000
> +#define SMC_LO_CHID		0xFFFF
>   
>   struct smc_lo_dev {
>   	struct smcd_dev *smcd;
>   	struct device dev;
> +	u16 chid;
> +	struct smcd_gid local_gid;
>   };
>   #endif
>