Re: [PATCH net,v5,2/2] net/smc: modify smc_sock structure

[Jan Karcher <jaka@linux.ibm.com>]

On 20/08/2024 12:45, Paolo Abeni wrote:
> 
> 
> On 8/15/24 06:39, Jeongjun Park wrote:
>> Since inet_sk(sk)->pinet6 and smc_sk(sk)->clcsock practically
>> point to the same address, when smc_create_clcsk() stores the newly
>> created clcsock in smc_sk(sk)->clcsock, inet_sk(sk)->pinet6 is corrupted
>> into clcsock. This causes NULL pointer dereference and various other
>> memory corruptions.
>>
>> To solve this, we need to modify the smc_sock structure.
>>
>> Fixes: ac7138746e14 ("smc: establish new socket family")
>> Signed-off-by: Jeongjun Park <aha310510@gmail.com>
>> ---
>>   net/smc/smc.h | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/smc/smc.h b/net/smc/smc.h
>> index 34b781e463c4..0d67a02a6ab1 100644
>> --- a/net/smc/smc.h
>> +++ b/net/smc/smc.h
>> @@ -283,7 +283,10 @@ struct smc_connection {
>>   };
>>   struct smc_sock {                /* smc sock container */
>> -    struct sock        sk;
>> +    union {
>> +        struct sock        sk;
>> +        struct inet_sock    inet;
>> +    };
>>       struct socket        *clcsock;    /* internal tcp socket */
>>       void            (*clcsk_state_change)(struct sock *sk);
>>                           /* original stat_change fct. */
> 
> As per the running discussion here:
> 
> https://lore.kernel.org/all/5ad4de6f-48d4-4d1b-b062-e1cd2e8b3600@linux.ibm.com/#t
> 
> you should include at least a add a comment to the union, describing 
> which one is used in which case.
> 
> My personal preference would be directly replacing 'struct sk' with
> 'struct inet_sock inet;' and adjust all the smc->sk access accordingly, 
> likely via a new helper.
> 
> I understand that would be much more invasive, but would align with 
> other AF.

Hi all,

thanks for looking into this patch and providing your view on this topic.

My personal prefernce is clean. I want to have a clean SMC protocol, in 
order to get it on a good path for future improvements.
The union, IMO, is not clean. It makes the problem less implicit but the 
problem is still there.

Jeongjun, do i understand correct that you've tested the change proposed 
by Alexandra with more changes required?

"""
Although this fix would require more code changes, we tested the bug and
confirmed that it was not triggered and the functionality was working
normally.
"""
https://lore.kernel.org/all/20240814150558.46178-1-aha310510@gmail.com/

If so would you mind adding a helper for this check as Paolo suggested 
and send it?
This way we see which change is better for the future.

The statement that SMC would be more aligned with other AFs is already a 
  big win in my book.

Thanks
- Jan

> 
> Thanks,
> 
> Paolo
>