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

["D. Wythe" <alibuda@linux.alibaba.com >]

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