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

[Alexandra Winter <wintera@linux.ibm.com>]

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.