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

[D. Wythe]

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

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.

Best wishes,
D. Wythe

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