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

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

On 8/15/24 3:56 PM, Alexandra Winter wrote:
>
> On 15.08.24 09:34, D. Wythe wrote:
>>
>> 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.
>>
>
> At least the union makes it visible now, so it is cleaner than before.
> Maybe add a comment to the union, which one is used in which case?
>
>> Best wishes,
>> D. Wythe
>>
> [...]
>
>>>>>>> -#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)
>>>>>>>
>
> Just an idea: Maybe it would be sufficient to do the type judgement in smc_sk() ?

I'm afraid not. We need do at least like this

void  smc_sendmsg(struct sock *sk, ...)
{
     struct smc_inet_sock *inet_smc;
     struct smc_sock * smc ;

     if (sk->protocol == IPPROTO_SMC) {
         inet_smc = smc_inet_sk(sk);
         do_same_sendmsg_but_with_inet_sock(inet_smc);
     } else {
         smc = smc_sk(sk);
         do_same_sendmsg_but_with_smc_sock(smc);
     }
}

I am more prefer to what you said about adding more comments. Of course 
it's just my idea,
We can also see if Jan and Wenjia have any other ideas too.

Best wishes,
D. Wythe


>