[PATCH net] net/smc: prevent device removal during connection setup

[PATCH net] net/smc: prevent device removal during connection setup

[Qi Zhang]

From: Chengfeng Ye <nicoyip.dev@gmail.com>

smc_pnet_determine_gid() stores an smc_ib_device pointer in connection
initialization data while holding smc_ib_devices.mutex. Connection setup
keeps using the pointer after releasing the mutex, but device removal only
waits for links which have already incremented lnk_cnt.

This permits the following interleaving:

  connection setup                 device removal
  ----------------                 --------------
  select smcibdev
  store ini->smcrv2.ib_dev_v2
  mutex_unlock()
                                   remove smcibdev from the device list
                                   observe lnk_cnt == 0
                                   return from the remove callback
                                   kfree(smcibdev)
  copy smcibdev->pnetid

KASAN reported:

  BUG: KASAN: slab-use-after-free in smc_conn_create+0x1330/0x2680
  Read of size 16 at addr ffff888112dbd934 by task smc_bug86/97
  Call Trace:
   __asan_memcpy
   smc_conn_create
   __smc_connect
   smc_connect
   __sys_connect
  Allocated by task 93:
   smc_ib_add_dev
   add_client_context
   ib_register_device
   nldev_newlink
  Freed by task 98:
   kfree
   remove_client_context
   __ib_unregister_device
   nldev_dellink

Track selected-device users separately from established links. Acquire a
selection reference while smc_ib_devices.mutex protects the device list,
and release it when the connection initialization data is discarded. After
delisting a device, wait for these users before terminating its links. This
lets in-flight setup either fail or make any newly created link visible to
the termination pass, while delisting prevents new users.

Fixes: a046d57da19f ("smc: CLC handshake (incl. preparation steps)")
Cc: stable@vger.kernel.org
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
Signed-off-by: Qi Zhang <marsy12010123@gmail.com>
---
 net/smc/af_smc.c   | 11 +++++++----
 net/smc/smc_core.c | 11 +++++++++++
 net/smc/smc_core.h |  3 +++
 net/smc/smc_ib.c   |  5 +++++
 net/smc/smc_ib.h   | 13 +++++++++++++
 net/smc/smc_llc.c  |  8 ++++----
 net/smc/smc_pnet.c |  8 ++++++++
 7 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index b5db69073e20..160271464305 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -1225,7 +1225,7 @@ void smc_fill_gid_list(struct smc_link_group *lgr,
 	       SMC_GID_SIZE);
 
 out:
-	kfree(alt_ini);
+	smc_init_info_free(alt_ini);
 }
 
 static int smc_connect_rdma_v2_prepare(struct smc_sock *smc,
@@ -1596,14 +1596,14 @@ static int __smc_connect(struct smc_sock *smc)
 	SMC_STAT_CLNT_SUCC_INC(sock_net(smc->clcsock->sk), aclc);
 	smc_connect_ism_vlan_cleanup(ini);
 	kfree(buf);
-	kfree(ini);
+	smc_init_info_free(ini);
 	return 0;
 
 vlan_cleanup:
 	smc_connect_ism_vlan_cleanup(ini);
 	kfree(buf);
 fallback:
-	kfree(ini);
+	smc_init_info_free(ini);
 	return smc_connect_decline_fallback(smc, rc, version);
 }
 
@@ -2343,7 +2343,10 @@ static void smc_find_rdma_v2_device_serv(struct smc_sock *new_smc,
 
 not_found:
 	ini->smcr_version &= ~SMC_V2;
+	if (ini->smcrv2.ib_dev_v2_ref)
+		smc_ibdev_init_put(ini->smcrv2.ib_dev_v2);
 	ini->smcrv2.ib_dev_v2 = NULL;
+	ini->smcrv2.ib_dev_v2_ref = false;
 	ini->check_smcrv2 = false;
 }
 
