Re: [PATCH net,v4] net/smc: prevent NULL pointer dereference in txopt_get

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

On 15.08.24 08:43, D. Wythe wrote:
> 
> 
> On 8/15/24 11:15 AM, Jeongjun Park wrote:
>> 2024년 8월 15일 (목) 오전 11:51, D. Wythe <alibuda@linux.alibaba.com>님이 작성:
>>>
>>>
>>> On 8/14/24 11:05 PM, Jeongjun Park wrote:
>>>> Alexandra Winter wrote:
>>>>> On 14.08.24 15:11, D. Wythe wrote:
>>>>>>       struct smc_sock {                /* smc sock container */
>>>>>> -    struct sock        sk;
>>>>>> +    union {
>>>>>> +        struct sock        sk;
>>>>>> +        struct inet_sock    inet;
>>>>>> +    };
>>>>> I don't see a path where this breaks, but it looks risky to me.
>>>>> Is an smc_sock always an inet_sock as well? Then can't you go with smc_sock->inet_sock->sk ?
>>>>> Or only in the IPPROTO SMC case, and in the AF_SMC case it is not an inet_sock?
>>>
>>> There is no smc_sock->inet_sock->sk before. And this part here was to
>>> make smc_sock also
>>> be an inet_sock.
>>>
>>> For IPPROTO_SMC, smc_sock should be an inet_sock, but it is not before.
>>> So, the initialization of certain fields
>>> in smc_sock(for example, clcsk) will overwrite modifications made to the
>>> inet_sock part in inet(6)_create.
>>>
>>> For AF_SMC,  the only problem is that  some space will be wasted. Since
>>> AF_SMC don't care the inet_sock part.
>>> However, make the use of sock by AF_SMC and IPPROTO_SMC separately for
>>> the sake of avoid wasting some space
>>> is a little bit extreme.
>>>


Thank you for the explanation D. Wythe. That was my impression also. 
I think it is not very clean and risky to use the same structure (smc_sock)
as inet_sock for IPPROTO_SMC and as smc_sock type for AF_SMC.
I am not concerned about wasting space, mroe about maintainability.



>> Okay. I think using inet_sock instead of sock is also a good idea, but I
>> understand for now.
>>
>> However, for some reason this patch status has become Changes Requested


Afaiu, changes requested in this case means that there is discussion ongoing.


>> , so we will split the patch into two and resend the v5 patch.
>>
>> Regards,
>> Jeongjun Park
> 
> Why so hurry ? Are you rushing for some tasks ? Please be patient.
> 
> The discussion is still ongoing, and you need to wait for everyone's opinions,
> at least you can wait a few days to see if there are any other opinions, even if you think
> your patch is correct.
> 
[...]
> 
> Best wishes,
> D. Wythe


I understand that we have a real problem and need a fix. But I agree with D. Wythe,
please give people a chance for discussion before sending new versions.
Also a version history would be helpful (what changed and why)


>>>> hmm... then how about changing it to something like this?
>>>>
>>>> @@ -283,7 +283,7 @@ struct smc_connection {
>>>>    };
>>>>
>>>>    struct smc_sock {                           /* smc sock container */
>>>> -     struct sock             sk;
>>>> +     struct inet_sock        inet;
>>>>        struct socket           *clcsock;       /* internal tcp socket */
>>>>        void                    (*clcsk_state_change)(struct sock *sk);
>>>
>>> Don't.
>>>
>>>>                                                /* original stat_change fct. */
>>>> @@ -327,7 +327,7 @@ struct smc_sock {                         /* smc sock container */
>>>>                                                 * */
>>>>    };
>>>>
>>>> -#define smc_sk(ptr) container_of_const(ptr, struct smc_sock, sk)
>>>> +#define smc_sk(ptr) container_of_const(ptr, struct smc_sock, inet.sk)
>>>>
>>>>    static inline void smc_init_saved_callbacks(struct smc_sock *smc)
>>>>    {
>>>>
>>>> It is definitely not normal to make the first member of smc_sock as sock.
>>>>
>>>> Therefore, I think it would be appropriate to modify it to use inet_sock
>>>> as the first member like other protocols (sctp, dccp) and access sk in a
>>>> way like &smc->inet.sk.
>>>>
>>>> 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.
>>>>
>>>> What do you think?


Yes, that looks like what I had in mind. 
I am not familiar enough with the details of the SMC code to judge all implications.


>>>>
>>>> Regards,
>>>> Jeongjun Park
> 
>