netdev - Re: [RFC net-next 10/17] net/dibs: Define dibs_client_ops and dibs_dev

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]

Message-ID: <20250807194715.GP61519@horms.kernel.org>
Date: Thu, 7 Aug 2025 20:47:15 +0100
From: Simon Horman <horms@...nel.org>
To: Alexandra Winter <wintera@...ux.ibm.com>
Cc: David Miller <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
	Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"D. Wythe" <alibuda@...ux.alibaba.com>,
	Dust Li <dust.li@...ux.alibaba.com>,
	Sidraya Jayagond <sidraya@...ux.ibm.com>,
	Wenjia Zhang <wenjia@...ux.ibm.com>,
	Julian Ruess <julianr@...ux.ibm.com>, netdev@...r.kernel.org,
	linux-s390@...r.kernel.org, Heiko Carstens <hca@...ux.ibm.com>,
	Vasily Gorbik <gor@...ux.ibm.com>,
	Alexander Gordeev <agordeev@...ux.ibm.com>,
	Christian Borntraeger <borntraeger@...ux.ibm.com>,
	Sven Schnelle <svens@...ux.ibm.com>,
	Thorsten Winkler <twinkler@...ux.ibm.com>,
	Mahanta Jambigi <mjambigi@...ux.ibm.com>,
	Tony Lu <tonylu@...ux.alibaba.com>,
	Wen Gu <guwen@...ux.alibaba.com>, Halil Pasic <pasic@...ux.ibm.com>,
	linux-rdma@...r.kernel.org
Subject: Re: [RFC net-next 10/17] net/dibs: Define dibs_client_ops and
 dibs_dev_ops

On Wed, Aug 06, 2025 at 05:41:15PM +0200, Alexandra Winter wrote:

...

> diff --git a/net/smc/smc_ism.c b/net/smc/smc_ism.c

...

> -static void smcd_register_dev(struct ism_dev *ism)
> +static void smcd_register_dev(struct dibs_dev *dibs)
>  {
> -	const struct smcd_ops *ops = ism_get_smcd_ops();
>  	struct smcd_dev *smcd, *fentry;
> +	const struct smcd_ops *ops;
> +	struct smc_lo_dev *smc_lo;
> +	struct ism_dev *ism;
>  
> -	if (!ops)
> -		return;
> +	if (smc_ism_is_loopback(dibs)) {
> +		if (smc_loopback_init(&smc_lo))
> +			return;
> +	}
>  
> -	smcd = smcd_alloc_dev(&ism->pdev->dev, dev_name(&ism->pdev->dev), ops,
> -			      ISM_NR_DMBS);
> +	if (smc_ism_is_loopback(dibs)) {
> +		ops = smc_lo_get_smcd_ops();
> +		smcd = smcd_alloc_dev(dev_name(&smc_lo->dev), ops,
> +				      SMC_LO_MAX_DMBS);
> +	} else {
> +		ism = dibs->drv_priv;
> +		ops = ism_get_smcd_ops();
> +		smcd = smcd_alloc_dev(dev_name(&ism->pdev->dev), ops,
> +				      ISM_NR_DMBS);
> +	}

Hi Alexandra,

ism is initialised conditionally here.

But towards the end of this function the following dereferences
ism unconditionally. And it's not clear to me this won't occur
even if ism wasn't initialised above.

        if (smc_pnet_is_pnetid_set(smcd->pnetid))
                pr_warn_ratelimited("smc: adding smcd device %s with pnetid %.16s%s\n",
                                    dev_name(&ism->dev), smcd->pnetid,
                                    smcd->pnetid_by_user ?
                                        " (user defined)" :
                                        "");
        else
                pr_warn_ratelimited("smc: adding smcd device %s without pnetid\n",
                                    dev_name(&ism->dev));


>  	if (!smcd)
>  		return;
> -	smcd->priv = ism;
> +
> +	smcd->dibs = dibs;
> +	dibs_set_priv(dibs, &smc_dibs_client, smcd);
> +
> +	if (smc_ism_is_loopback(dibs)) {
> +		smcd->priv = smc_lo;
> +		smc_lo->smcd = smcd;
> +	} else {
> +		smcd->priv = ism;
> +		ism_set_priv(ism, &smc_ism_client, smcd);

This function is now compiled even if CONFIG_ISM is not enabled.
But smc_ism_client is only defined if CONFIG_ISM is enabled.

I think this code is removed by later patches. But nonetheless
I also think this leads to a build error and it's best
to avoid transient build errors as they break bisection.

> +		if (smc_pnetid_by_dev_port(&ism->pdev->dev, 0, smcd->pnetid))
> +			smc_pnetid_by_table_smcd(smcd);
> +	}
> +
>  	smcd->client = &smc_ism_client;

Ditto.

> -	ism_set_priv(ism, &smc_ism_client, smcd);
> -	if (smc_pnetid_by_dev_port(&ism->pdev->dev, 0, smcd->pnetid))
> -		smc_pnetid_by_table_smcd(smcd);
>  
>  	if (smcd->ops->supports_v2())
>  		smc_ism_set_v2_capable();

...