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@xxxxxxxxxxxxxxxxx>님이 작성:
>>>
>>>
>>> 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
>
>
