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

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

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

There is no need to send a new patch. If this patch is approved, the net 
maintainer will handle it,
regardless of whether it is a change request or not.

And your new patch, I don't want to go too far, as you are a newcomer, I 
appreciate your report and
willingness to fix this issue. But it's wrong.

If you want to split them, embedding inet_sock should be the first 
patch, which is a basic logical issue.

Then, don't send patches so frequently, I'm very worried that you will 
immediately send out v6 after
seeing it.

Best wishes,
D. Wythe

>>> 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?
>>>
>>> Regards,
>>> Jeongjun Park