[PATCH net] net/smc: fix mismatch between CLC header and proposal extensions

[PATCH net] net/smc: fix mismatch between CLC header and proposal extensions

[D. Wythe]

The current CLC proposal message construction uses a mix of
`ini->smc_type_v1/v2` and `pclc_base->hdr.typev1/v2` to decide whether
to include optional extensions (IPv6 prefix extension for v1, and v2
extension). This leads to a critical inconsistency: when
`smc_clc_prfx_set()` fails - for example, in IPv6-only environments with
only link-local addresses, or when the local IP address and the outgoing
interface’s network address are not in the same subnet.

As a result, the proposal message is assembled using the stale
`ini->smc_type_v1` value—causing the IPv6 prefix extension to be
included even though the header indicates v1 is not supported.
The peer then receives a malformed CLC proposal where the header type
does not match the payload, and immediately resets the connection.

Fix this by consistently using `pclc_base->hdr.typev1` and
`pclc_base->hdr.typev2`—the authoritative fields that reflect the
actual capabilities advertised in the CLC header—when deciding whether
to include optional extensions, as required by the SMC-R v2
specification ("V1 IP Subnet Extension and V2 Extension only present if
applicable").

Fixes: 8c3dca341aea ("net/smc: build and send V2 CLC proposal")
Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
---
 net/smc/smc_clc.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c
index 157aace169d4..d9ff5f433720 100644
--- a/net/smc/smc_clc.c
+++ b/net/smc/smc_clc.c
@@ -922,7 +922,7 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
 				htons(smc_ism_get_chid(ini->ism_dev[0]));
 		}
 	}
-	if (ini->smc_type_v2 == SMC_TYPE_N) {
+	if (pclc_base->hdr.typev2 == SMC_TYPE_N) {
 		pclc_smcd->v2_ext_offset = 0;
 	} else {
 		struct smc_clc_eid_entry *ueident;
@@ -931,7 +931,7 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
 		v2_ext->hdr.flag.release = SMC_RELEASE;
 		v2_ext_offset = sizeof(*pclc_smcd) -
 			offsetofend(struct smc_clc_msg_smcd, v2_ext_offset);
-		if (ini->smc_type_v1 != SMC_TYPE_N)
+		if (pclc_base->hdr.typev1 != SMC_TYPE_N)
 			v2_ext_offset += sizeof(*pclc_prfx) +
 						pclc_prfx->ipv6_prefixes_cnt *
 						sizeof(ipv6_prfx[0]);
@@ -949,7 +949,7 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
 		}
 		read_unlock(&smc_clc_eid_table.lock);
 	}
-	if (smcd_indicated(ini->smc_type_v2)) {
+	if (smcd_indicated(pclc_base->hdr.typev2)) {
 		struct smcd_gid smcd_gid;
 		u8 *eid = NULL;
 		int entry = 0;
@@ -987,7 +987,7 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
 		}
 		v2_ext->hdr.ism_gid_cnt = entry;
 	}
