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