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

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

On 8/15/24 3:03 PM, Alexandra Winter wrote:
>
> 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.
>
>

Hi Alexandra,

I understand your concern, the maintainability is of course the most 
important. But if we use different
sock types for IPPROTO_SMC and AF_SMC, it would actually be detrimental 
to maintenance because
we have to use a judgment of which type of sock is to use in all the 
code of smc, it's really dirty.

In fact, because a sock is either given to IPPROTO_SMC as inet_sock or 
to AF_SMC as smc_sock,
it cannot exist the same time.  So it's hard to say what risks there are.

Of course, I have to say that this may not be that clean, but compared 
to adding a type judgment
for every sock usage, it is already a very clean approach.

Best wishes,
D. Wythe

>>> 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
>>