-	if (smcr_indicated(ini->smc_type_v2)) {
+	if (smcr_indicated(pclc_base->hdr.typev2)) {
 		memcpy(v2_ext->roce, ini->smcrv2.ib_gid_v2, SMC_GID_SIZE);
 		v2_ext->max_conns = net->smc.sysctl_max_conns_per_lgr;
 		v2_ext->max_links = net->smc.sysctl_max_links_per_lgr;
@@ -1003,7 +1003,7 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
 	vec[i++].iov_len = sizeof(*pclc_base);
 	vec[i].iov_base = pclc_smcd;
 	vec[i++].iov_len = sizeof(*pclc_smcd);
-	if (ini->smc_type_v1 != SMC_TYPE_N) {
+	if (pclc_base->hdr.typev1 != SMC_TYPE_N) {
 		vec[i].iov_base = pclc_prfx;
 		vec[i++].iov_len = sizeof(*pclc_prfx);
 		if (pclc_prfx->ipv6_prefixes_cnt > 0) {
@@ -1012,11 +1012,11 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
 					   sizeof(ipv6_prfx[0]);
 		}
 	}
-	if (ini->smc_type_v2 != SMC_TYPE_N) {
+	if (pclc_base->hdr.typev2 != SMC_TYPE_N) {
 		vec[i].iov_base = v2_ext;
 		vec[i++].iov_len = sizeof(*v2_ext) +
 				   (v2_ext->hdr.eid_cnt * SMC_MAX_EID_LEN);
-		if (smcd_indicated(ini->smc_type_v2)) {
+		if (smcd_indicated(pclc_base->hdr.typev2)) {
 			vec[i].iov_base = smcd_v2_ext;
 			vec[i++].iov_len = sizeof(*smcd_v2_ext);
 			if (ini->ism_offered_cnt) {
-- 
2.45.0

Re: [PATCH net] net/smc: fix mismatch between CLC header and proposal extensions

[Alexandra Winter]

On 31.10.25 04:18, D. Wythe wrote:
> The current CLC proposal message construction uses a mix of
> `ini->smc_type_v1/v2` and `pclc_base->hdr.typev1/v2` to decide whether
> to include optional extensions (IPv6 prefix extension for v1, and v2
> extension). This leads to a critical inconsistency: when
> `smc_clc_prfx_set()` fails - for example, in IPv6-only environments with
> only link-local addresses, or when the local IP address and the outgoing
> interface’s network address are not in the same subnet.
> 
> As a result, the proposal message is assembled using the stale
> `ini->smc_type_v1` value—causing the IPv6 prefix extension to be
> included even though the header indicates v1 is not supported.
> The peer then receives a malformed CLC proposal where the header type
> does not match the payload, and immediately resets the connection.
> 
> Fix this by consistently using `pclc_base->hdr.typev1` and
> `pclc_base->hdr.typev2`—the authoritative fields that reflect the
> actual capabilities advertised in the CLC header—when deciding whether
> to include optional extensions, as required by the SMC-R v2
> specification ("V1 IP Subnet Extension and V2 Extension only present if
> applicable").


Just thinking out loud:
It seems to me that the 'ini' structure exists once per socket and is used
to pass information between many functions involved with the handshake.
Did you consider updating ini->smc_type_v1/v2 when `smc_clc_prfx_set()` fails,
and using ini as the authoritative source?
With your patch, it seems to me `ini->smc_type_v1` still contains a stale value,
which may lead to issues in other places or future code.

Re: [PATCH net] net/smc: fix mismatch between CLC header and proposal extensions

[D. Wythe]

On Mon, Nov 03, 2025 at 09:28:22AM +0100, Alexandra Winter wrote:
> 
> 
> On 31.10.25 04:18, D. Wythe wrote:
> > The current CLC proposal message construction uses a mix of
> > `ini->smc_type_v1/v2` and `pclc_base->hdr.typev1/v2` to decide whether
> > to include optional extensions (IPv6 prefix extension for v1, and v2
> > extension). This leads to a critical inconsistency: when
> > `smc_clc_prfx_set()` fails - for example, in IPv6-only environments with
> > only link-local addresses, or when the local IP address and the outgoing
> > interface’s network address are not in the same subnet.
> > 
> > As a result, the proposal message is assembled using the stale
> > `ini->smc_type_v1` value—causing the IPv6 prefix extension to be
> > included even though the header indicates v1 is not supported.
> > The peer then receives a malformed CLC proposal where the header type
> > does not match the payload, and immediately resets the connection.
> > 
> > Fix this by consistently using `pclc_base->hdr.typev1` and
> > `pclc_base->hdr.typev2`—the authoritative fields that reflect the
> > actual capabilities advertised in the CLC header—when deciding whether
> > to include optional extensions, as required by the SMC-R v2
> > specification ("V1 IP Subnet Extension and V2 Extension only present if
> > applicable").
> 
> 
> Just thinking out loud:
> It seems to me that the 'ini' structure exists once per socket and is used
> to pass information between many functions involved with the handshake.
> Did you consider updating ini->smc_type_v1/v2 when `smc_clc_prfx_set()` fails,
> and using ini as the authoritative source?
> With your patch, it seems to me `ini->smc_type_v1` still contains a stale value,
> which may lead to issues in other places or future code.

Based on my understanding, ini->smc_type_v1/v2 represents the local
device's inherent hardware capabilities. This value is a static property
and, from my perspective, should remain immutable, independent of
transient network conditions such as invalid IPv6 prefixes or GID
mismatches. Therefore, I believe modifying this field within
smc_clc_send_proposal() might not be the most appropriate approach.

In contrast, pclc_base->hdr.typev1/v2 reflects the actual capabilities
negotiated for a specific connection—what we might term "soft
capabilities." These can, and often do, dynamically adjust based on
current network conditions (e.g., in the event of a prefix validation
failure) and could potentially be restored if network conditions
improve.

Furthermore, once CLC negotiation is complete, the SMC protocol stack
relies exclusively on these negotiated results for all subsequent
operations. It no longer refers to the initial capability values stored
in ini. Consequently, maintaining ini->smc_type_v1/v2 in its original,
unaltered state appears to present no practical risks or functional
issues.

Re: [PATCH net] net/smc: fix mismatch between CLC header and proposal extensions

[Alexandra Winter]

On 04.11.25 08:08, D. Wythe wrote:
> On Mon, Nov 03, 2025 at 09:28:22AM +0100, Alexandra Winter wrote:
>>
>>
>> On 31.10.25 04:18, D. Wythe wrote:
>>> The current CLC proposal message construction uses a mix of
>>> `ini->smc_type_v1/v2` and `pclc_base->hdr.typev1/v2` to decide whether
>>> to include optional extensions (IPv6 prefix extension for v1, and v2
>>> extension). This leads to a critical inconsistency: when
>>> `smc_clc_prfx_set()` fails - for example, in IPv6-only environments with
>>> only link-local addresses, or when the local IP address and the outgoing
>>> interface’s network address are not in the same subnet.
>>>
>>> As a result, the proposal message is assembled using the stale
>>> `ini->smc_type_v1` value—causing the IPv6 prefix extension to be
>>> included even though the header indicates v1 is not supported.
>>> The peer then receives a malformed CLC proposal where the header type
>>> does not match the payload, and immediately resets the connection.
>>>
>>> Fix this by consistently using `pclc_base->hdr.typev1` and
>>> `pclc_base->hdr.typev2`—the authoritative fields that reflect the
>>> actual capabilities advertised in the CLC header—when deciding whether
>>> to include optional extensions, as required by the SMC-R v2
>>> specification ("V1 IP Subnet Extension and V2 Extension only present if
>>> applicable").
>>
>>
>> Just thinking out loud:
>> It seems to me that the 'ini' structure exists once per socket and is used
>> to pass information between many functions involved with the handshake.
>> Did you consider updating ini->smc_type_v1/v2 when `smc_clc_prfx_set()` fails,
>> and using ini as the authoritative source?
>> With your patch, it seems to me `ini->smc_type_v1` still contains a stale value,
>> which may lead to issues in other places or future code.
> 
> Based on my understanding, ini->smc_type_v1/v2 represents the local
> device's inherent hardware capabilities. This value is a static property
> and, from my perspective, should remain immutable, independent of
> transient network conditions such as invalid IPv6 prefixes or GID
> mismatches. Therefore, I believe modifying this field within
> smc_clc_send_proposal() might not be the most appropriate approach.


'ini' is allocated in __smc_connect() and in smc_listen_work().
So it seems to me the purpose of 'ini' is to store information about the
current connection, not device's inherent hardware capabilities.

Fields like ini->smc_type_v1/v2 and ini->smcd/r_version are adjusted in
multiple places during the handshake.
I must say that the usage of these fields is confusing and looks somehow
redundant to me.
But looking at pclc_base->hdr.typev1/v2, as yet another source of
information doesn't make things cleaner IMO.


> 
> In contrast, pclc_base->hdr.typev1/v2 reflects the actual capabilities
> negotiated for a specific connection—what we might term "soft
> capabilities." These can, and often do, dynamically adjust based on
> current network conditions (e.g., in the event of a prefix validation
> failure) and could potentially be restored if network conditions
> improve.

I don't understand.
The pclc block is freed at the end of smc_clc_send_proposal(). Its
only purpose is to be sent out as intitial proposal. How could you
restore it if network conditions improve?


> 
> Furthermore, once CLC negotiation is complete, the SMC protocol stack
> relies exclusively on these negotiated results for all subsequent
> operations. It no longer refers to the initial capability values stored
> in ini. 

Could you give an example where these negotiated results are referred?
Or do you mean within smc_clc_send_proposal()? The pclc block is freed
at the end of smc_clc_send_proposal(), so where is that result stored?


> Consequently, maintaining ini->smc_type_v1/v2 in its original,
> unaltered state appears to present no practical risks or functional
> issues.

Even if nobody reads these fields today after smc_clc_send_proposal(),
I don't think it is good design to leave stale values there and hope
future editors will understand that.
I understand your patch fixes the observed problem. I am just wondering,
whether it makes the code more maintainable or even more confusing than before.

Re: [PATCH net] net/smc: fix mismatch between CLC header and proposal extensions

[D. Wythe]

On Tue, Nov 04, 2025 at 09:51:09AM +0100, Alexandra Winter wrote:
> 
> 
> On 04.11.25 08:08, D. Wythe wrote:
> > On Mon, Nov 03, 2025 at 09:28:22AM +0100, Alexandra Winter wrote:
> >>
> >>
> >> On 31.10.25 04:18, D. Wythe wrote:
> >>> The current CLC proposal message construction uses a mix of
> >>> `ini->smc_type_v1/v2` and `pclc_base->hdr.typev1/v2` to decide whether
> >>> to include optional extensions (IPv6 prefix extension for v1, and v2
> >>> extension). This leads to a critical inconsistency: when
> >>> `smc_clc_prfx_set()` fails - for example, in IPv6-only environments with
> >>> only link-local addresses, or when the local IP address and the outgoing
> >>> interface’s network address are not in the same subnet.
> >>>
> >>> As a result, the proposal message is assembled using the stale
> >>> `ini->smc_type_v1` value—causing the IPv6 prefix extension to be
> >>> included even though the header indicates v1 is not supported.
> >>> The peer then receives a malformed CLC proposal where the header type
> >>> does not match the payload, and immediately resets the connection.
> >>>
> >>> Fix this by consistently using `pclc_base->hdr.typev1` and
> >>> `pclc_base->hdr.typev2`—the authoritative fields that reflect the
> >>> actual capabilities advertised in the CLC header—when deciding whether
> >>> to include optional extensions, as required by the SMC-R v2
> >>> specification ("V1 IP Subnet Extension and V2 Extension only present if
> >>> applicable").
> >>
> >>
> >> Just thinking out loud:
> >> It seems to me that the 'ini' structure exists once per socket and is used
> >> to pass information between many functions involved with the handshake.
> >> Did you consider updating ini->smc_type_v1/v2 when `smc_clc_prfx_set()` fails,
> >> and using ini as the authoritative source?
> >> With your patch, it seems to me `ini->smc_type_v1` still contains a stale value,
> >> which may lead to issues in other places or future code.
> > 
> > Based on my understanding, ini->smc_type_v1/v2 represents the local
> > device's inherent hardware capabilities. This value is a static property
> > and, from my perspective, should remain immutable, independent of
> > transient network conditions such as invalid IPv6 prefixes or GID
> > mismatches. Therefore, I believe modifying this field within
> > smc_clc_send_proposal() might not be the most appropriate approach.
> 
> 
> 'ini' is allocated in __smc_connect() and in smc_listen_work().
> So it seems to me the purpose of 'ini' is to store information about the
> current connection, not device's inherent hardware capabilities.
> 
> Fields like ini->smc_type_v1/v2 and ini->smcd/r_version are adjusted in
> multiple places during the handshake.
> I must say that the usage of these fields is confusing and looks somehow
> redundant to me.
> But looking at pclc_base->hdr.typev1/v2, as yet another source of
> information doesn't make things cleaner IMO.
>

That’s definitely a reasonable way to look at it as well. If the community
prefers this interpretation as more natural, I’m fully open to it.

I’d like to do some testing first, as I have concerns about
possible side effects from directly modifying ini and if nothing
problematic shows up, I’ll send the updated version with this change.

Best wishes,
D. Wythe