@@ -2587,7 +2590,7 @@ static void smc_listen_work(struct work_struct *work)
 	smc_listen_decline(new_smc, rc, ini ? ini->first_contact_local : 0,
 			   proposal_version);
 out_free:
-	kfree(ini);
+	smc_init_info_free(ini);
 	kfree(buf);
 }
 
diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index cf6b620fef05..e59f327f294c 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -55,6 +55,17 @@ static void __smc_lgr_terminate(struct smc_link_group *lgr, bool soft);
 
 static void smc_link_down_work(struct work_struct *work);
 
+void smc_init_info_free(struct smc_init_info *ini)
+{
+	if (!ini)
+		return;
+	if (ini->ib_dev_ref)
+		smc_ibdev_init_put(ini->ib_dev);
+	if (ini->smcrv2.ib_dev_v2_ref)
+		smc_ibdev_init_put(ini->smcrv2.ib_dev_v2);
+	kfree(ini);
+}
+
 /* return head of link group list and its lock for a given link group */
 static inline struct list_head *smc_lgr_list_head(struct smc_link_group *lgr,
 						  spinlock_t **lgr_lock)
diff --git a/net/smc/smc_core.h b/net/smc/smc_core.h
index 5c18f08a4c8a..385f16cc5368 100644
--- a/net/smc/smc_core.h
+++ b/net/smc/smc_core.h
@@ -403,6 +403,7 @@ struct smc_init_info_smcrv2 {
 
 	/* Output fields when saddr is set */
 	struct smc_ib_device	*ib_dev_v2;
+	bool			ib_dev_v2_ref;
 	u8			ib_port_v2;
 	u8			ib_gid_v2[SMC_GID_SIZE];
 
@@ -438,6 +439,7 @@ struct smc_init_info {
 	u8			peer_mac[ETH_ALEN];
 	u8			peer_systemid[SMC_SYSTEMID_LEN];
 	struct smc_ib_device	*ib_dev;
+	bool			ib_dev_ref;
 	u8			ib_gid[SMC_GID_SIZE];
 	u8			ib_port;
 	u32			ib_clcqpn;
@@ -594,6 +596,7 @@ void smc_rtoken_set2(struct smc_link_group *lgr, int rtok_idx, int link_id,
 void smc_sndbuf_sync_sg_for_device(struct smc_connection *conn);
 void smc_rmb_sync_sg_for_cpu(struct smc_connection *conn);
 int smc_vlan_by_tcpsk(struct socket *clcsock, struct smc_init_info *ini);
+void smc_init_info_free(struct smc_init_info *ini);
 
 void smc_conn_free(struct smc_connection *conn);
 int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini);
diff --git a/net/smc/smc_ib.c b/net/smc/smc_ib.c
index 9bb495707445..01c9b5c62df7 100644
--- a/net/smc/smc_ib.c
+++ b/net/smc/smc_ib.c
@@ -950,6 +950,8 @@ static int smc_ib_add_dev(struct ib_device *ibdev)
 
 	smcibdev->ibdev = ibdev;
 	INIT_WORK(&smcibdev->port_event_work, smc_ib_port_event_work);
+	atomic_set(&smcibdev->init_cnt, 0);
+	init_waitqueue_head(&smcibdev->init_wait);
 	atomic_set(&smcibdev->lnk_cnt, 0);
 	init_waitqueue_head(&smcibdev->lnks_deleted);
 	mutex_init(&smcibdev->mutex);
@@ -1000,6 +1002,9 @@ static void smc_ib_remove_dev(struct ib_device *ibdev, void *client_data)
 	mutex_unlock(&smc_ib_devices.mutex);
 	pr_warn_ratelimited("smc: removing ib device %s\n",
 			    smcibdev->ibdev->name);
+	if (atomic_read(&smcibdev->init_cnt))
+		wait_event(smcibdev->init_wait,
+			   !atomic_read(&smcibdev->init_cnt));
 	smc_smcr_terminate_all(smcibdev);
 	smc_ib_cleanup_per_ibdev(smcibdev);
 	ib_unregister_event_handler(&smcibdev->event_handler);
diff --git a/net/smc/smc_ib.h b/net/smc/smc_ib.h
index ef8ac2b7546d..8e9fd5a200a2 100644
--- a/net/smc/smc_ib.h
+++ b/net/smc/smc_ib.h
@@ -51,6 +51,8 @@ struct smc_ib_device {				/* ib-device infos for smc */
 	struct work_struct	port_event_work;
 	unsigned long		port_event_mask;
 	DECLARE_BITMAP(ports_going_away, SMC_MAX_PORTS);
+	atomic_t		init_cnt;	/* number of device selections */
+	wait_queue_head_t	init_wait;	/* wait for selections to end */
 	atomic_t		lnk_cnt;	/* number of links on ibdev */
 	wait_queue_head_t	lnks_deleted;	/* wait 4 removal of all links*/
 	struct mutex		mutex;		/* protect dev setup+cleanup */
@@ -59,6 +61,17 @@ struct smc_ib_device {				/* ib-device infos for smc */
 	int			ndev_ifidx[SMC_MAX_PORTS]; /* ndev if indexes */
 };
 
+static inline void smc_ibdev_init_get(struct smc_ib_device *smcibdev)
+{
+	atomic_inc(&smcibdev->init_cnt);
+}
+
+static inline void smc_ibdev_init_put(struct smc_ib_device *smcibdev)
+{
+	if (atomic_dec_and_test(&smcibdev->init_cnt))
+		wake_up(&smcibdev->init_wait);
+}
+
 static inline __be32 smc_ib_gid_to_ipv4(u8 gid[SMC_GID_SIZE])
 {
 	struct in6_addr *addr6 = (struct in6_addr *)gid;
diff --git a/net/smc/smc_llc.c b/net/smc/smc_llc.c
index 954b2ff1815c..15ae19a9db07 100644
--- a/net/smc/smc_llc.c
+++ b/net/smc/smc_llc.c
@@ -1118,7 +1118,7 @@ int smc_llc_cli_add_link(struct smc_link *link, struct smc_llc_qentry *qentry)
 out_reject:
 	smc_llc_cli_add_link_reject(qentry);
 out:
-	kfree(ini);
+	smc_init_info_free(ini);
 	kfree(qentry);
 	return rc;
 }
@@ -1192,7 +1192,7 @@ static void smc_llc_cli_add_link_invite(struct smc_link *link,
 	smc_llc_send_add_link(link, ini->ib_dev->mac[ini->ib_port - 1],
 			      ini->ib_gid, NULL, SMC_LLC_REQ);
 out:
-	kfree(ini);
+	smc_init_info_free(ini);
 	kfree(qentry);
 }
 
@@ -1512,7 +1512,7 @@ int smc_llc_srv_add_link(struct smc_link *link,
 	rc = smc_llc_srv_conf_link(link, link_new, lgr_new_t);
 	if (rc)
 		goto out_err;
-	kfree(ini);
+	smc_init_info_free(ini);
 	return 0;
 out_err:
 	if (link_new) {
@@ -1520,7 +1520,7 @@ int smc_llc_srv_add_link(struct smc_link *link,
 		smcr_link_clear(link_new, false);
 	}
 out:
-	kfree(ini);
+	smc_init_info_free(ini);
 	if (send_req_add_link_resp)
 		smc_llc_send_req_add_link_response(req_qentry);
 	return rc;
diff --git a/net/smc/smc_pnet.c b/net/smc/smc_pnet.c
index 63e286e2dfaa..7e01e01a522b 100644
--- a/net/smc/smc_pnet.c
+++ b/net/smc/smc_pnet.c
@@ -980,14 +980,22 @@ static int smc_pnet_determine_gid(struct smc_ib_device *ibdev, int i,
 	if (!ini->check_smcrv2 &&
 	    !smc_ib_determine_gid(ibdev, i, ini->vlan_id, ini->ib_gid, NULL,
 				  NULL)) {
+		if (ini->ib_dev_ref)
+			smc_ibdev_init_put(ini->ib_dev);
+		smc_ibdev_init_get(ibdev);
 		ini->ib_dev = ibdev;
+		ini->ib_dev_ref = true;
 		ini->ib_port = i;
 		return 0;
 	}
 	if (ini->check_smcrv2 &&
 	    !smc_ib_determine_gid(ibdev, i, ini->vlan_id, ini->smcrv2.ib_gid_v2,
 				  NULL, &ini->smcrv2)) {
+		if (ini->smcrv2.ib_dev_v2_ref)
+			smc_ibdev_init_put(ini->smcrv2.ib_dev_v2);
+		smc_ibdev_init_get(ibdev);
 		ini->smcrv2.ib_dev_v2 = ibdev;
+		ini->smcrv2.ib_dev_v2_ref = true;
 		ini->smcrv2.ib_port_v2 = i;
 		return 0;
 	}
-- 
2.43.0

Re: [PATCH net] net/smc: prevent device removal during connection setup

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Use-After-Free in smc_ibdev_init_put() racing with smc_ib_remove_dev()

Pre-existing issues:
- [High] Incomplete Fix / Use-After-Free for SMC-D (ISM) devices during connection setup
--

--- Patch [1]: [PATCH net] net/smc: prevent device removal during connection setup ---
commit 7f17c01034c751116f18a345f6abf2f1dee4b013
Author: Chengfeng Ye <nicoyip.dev@gmail.com>
Subject: net/smc: prevent device removal during connection setup

This patch introduces a reference count mechanism to track in-flight
connection setups and prevent SMC-R devices from being removed prematurely.

> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index b5db69073e20f..160271464305c 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
[ ... ]
> @@ -1596,14 +1596,14 @@ static int __smc_connect(struct smc_sock *smc)
>  	SMC_STAT_CLNT_SUCC_INC(sock_net(smc->clcsock->sk), aclc);
>  	smc_connect_ism_vlan_cleanup(ini);
>  	kfree(buf);
> -	kfree(ini);
> +	smc_init_info_free(ini);

[Severity: High]
This is a pre-existing issue, but while this patch addresses the use-after-free
for RoCE devices, does the same vulnerability still exist for SMC-D (ISM)
devices?

Looking at the connection setup path, an ISM device pointer is selected and
stored without taking a reference:

net/smc/af_smc.c:smc_find_ism_v2_device_clnt() {
    ...
    ini->ism_dev[i] = smcd;
    ...
}

If the ISM device is unregistered concurrently, smcd_unregister_dev() frees it
without waiting for in-flight connection setups:

net/smc/smc_ism.c:smcd_unregister_dev() {
    ...
    kfree(smcd);
}

When the connection setup resumes, the freed smcd pointer is dereferenced:

net/smc/smc_core.c:smc_conn_create() {
    ...
    lgr_list = ini->is_smcd ? &ini->ism_dev[ini->ism_selected]->lgr_list :
                              &smc_lgr_list.list;
    ...
}

Should a similar initialization reference count be added for SMC-D devices?

> diff --git a/net/smc/smc_ib.c b/net/smc/smc_ib.c
> index 9bb495707445e..01c9b5c62df76 100644
> --- a/net/smc/smc_ib.c
> +++ b/net/smc/smc_ib.c
[ ... ]
> @@ -1000,6 +1002,9 @@ static void smc_ib_remove_dev(struct ib_device *ibdev, void *client_data)
>  	mutex_unlock(&smc_ib_devices.mutex);
>  	pr_warn_ratelimited("smc: removing ib device %s\n",
>  			    smcibdev->ibdev->name);
> +	if (atomic_read(&smcibdev->init_cnt))
> +		wait_event(smcibdev->init_wait,
> +			   !atomic_read(&smcibdev->init_cnt));
>  	smc_smcr_terminate_all(smcibdev);
>  	smc_ib_cleanup_per_ibdev(smcibdev);
>  	ib_unregister_event_handler(&smcibdev->event_handler);
> diff --git a/net/smc/smc_ib.h b/net/smc/smc_ib.h
> index ef8ac2b7546df..8e9fd5a200a2b 100644
> --- a/net/smc/smc_ib.h
> +++ b/net/smc/smc_ib.h
[ ... ]
> @@ -59,6 +61,17 @@ struct smc_ib_device {				/* ib-device infos for smc */
>  	int			ndev_ifidx[SMC_MAX_PORTS]; /* ndev if indexes */
>  };
>  
> +static inline void smc_ibdev_init_get(struct smc_ib_device *smcibdev)
> +{
> +	atomic_inc(&smcibdev->init_cnt);
> +}
> +
> +static inline void smc_ibdev_init_put(struct smc_ib_device *smcibdev)
> +{
> +	if (atomic_dec_and_test(&smcibdev->init_cnt))
> +		wake_up(&smcibdev->init_wait);
> +}
> +

[Severity: High]
Can this race with device removal and cause a use-after-free of smcibdev?

If Thread A executes smc_ibdev_init_put() and drops the count to 0 via
atomic_dec_and_test(), it might be preempted before calling wake_up():

Thread A:
    if (atomic_dec_and_test(&smcibdev->init_cnt))
        <preempted>

Thread B then executes smc_ib_remove_dev(), sees the count is 0, skips
wait_event(), and frees the device:

Thread B (smc_ib_remove_dev):
    if (atomic_read(&smcibdev->init_cnt))
        wait_event(...) // skipped
    ...
    kfree(smcibdev);

When Thread A resumes, it will call wake_up() on the now freed smcibdev
pointer.

Could this be avoided by using standard completions or by moving the cleanup
logic into a release function when the count reaches zero?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804123418.2214979-1-marsy12010123@gmail.com?part=1

Re: [PATCH net] net/smc: prevent device removal during connection setup

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260804123418.2214979-1-marsy12010123@gmail.com?part=1
---
--- Patch [1]: [PATCH net] net/smc: prevent device removal during connection setup ---
commit 4fbebcb75cff20404187c33315169e87737af9b3
Author: Chengfeng Ye <nicoyip.dev@gmail.com>

net/smc: prevent device removal during connection setup

This adds a new init_cnt/init_wait pair to struct smc_ib_device, takes a
"selection reference" in smc_pnet_determine_gid() while
smc_ib_devices.mutex is held, releases it from a new smc_init_info_free()
helper, and makes smc_ib_remove_dev() wait for those references before
terminating links.

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index b4208cb186c5e..d2f2d4e2e91c1 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -55,6 +55,17 @@ static void __smc_lgr_terminate(struct smc_link_group *lgr, bool soft);
>  
>  static void smc_link_down_work(struct work_struct *work);
>  
> +void smc_init_info_free(struct smc_init_info *ini)
> +{
> +	if (!ini)
> +		return;
> +	if (ini->ib_dev_ref)
> +		smc_ibdev_init_put(ini->ib_dev);
> +	if (ini->smcrv2.ib_dev_v2_ref)
> +		smc_ibdev_init_put(ini->smcrv2.ib_dev_v2);
> +	kfree(ini);
> +}
> +

[ ... ]

> diff --git a/net/smc/smc_ib.c b/net/smc/smc_ib.c
> index 9bb495707445e..01c9b5c62df76 100644
> --- a/net/smc/smc_ib.c
> +++ b/net/smc/smc_ib.c
> @@ -950,6 +950,8 @@ static int smc_ib_add_dev(struct ib_device *ibdev)
>  
>  	smcibdev->ibdev = ibdev;
>  	INIT_WORK(&smcibdev->port_event_work, smc_ib_port_event_work);
> +	atomic_set(&smcibdev->init_cnt, 0);
> +	init_waitqueue_head(&smcibdev->init_wait);
>  	atomic_set(&smcibdev->lnk_cnt, 0);
>  	init_waitqueue_head(&smcibdev->lnks_deleted);
>  	mutex_init(&smcibdev->mutex);
> @@ -1000,6 +1002,9 @@ static void smc_ib_remove_dev(struct ib_device *ibdev, void *client_data)
>  	mutex_unlock(&smc_ib_devices.mutex);
>  	pr_warn_ratelimited("smc: removing ib device %s\n",
>  			    smcibdev->ibdev->name);
> +	if (atomic_read(&smcibdev->init_cnt))
> +		wait_event(smcibdev->init_wait,
> +			   !atomic_read(&smcibdev->init_cnt));

[Severity: High]
[Finding: main-1-0]
[Sources: opus-5]
Can a remote peer stall this wait_event() indefinitely?  It is
uninterruptible and has no timeout, and the reference it waits for is held
across peer-controlled blocking TCP I/O on the clcsock.

On the client side:

net/smc/af_smc.c:__smc_connect() {
	rc = smc_find_proposal_devices(smc, ini);   /* takes init_cnt via
						      smc_pnet_determine_gid() */
	...
	rc = smc_connect_clc(smc, aclc, ini);       /* send proposal,
						      wait for accept */
	...
	smc_init_info_free(ini);                    /* drops init_cnt */
}

smc_connect_clc() -> smc_clc_send_proposal() does a blocking kernel_sendmsg()
on the kernel clcsock whose sk_sndtimeo is MAX_SCHEDULE_TIMEOUT, so a peer
that stops reading holds the sender until TCP gives up.  The receive side is
chunked:

net/smc/smc_clc.c:smc_clc_wait_msg() {
	while (datlen) {
		u8 tmp[SMC_CLC_RECV_BUF_LEN];
		...
		len = sock_recvmsg(smc->clcsock, &msg, krflags);
	}
}

sk_rcvtimeo is re-armed to CLC_WAIT_TIME for every 100 byte chunk, and datlen
comes from the peer, so the total time is peer controlled rather than bounded
by CLC_WAIT_TIME.

The server side holds the same reference even longer: smc_listen_work() takes
it in smc_listen_find_device() under the global smc_server_lgr_pending mutex
and only releases it at out_free, after the CONFIRM smc_clc_wait_msg() and
smc_listen_rdma_finish().

Should something also prod the in-flight setup to give up?  The new wait runs
before smc_smcr_terminate_all(), and smc_ib_remove_dev() does not set
ports_going_away or inject an error, so nothing makes the handshake abort
early.  The existing lnk_cnt wait in smc_smcr_terminate_all() runs after
smcr_link_down_cond_sched() has actively driven the links down, so it is
bounded by local teardown.

As written, ib_unregister_device() (rdma link delete, driver unbind, hot
unplug, module unload) can block long past the hung task timeout.

>  	smc_smcr_terminate_all(smcibdev);
>  	smc_ib_cleanup_per_ibdev(smcibdev);
>  	ib_unregister_event_handler(&smcibdev->event_handler);
> diff --git a/net/smc/smc_ib.h b/net/smc/smc_ib.h
> index ef8ac2b7546df..8e9fd5a200a2b 100644
> --- a/net/smc/smc_ib.h
> +++ b/net/smc/smc_ib.h
> @@ -51,6 +51,8 @@ struct smc_ib_device {				/* ib-device infos for smc */
>  	struct work_struct	port_event_work;
>  	unsigned long		port_event_mask;
>  	DECLARE_BITMAP(ports_going_away, SMC_MAX_PORTS);
> +	atomic_t		init_cnt;	/* number of device selections */
> +	wait_queue_head_t	init_wait;	/* wait for selections to end */
>  	atomic_t		lnk_cnt;	/* number of links on ibdev */
>  	wait_queue_head_t	lnks_deleted;	/* wait 4 removal of all links*/
>  	struct mutex		mutex;		/* protect dev setup+cleanup */
> @@ -59,6 +61,17 @@ struct smc_ib_device {				/* ib-device infos for smc */
>  	int			ndev_ifidx[SMC_MAX_PORTS]; /* ndev if indexes */
>  };
>  
> +static inline void smc_ibdev_init_get(struct smc_ib_device *smcibdev)
> +{
> +	atomic_inc(&smcibdev->init_cnt);
> +}
> +
> +static inline void smc_ibdev_init_put(struct smc_ib_device *smcibdev)
> +{
> +	if (atomic_dec_and_test(&smcibdev->init_cnt))
> +		wake_up(&smcibdev->init_wait);
> +}

[Severity: High]
[Finding: main-5-5]
[Sources: opus-5]
Is this wake_up() safe?  It touches smcibdev after the last reference on it
has already been dropped.

wait_event() checks its condition before sleeping and again inside
___wait_event() after prepare_to_wait_event(), so the waiter can leave the
wait purely because init_cnt is already zero, without any wakeup:

include/linux/wait.h
	#define wait_event(wq_head, condition)		\
	do {						\
		might_sleep();				\
		if (condition)				\
			break;				\
		__wait_event(wq_head, condition);	\
	} while (0)

So the following interleaving looks possible:

  connection setup                     device removal
  ----------------                     --------------
  smc_init_info_free()
    smc_ibdev_init_put()
      atomic_dec_and_test() == true
  <preempted>
                                       wait_event() sees init_cnt == 0
                                       smc_smcr_terminate_all()
                                       smc_ib_cleanup_per_ibdev()
                                       cancel_work_sync()
                                       kfree(smcibdev)
  wake_up(&smcibdev->init_wait)

The final wake_up() would then take a freed spinlock and walk a freed wait
list.  Would a completion, or a refcount whose release callback performs the
free, close this window?  The existing lnk_cnt/lnks_deleted code has the same
shape, but this patch adds a second independent instance of it.

>  static inline __be32 smc_ib_gid_to_ipv4(u8 gid[SMC_GID_SIZE])
>  {
>  	struct in6_addr *addr6 = (struct in6_addr *)gid;
> diff --git a/net/smc/smc_llc.c b/net/smc/smc_llc.c
> index aa6d83af55eda..a8f8876aea563 100644
> --- a/net/smc/smc_llc.c
> +++ b/net/smc/smc_llc.c
> @@ -1118,7 +1118,7 @@ int smc_llc_cli_add_link(struct smc_link *link, struct smc_llc_qentry *qentry)
>  out_reject:
>  	smc_llc_cli_add_link_reject(qentry);
>  out:
> -	kfree(ini);
> +	smc_init_info_free(ini);
>  	kfree(qentry);
>  	return rc;
>  }

[Severity: Low]
[Finding: main-2-2]
[Sources: opus-5]
This isn't a bug, but the ownership rule for ini->ib_dev and
ini->smcrv2.ib_dev_v2 becomes non-uniform.  Earlier in this same function,
and in smc_llc_srv_add_link(), the fields are filled in with a borrowed
pointer and no smc_ibdev_init_get():

net/smc/smc_llc.c:smc_llc_cli_add_link() {
	if (lgr->smc_version == SMC_V2 && !ini->smcrv2.ib_dev_v2) {
		lgr_new_t = SMC_LGR_ASYMMETRIC_LOCAL;
		ini->smcrv2.ib_dev_v2 = link->smcibdev;
		ini->smcrv2.ib_port_v2 = link->ibport;
	} else if (lgr->smc_version < SMC_V2 && !ini->ib_dev) {
		ini->ib_dev = link->smcibdev;
	...
}

No reference is leaked or over-released, since both assignments are guarded
by the pointer still being NULL and smc_init_info_free() keys purely on the
new flags, and the borrowed device is kept alive by the parent link's
lnk_cnt.  Could a comment next to ib_dev_ref/ib_dev_v2_ref state which
writers own a selection reference, so future callers storing into these
fields know which convention applies?

[ ... ]

> diff --git a/net/smc/smc_pnet.c b/net/smc/smc_pnet.c
> index 63e286e2dfaa8..7e01e01a522b7 100644
> --- a/net/smc/smc_pnet.c
> +++ b/net/smc/smc_pnet.c
> @@ -980,14 +980,22 @@ static int smc_pnet_determine_gid(struct smc_ib_device *ibdev, int i,
>  	if (!ini->check_smcrv2 &&
>  	    !smc_ib_determine_gid(ibdev, i, ini->vlan_id, ini->ib_gid, NULL,
>  				  NULL)) {
> +		if (ini->ib_dev_ref)
> +			smc_ibdev_init_put(ini->ib_dev);
> +		smc_ibdev_init_get(ibdev);
>  		ini->ib_dev = ibdev;
> +		ini->ib_dev_ref = true;
>  		ini->ib_port = i;
>  		return 0;
>  	}

[Severity: High]
[Finding: main-1-1]
[Sources: opus-5]
This isn't a bug introduced by this patch, but the same "publish device
pointer under a list mutex, use it after unlock, device kfree()d by the
removal callback" pattern still exists for SMC-D/ISM, and the commit message
reads as if the class is now covered ("prevent device removal during
connection setup", "Track selected-device users separately from established
links").

The ISM selection is the sibling of the code being fixed here:

net/smc/smc_pnet.c:smc_pnet_find_ism_by_pnetid() {
	mutex_lock(&smcd_dev_list.mutex);
	list_for_each_entry(ismdev, &smcd_dev_list.list, list) {
		if (smc_pnet_match(...) &&
		    !ismdev->going_away && ...) {
			ini->ism_dev[0] = ismdev;
			break;
		}
	}
	mutex_unlock(&smcd_dev_list.mutex);
}

The pointer is dereferenced after the unlock, for example in
smc_clc_send_proposal():

	if (ini->ism_dev[0]) {
		smcd = ini->ism_dev[0];
		copy_to_smcdgid(&smcd_gid, &smcd->dibs->gid);

while removal frees the device with no counter or wait matching the new
init_cnt, and struct smcd_dev has no refcount:

net/smc/smc_ism.c:smcd_unregister_dev() {
	smcd->going_away = 1;
	smc_smcd_terminate_all(smcd);
	mutex_lock(&smcd_dev_list.mutex);
	list_del_init(&smcd->list);
	mutex_unlock(&smcd_dev_list.mutex);
	destroy_workqueue(smcd->event_wq);
	kfree(smcd);
}

going_away is only sampled before the mutex is dropped, so it does not cover
the post-unlock window, which is the same argument the commit message makes
for the RoCE case.  Could the changelog say that only the smc_ib_device half
is addressed, or could the ISM path be handled too?

>  	if (ini->check_smcrv2 &&
>  	    !smc_ib_determine_gid(ibdev, i, ini->vlan_id, ini->smcrv2.ib_gid_v2,
>  				  NULL, &ini->smcrv2)) {
> +		if (ini->smcrv2.ib_dev_v2_ref)
> +			smc_ibdev_init_put(ini->smcrv2.ib_dev_v2);
> +		smc_ibdev_init_get(ibdev);
>  		ini->smcrv2.ib_dev_v2 = ibdev;
> +		ini->smcrv2.ib_dev_v2_ref = true;
>  		ini->smcrv2.ib_port_v2 = i;
>  		return 0;
>  	}