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

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

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() ?