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

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


> 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