[net,v3] net/smc: prevent NULL pointer dereference in txopt_get

Commit Message

Jeongjun Park Aug. 13, 2024, 10:07 a.m. UTC

Since smc_inet6_prot does not initialize ipv6_pinfo_offset, inet6_create()
copies an incorrect address value, sk + 0 (offset), to inet_sk(sk)->pinet6.

In addition, since inet_sk(sk)->pinet6 and smc_sk(sk)->clcsock practically
point to the same address, when smc_create_clcsk() stores the newly
created clcsock in smc_sk(sk)->clcsock, inet_sk(sk)->pinet6 is corrupted
into clcsock. This causes NULL pointer dereference and various other
memory corruptions.

To solve this, we need to add a smc6_sock structure for ipv6_pinfo_offset
initialization and modify the smc_sock structure.

Reported-by: [e-mail]
Tested-by: [e-mail]
Fixes: d25a92ccae6b ("net/smc: Introduce IPPROTO_SMC")
Signed-off-by: Jeongjun Park <[e-mail]>
---
 net/smc/smc.h      | 19 ++++++++++---------
 net/smc/smc_inet.c | 24 +++++++++++++++---------
 2 files changed, 25 insertions(+), 18 deletions(-)

--

Comments

D. Wythe Aug. 13, 2024, 11:11 a.m. UTC | #1

On 8/13/24 6:07 PM, Jeongjun Park wrote:
> Since smc_inet6_prot does not initialize ipv6_pinfo_offset, inet6_create()
> copies an incorrect address value, sk + 0 (offset), to inet_sk(sk)->pinet6.
>
> In addition, since inet_sk(sk)->pinet6 and smc_sk(sk)->clcsock practically
> point to the same address, when smc_create_clcsk() stores the newly
> created clcsock in smc_sk(sk)->clcsock, inet_sk(sk)->pinet6 is corrupted
> into clcsock. This causes NULL pointer dereference and various other
> memory corruptions.
>
> To solve this, we need to add a smc6_sock structure for ipv6_pinfo_offset
> initialization and modify the smc_sock structure.
>
> Reported-by: [e-mail]
> Tested-by: [e-mail]
> Fixes: d25a92ccae6b ("net/smc: Introduce IPPROTO_SMC")
> Signed-off-by: Jeongjun Park <[e-mail]>
> ---
>   net/smc/smc.h      | 19 ++++++++++---------
>   net/smc/smc_inet.c | 24 +++++++++++++++---------
>   2 files changed, 25 insertions(+), 18 deletions(-)
>
> diff --git a/net/smc/smc.h b/net/smc/smc.h
> index 34b781e463c4..f4d9338b5ed5 100644
> --- a/net/smc/smc.h
> +++ b/net/smc/smc.h
> @@ -284,15 +284,6 @@ struct smc_connection {
>   
>   struct smc_sock {				/* smc sock container */
>   	struct sock		sk;
> -	struct socket		*clcsock;	/* internal tcp socket */
> -	void			(*clcsk_state_change)(struct sock *sk);
> -						/* original stat_change fct. */
> -	void			(*clcsk_data_ready)(struct sock *sk);
> -						/* original data_ready fct. */
> -	void			(*clcsk_write_space)(struct sock *sk);
> -						/* original write_space fct. */
> -	void			(*clcsk_error_report)(struct sock *sk);
> -						/* original error_report fct. */
>   	struct smc_connection	conn;		/* smc connection */
>   	struct smc_sock		*listen_smc;	/* listen parent */
>   	struct work_struct	connect_work;	/* handle non-blocking connect*/
> @@ -325,6 +316,16 @@ struct smc_sock {				/* smc sock container */
>   						/* protects clcsock of a listen
>   						 * socket
>   						 * */
> +	struct socket		*clcsock;	/* internal tcp socket */
> +	void			(*clcsk_state_change)(struct sock *sk);
> +						/* original stat_change fct. */
> +	void			(*clcsk_data_ready)(struct sock *sk);
> +						/* original data_ready fct. */
> +	void			(*clcsk_write_space)(struct sock *sk);
> +						/* original write_space fct. */
> +	void			(*clcsk_error_report)(struct sock *sk);
> +						/* original error_report fct. */
> +
>   };

Please don't send patches so frequently 

D. Wythe Aug. 13, 2024, 11:37 a.m. UTC | #2

On 8/13/24 6:07 PM, Jeongjun Park wrote:
> Since smc_inet6_prot does not initialize ipv6_pinfo_offset, inet6_create()
> copies an incorrect address value, sk + 0 (offset), to inet_sk(sk)->pinet6.
>
> In addition, since inet_sk(sk)->pinet6 and smc_sk(sk)->clcsock practically
> point to the same address, when smc_create_clcsk() stores the newly
> created clcsock in smc_sk(sk)->clcsock, inet_sk(sk)->pinet6 is corrupted
> into clcsock. This causes NULL pointer dereference and various other
> memory corruptions.
>
> To solve this, we need to add a smc6_sock structure for ipv6_pinfo_offset
> initialization and modify the smc_sock structure.
>
> Reported-by: [e-mail]
> Tested-by: [e-mail]
> Fixes: d25a92ccae6b ("net/smc: Introduce IPPROTO_SMC")
> Signed-off-by: Jeongjun Park <[e-mail]>
> ---
>   net/smc/smc.h      | 19 ++++++++++---------
>   net/smc/smc_inet.c | 24 +++++++++++++++---------
>   2 files changed, 25 insertions(+), 18 deletions(-)
>
> diff --git a/net/smc/smc.h b/net/smc/smc.h
> index 34b781e463c4..f4d9338b5ed5 100644
> --- a/net/smc/smc.h
> +++ b/net/smc/smc.h
> @@ -284,15 +284,6 @@ struct smc_connection {
>   
>   struct smc_sock {				/* smc sock container */
>   	struct sock		sk;
> -	struct socket		*clcsock;	/* internal tcp socket */
> -	void			(*clcsk_state_change)(struct sock *sk);
> -						/* original stat_change fct. */
> -	void			(*clcsk_data_ready)(struct sock *sk);
> -						/* original data_ready fct. */
> -	void			(*clcsk_write_space)(struct sock *sk);
> -						/* original write_space fct. */
> -	void			(*clcsk_error_report)(struct sock *sk);
> -						/* original error_report fct. */
>   	struct smc_connection	conn;		/* smc connection */
>   	struct smc_sock		*listen_smc;	/* listen parent */
>   	struct work_struct	connect_work;	/* handle non-blocking connect*/
> @@ -325,6 +316,16 @@ struct smc_sock {				/* smc sock container */
>   						/* protects clcsock of a listen
>   						 * socket
>   						 * */
> +	struct socket		*clcsock;	/* internal tcp socket */
> +	void			(*clcsk_state_change)(struct sock *sk);
> +						/* original stat_change fct. */
> +	void			(*clcsk_data_ready)(struct sock *sk);
> +						/* original data_ready fct. */
> +	void			(*clcsk_write_space)(struct sock *sk);
> +						/* original write_space fct. */
> +	void			(*clcsk_error_report)(struct sock *sk);
> +						/* original error_report fct. */
> +
>   };
>   
>   #define smc_sk(ptr) container_of_const(ptr, struct smc_sock, sk)
> diff --git a/net/smc/smc_inet.c b/net/smc/smc_inet.c
> index bece346dd8e9..25f34fd65e8d 100644
> --- a/net/smc/smc_inet.c
> +++ b/net/smc/smc_inet.c
> @@ -60,16 +60,22 @@ static struct inet_protosw smc_inet_protosw = {
>   };
>   
>   #if IS_ENABLED(CONFIG_IPV6)
> +struct smc6_sock {
> +	struct smc_sock smc;
> +	struct ipv6_pinfo np;
> +};

I prefer to:

struct ipv6_pinfo inet6;

> +
>   static struct proto smc_inet6_prot = {
> -	.name		= "INET6_SMC",
> -	.owner		= THIS_MODULE,
> -	.init		= smc_inet_init_sock,
> -	.hash		= smc_hash_sk,
> -	.unhash		= smc_unhash_sk,
> -	.release_cb	= smc_release_cb,
> -	.obj_size	= sizeof(struct smc_sock),
> -	.h.smc_hash	= &smc_v6_hashinfo,
> -	.slab_flags	= SLAB_TYPESAFE_BY_RCU,
> +	.name				= "INET6_SMC",
> +	.owner				= THIS_MODULE,
> +	.init				= smc_inet_init_sock,
> +	.hash				= smc_hash_sk,
> +	.unhash				= smc_unhash_sk,
> +	.release_cb			= smc_release_cb,
> +	.obj_size			= sizeof(struct smc6_sock),
> +	.h.smc_hash			= &smc_v6_hashinfo,
> +	.slab_flags			= SLAB_TYPESAFE_BY_RCU,
> +	.ipv6_pinfo_offset		= offsetof(struct smc6_sock, np),
>   };
>   
>   static const struct proto_ops smc_inet6_stream_ops = {
> --

Jeongjun Park Aug. 13, 2024, 11:48 a.m. UTC | #3

D. Wythe wrote:
>
>
>
> On 8/13/24 6:07 PM, Jeongjun Park wrote:
> > Since smc_inet6_prot does not initialize ipv6_pinfo_offset, inet6_create()
> > copies an incorrect address value, sk + 0 (offset), to inet_sk(sk)->pinet6.
> >
> > In addition, since inet_sk(sk)->pinet6 and smc_sk(sk)->clcsock practically
> > point to the same address, when smc_create_clcsk() stores the newly
> > created clcsock in smc_sk(sk)->clcsock, inet_sk(sk)->pinet6 is corrupted
> > into clcsock. This causes NULL pointer dereference and various other
> > memory corruptions.
> >
> > To solve this, we need to add a smc6_sock structure for ipv6_pinfo_offset
> > initialization and modify the smc_sock structure.
> >
> > Reported-by: [e-mail]
> > Tested-by: [e-mail]
> > Fixes: d25a92ccae6b ("net/smc: Introduce IPPROTO_SMC")
> > Signed-off-by: Jeongjun Park <[e-mail]>
> > ---
> >   net/smc/smc.h      | 19 ++++++++++---------
> >   net/smc/smc_inet.c | 24 +++++++++++++++---------
> >   2 files changed, 25 insertions(+), 18 deletions(-)
> >
> > diff --git a/net/smc/smc.h b/net/smc/smc.h
> > index 34b781e463c4..f4d9338b5ed5 100644
> > --- a/net/smc/smc.h
> > +++ b/net/smc/smc.h
> > @@ -284,15 +284,6 @@ struct smc_connection {
> >
> >   struct smc_sock {                           /* smc sock container */
> >       struct sock             sk;
> > -     struct socket           *clcsock;       /* internal tcp socket */
> > -     void                    (*clcsk_state_change)(struct sock *sk);
> > -                                             /* original stat_change fct. */
> > -     void                    (*clcsk_data_ready)(struct sock *sk);
> > -                                             /* original data_ready fct. */
> > -     void                    (*clcsk_write_space)(struct sock *sk);
> > -                                             /* original write_space fct. */
> > -     void                    (*clcsk_error_report)(struct sock *sk);
> > -                                             /* original error_report fct. */
> >       struct smc_connection   conn;           /* smc connection */
> >       struct smc_sock         *listen_smc;    /* listen parent */
> >       struct work_struct      connect_work;   /* handle non-blocking connect*/
> > @@ -325,6 +316,16 @@ struct smc_sock {                                /* smc sock container */
> >                                               /* protects clcsock of a listen
> >                                                * socket
> >                                                * */
> > +     struct socket           *clcsock;       /* internal tcp socket */
> > +     void                    (*clcsk_state_change)(struct sock *sk);
> > +                                             /* original stat_change fct. */
> > +     void                    (*clcsk_data_ready)(struct sock *sk);
> > +                                             /* original data_ready fct. */
> > +     void                    (*clcsk_write_space)(struct sock *sk);
> > +                                             /* original write_space fct. */
> > +     void                    (*clcsk_error_report)(struct sock *sk);
> > +                                             /* original error_report fct. */
> > +
> >   };
> >
> >   #define smc_sk(ptr) container_of_const(ptr, struct smc_sock, sk)
> > diff --git a/net/smc/smc_inet.c b/net/smc/smc_inet.c
> > index bece346dd8e9..25f34fd65e8d 100644
> > --- a/net/smc/smc_inet.c
> > +++ b/net/smc/smc_inet.c
> > @@ -60,16 +60,22 @@ static struct inet_protosw smc_inet_protosw = {
> >   };
> >
> >   #if IS_ENABLED(CONFIG_IPV6)
> > +struct smc6_sock {
> > +     struct smc_sock smc;
> > +     struct ipv6_pinfo np;
> > +};
>
> I prefer to:
>
> struct ipv6_pinfo inet6;

Okay, I'll write a v4 patch and send it to you tomorrow.

Regards,
Jeongjun Park

>
> > +
> >   static struct proto smc_inet6_prot = {
> > -     .name           = "INET6_SMC",
> > -     .owner          = THIS_MODULE,
> > -     .init           = smc_inet_init_sock,
> > -     .hash           = smc_hash_sk,
> > -     .unhash         = smc_unhash_sk,
> > -     .release_cb     = smc_release_cb,
> > -     .obj_size       = sizeof(struct smc_sock),
> > -     .h.smc_hash     = &smc_v6_hashinfo,
> > -     .slab_flags     = SLAB_TYPESAFE_BY_RCU,
> > +     .name                           = "INET6_SMC",
> > +     .owner                          = THIS_MODULE,
> > +     .init                           = smc_inet_init_sock,
> > +     .hash                           = smc_hash_sk,
> > +     .unhash                         = smc_unhash_sk,
> > +     .release_cb                     = smc_release_cb,
> > +     .obj_size                       = sizeof(struct smc6_sock),
> > +     .h.smc_hash                     = &smc_v6_hashinfo,
> > +     .slab_flags                     = SLAB_TYPESAFE_BY_RCU,
> > +     .ipv6_pinfo_offset              = offsetof(struct smc6_sock, np),
> >   };
> >
> >   static const struct proto_ops smc_inet6_stream_ops = {
> > --
>

D. Wythe Aug. 14, 2024, 2:25 a.m. UTC | #4

On 8/13/24 7:48 PM, Jeongjun Park wrote:
> D. Wythe wrote:
>>
>>
>> On 8/13/24 6:07 PM, Jeongjun Park wrote:
>>> Since smc_inet6_prot does not initialize ipv6_pinfo_offset, inet6_create()
>>> copies an incorrect address value, sk + 0 (offset), to inet_sk(sk)->pinet6.
>>>
>>> In addition, since inet_sk(sk)->pinet6 and smc_sk(sk)->clcsock practically
>>> point to the same address, when smc_create_clcsk() stores the newly
>>> created clcsock in smc_sk(sk)->clcsock, inet_sk(sk)->pinet6 is corrupted
>>> into clcsock. This causes NULL pointer dereference and various other
>>> memory corruptions.
>>>
>>> To solve this, we need to add a smc6_sock structure for ipv6_pinfo_offset
>>> initialization and modify the smc_sock structure.
>>>
>>> Reported-by: [e-mail]
>>> Tested-by: [e-mail]
>>> Fixes: d25a92ccae6b ("net/smc: Introduce IPPROTO_SMC")
>>> Signed-off-by: Jeongjun Park <[e-mail]>
>>> ---
>>>    net/smc/smc.h      | 19 ++++++++++---------
>>>    net/smc/smc_inet.c | 24 +++++++++++++++---------
>>>    2 files changed, 25 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/net/smc/smc.h b/net/smc/smc.h
>>> index 34b781e463c4..f4d9338b5ed5 100644
>>> --- a/net/smc/smc.h
>>> +++ b/net/smc/smc.h
>>> @@ -284,15 +284,6 @@ struct smc_connection {
>>>
>>>    struct smc_sock {                           /* smc sock container */
>>>        struct sock             sk;
>>> -     struct socket           *clcsock;       /* internal tcp socket */
>>> -     void                    (*clcsk_state_change)(struct sock *sk);
>>> -                                             /* original stat_change fct. */
>>> -     void                    (*clcsk_data_ready)(struct sock *sk);
>>> -                                             /* original data_ready fct. */
>>> -     void                    (*clcsk_write_space)(struct sock *sk);
>>> -                                             /* original write_space fct. */
>>> -     void                    (*clcsk_error_report)(struct sock *sk);
>>> -                                             /* original error_report fct. */
>>>        struct smc_connection   conn;           /* smc connection */
>>>        struct smc_sock         *listen_smc;    /* listen parent */
>>>        struct work_struct      connect_work;   /* handle non-blocking connect*/
>>> @@ -325,6 +316,16 @@ struct smc_sock {                                /* smc sock container */
>>>                                                /* protects clcsock of a listen
>>>                                                 * socket
>>>                                                 * */
>>> +     struct socket           *clcsock;       /* internal tcp socket */
>>> +     void                    (*clcsk_state_change)(struct sock *sk);
>>> +                                             /* original stat_change fct. */
>>> +     void                    (*clcsk_data_ready)(struct sock *sk);
>>> +                                             /* original data_ready fct. */
>>> +     void                    (*clcsk_write_space)(struct sock *sk);
>>> +                                             /* original write_space fct. */
>>> +     void                    (*clcsk_error_report)(struct sock *sk);
>>> +                                             /* original error_report fct. */
>>> +
>>>    };
>>>
>>>    #define smc_sk(ptr) container_of_const(ptr, struct smc_sock, sk)
>>> diff --git a/net/smc/smc_inet.c b/net/smc/smc_inet.c
>>> index bece346dd8e9..25f34fd65e8d 100644
>>> --- a/net/smc/smc_inet.c
>>> +++ b/net/smc/smc_inet.c
>>> @@ -60,16 +60,22 @@ static struct inet_protosw smc_inet_protosw = {
>>>    };
>>>
>>>    #if IS_ENABLED(CONFIG_IPV6)
>>> +struct smc6_sock {
>>> +     struct smc_sock smc;
>>> +     struct ipv6_pinfo np;
>>> +};
>> I prefer to:
>>
>> struct ipv6_pinfo inet6;
> Okay, I'll write a v4 patch and send it to you tomorrow.
>
> Regards,
> Jeongjun Park

Before you issue the v4, I still don't know why you move clcsk_xxx from 
smc_connection
to smc_sock, can you explain it ?

Also, regarding alignment, it's okay for me whether it's aligned or 
not，But I checked the styles of other types of
structures and did not strictly require alignment, so I now feel that 
there is no need to
modify so much to do alignment.

D. Wythe

>
>>> +
>>>    static struct proto smc_inet6_prot = {
>>> -     .name           = "INET6_SMC",
>>> -     .owner          = THIS_MODULE,
>>> -     .init           = smc_inet_init_sock,
>>> -     .hash           = smc_hash_sk,
>>> -     .unhash         = smc_unhash_sk,
>>> -     .release_cb     = smc_release_cb,
>>> -     .obj_size       = sizeof(struct smc_sock),
>>> -     .h.smc_hash     = &smc_v6_hashinfo,
>>> -     .slab_flags     = SLAB_TYPESAFE_BY_RCU,
>>> +     .name                           = "INET6_SMC",
>>> +     .owner                          = THIS_MODULE,
>>> +     .init                           = smc_inet_init_sock,
>>> +     .hash                           = smc_hash_sk,
>>> +     .unhash                         = smc_unhash_sk,
>>> +     .release_cb                     = smc_release_cb,
>>> +     .obj_size                       = sizeof(struct smc6_sock),
>>> +     .h.smc_hash                     = &smc_v6_hashinfo,
>>> +     .slab_flags                     = SLAB_TYPESAFE_BY_RCU,
>>> +     .ipv6_pinfo_offset              = offsetof(struct smc6_sock, np),
>>>    };
>>>
>>>    static const struct proto_ops smc_inet6_stream_ops = {
>>> --

D. Wythe Aug. 14, 2024, 2:42 a.m. UTC | #5

On 8/14/24 10:25 AM, D. Wythe wrote:
>
>
> On 8/13/24 7:48 PM, Jeongjun Park wrote:
>> D. Wythe wrote:
>>>
>>>
>>> On 8/13/24 6:07 PM, Jeongjun Park wrote:
>>>> Since smc_inet6_prot does not initialize ipv6_pinfo_offset, 
>>>> inet6_create()
>>>> copies an incorrect address value, sk + 0 (offset), to 
>>>> inet_sk(sk)->pinet6.
>>>>
>>>> In addition, since inet_sk(sk)->pinet6 and smc_sk(sk)->clcsock 
>>>> practically
>>>> point to the same address, when smc_create_clcsk() stores the newly
>>>> created clcsock in smc_sk(sk)->clcsock, inet_sk(sk)->pinet6 is 
>>>> corrupted
>>>> into clcsock. This causes NULL pointer dereference and various other
>>>> memory corruptions.
>>>>
>>>> To solve this, we need to add a smc6_sock structure for 
>>>> ipv6_pinfo_offset
>>>> initialization and modify the smc_sock structure.
>>>>
>>>> Reported-by: [e-mail]
>>>> Tested-by: [e-mail]
>>>> Fixes: d25a92ccae6b ("net/smc: Introduce IPPROTO_SMC")
>>>> Signed-off-by: Jeongjun Park <[e-mail]>
>>>> ---
>>>>    net/smc/smc.h      | 19 ++++++++++---------
>>>>    net/smc/smc_inet.c | 24 +++++++++++++++---------
>>>>    2 files changed, 25 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/net/smc/smc.h b/net/smc/smc.h
>>>> index 34b781e463c4..f4d9338b5ed5 100644
>>>> --- a/net/smc/smc.h
>>>> +++ b/net/smc/smc.h
>>>> @@ -284,15 +284,6 @@ struct smc_connection {
>>>>
>>>>    struct smc_sock {                           /* smc sock 
>>>> container */
>>>>        struct sock             sk;
>>>> -     struct socket           *clcsock;       /* internal tcp 
>>>> socket */
>>>> -     void                    (*clcsk_state_change)(struct sock *sk);
>>>> -                                             /* original 
>>>> stat_change fct. */
>>>> -     void                    (*clcsk_data_ready)(struct sock *sk);
>>>> -                                             /* original 
>>>> data_ready fct. */
>>>> -     void                    (*clcsk_write_space)(struct sock *sk);
>>>> -                                             /* original 
>>>> write_space fct. */
>>>> -     void                    (*clcsk_error_report)(struct sock *sk);
>>>> -                                             /* original 
>>>> error_report fct. */
>>>>        struct smc_connection   conn;           /* smc connection */
>>>>        struct smc_sock         *listen_smc;    /* listen parent */
>>>>        struct work_struct      connect_work;   /* handle 
>>>> non-blocking connect*/
>>>> @@ -325,6 +316,16 @@ struct smc_sock 
>>>> {                                /* smc sock container */
>>>>                                                /* protects clcsock 
>>>> of a listen
>>>>                                                 * socket
>>>>                                                 * */
>>>> +     struct socket           *clcsock;       /* internal tcp 
>>>> socket */
>>>> +     void                    (*clcsk_state_change)(struct sock *sk);
>>>> +                                             /* original 
>>>> stat_change fct. */
>>>> +     void                    (*clcsk_data_ready)(struct sock *sk);
>>>> +                                             /* original 
>>>> data_ready fct. */
>>>> +     void                    (*clcsk_write_space)(struct sock *sk);
>>>> +                                             /* original 
>>>> write_space fct. */
>>>> +     void                    (*clcsk_error_report)(struct sock *sk);
>>>> +                                             /* original 
>>>> error_report fct. */
>>>> +
>>>>    };
>>>>
>>>>    #define smc_sk(ptr) container_of_const(ptr, struct smc_sock, sk)
>>>> diff --git a/net/smc/smc_inet.c b/net/smc/smc_inet.c
>>>> index bece346dd8e9..25f34fd65e8d 100644
>>>> --- a/net/smc/smc_inet.c
>>>> +++ b/net/smc/smc_inet.c
>>>> @@ -60,16 +60,22 @@ static struct inet_protosw smc_inet_protosw = {
>>>>    };
>>>>
>>>>    #if IS_ENABLED(CONFIG_IPV6)
>>>> +struct smc6_sock {
>>>> +     struct smc_sock smc;
>>>> +     struct ipv6_pinfo np;
>>>> +};
>>> I prefer to:
>>>
>>> struct ipv6_pinfo inet6;
>> Okay, I'll write a v4 patch and send it to you tomorrow.
>>
>> Regards,
>> Jeongjun Park
>
> Before you issue the v4, I still don't know why you move clcsk_xxx 
> from smc_connection
> to smc_sock, can you explain it ?


I misread it, it seems you're moving them from head to tail, but still, 
the same question,
why move it ?

Thanks
D. Wythe


>
> Also, regarding alignment, it's okay for me whether it's aligned or 
> not，But I checked the styles of other types of
> structures and did not strictly require alignment, so I now feel that 
> there is no need to
> modify so much to do alignment.
>
> D. Wythe



>
>>
>>>> +
>>>>    static struct proto smc_inet6_prot = {
>>>> -     .name           = "INET6_SMC",
>>>> -     .owner          = THIS_MODULE,
>>>> -     .init           = smc_inet_init_sock,
>>>> -     .hash           = smc_hash_sk,
>>>> -     .unhash         = smc_unhash_sk,
>>>> -     .release_cb     = smc_release_cb,
>>>> -     .obj_size       = sizeof(struct smc_sock),
>>>> -     .h.smc_hash     = &smc_v6_hashinfo,
>>>> -     .slab_flags     = SLAB_TYPESAFE_BY_RCU,
>>>> +     .name                           = "INET6_SMC",
>>>> +     .owner                          = THIS_MODULE,
>>>> +     .init                           = smc_inet_init_sock,
>>>> +     .hash                           = smc_hash_sk,
>>>> +     .unhash                         = smc_unhash_sk,
>>>> +     .release_cb                     = smc_release_cb,
>>>> +     .obj_size                       = sizeof(struct smc6_sock),
>>>> +     .h.smc_hash                     = &smc_v6_hashinfo,
>>>> +     .slab_flags                     = SLAB_TYPESAFE_BY_RCU,
>>>> +     .ipv6_pinfo_offset              = offsetof(struct smc6_sock, 
>>>> np),
>>>>    };
>>>>
>>>>    static const struct proto_ops smc_inet6_stream_ops = {
>>>> -- 
>

Jeongjun Park Aug. 14, 2024, 3:58 a.m. UTC | #6

Because clcsk_*, like clcsock, is initialized during the smc init process, 
the code was moved to prevent clcsk_* from having an address like 
inet_sk(sk)->pinet6, thereby preventing the previously initialized values 
​​from being tampered with.

Additionally, if you don't need alignment in smc_inet6_prot , I'll modify 
the patch to only add the necessary code without alignment.

Regards,
Jeongjun Park

>
>
> >
> > Also, regarding alignment, it's okay for me whether it's aligned or
> > not，But I checked the styles of other types of
> > structures and did not strictly require alignment, so I now feel that
> > there is no need to
> > modify so much to do alignment.
> >
> > D. Wythe
>
>
>
> >
> >>
> >>>> +
> >>>>    static struct proto smc_inet6_prot = {
> >>>> -     .name           = "INET6_SMC",
> >>>> -     .owner          = THIS_MODULE,
> >>>> -     .init           = smc_inet_init_sock,
> >>>> -     .hash           = smc_hash_sk,
> >>>> -     .unhash         = smc_unhash_sk,
> >>>> -     .release_cb     = smc_release_cb,
> >>>> -     .obj_size       = sizeof(struct smc_sock),
> >>>> -     .h.smc_hash     = &smc_v6_hashinfo,
> >>>> -     .slab_flags     = SLAB_TYPESAFE_BY_RCU,
> >>>> +     .name                           = "INET6_SMC",
> >>>> +     .owner                          = THIS_MODULE,
> >>>> +     .init                           = smc_inet_init_sock,
> >>>> +     .hash                           = smc_hash_sk,
> >>>> +     .unhash                         = smc_unhash_sk,
> >>>> +     .release_cb                     = smc_release_cb,
> >>>> +     .obj_size                       = sizeof(struct smc6_sock),
> >>>> +     .h.smc_hash                     = &smc_v6_hashinfo,
> >>>> +     .slab_flags                     = SLAB_TYPESAFE_BY_RCU,
> >>>> +     .ipv6_pinfo_offset              = offsetof(struct smc6_sock,
> >>>> np),
> >>>>    };
> >>>>
> >>>>    static const struct proto_ops smc_inet6_stream_ops = {
> >>>> --
> >
>

D. Wythe Aug. 14, 2024, 6 a.m. UTC | #7

On 8/14/24 11:58 AM, Jeongjun Park wrote:
> Because clcsk_*, like clcsock, is initialized during the smc init process,
> the code was moved to prevent clcsk_* from having an address like
> inet_sk(sk)->pinet6, thereby preventing the previously initialized values
> ​​from being tampered with.

I don't agree with your approach, but I finally got the problem you 
described. In fact, the issue here is that smc_sock should also be an 
inet_sock, whereas currently it's just a sock. Therefore, the best 
solution would be to embed an inet_sock within smc_sock rather than 
performing this movement as you suggested.

struct smc_sock {               /* smc sock container */
     union {
         struct sock         sk;
         struct inet_sock    inet;
     };

     ...

Thanks.
D. Wythe


>
> Additionally, if you don't need alignment in smc_inet6_prot , I'll modify
> the patch to only add the necessary code without alignment.
>
> Regards,
> Jeongjun Park


>>
>>> Also, regarding alignment, it's okay for me whether it's aligned or
>>> not，But I checked the styles of other types of
>>> structures and did not strictly require alignment, so I now feel that
>>> there is no need to
>>> modify so much to do alignment.
>>>
>>> D. Wythe
>>
>>
>>>>>> +
>>>>>>     static struct proto smc_inet6_prot = {
>>>>>> -     .name           = "INET6_SMC",
>>>>>> -     .owner          = THIS_MODULE,
>>>>>> -     .init           = smc_inet_init_sock,
>>>>>> -     .hash           = smc_hash_sk,
>>>>>> -     .unhash         = smc_unhash_sk,
>>>>>> -     .release_cb     = smc_release_cb,
>>>>>> -     .obj_size       = sizeof(struct smc_sock),
>>>>>> -     .h.smc_hash     = &smc_v6_hashinfo,
>>>>>> -     .slab_flags     = SLAB_TYPESAFE_BY_RCU,
>>>>>> +     .name                           = "INET6_SMC",
>>>>>> +     .owner                          = THIS_MODULE,
>>>>>> +     .init                           = smc_inet_init_sock,
>>>>>> +     .hash                           = smc_hash_sk,
>>>>>> +     .unhash                         = smc_unhash_sk,
>>>>>> +     .release_cb                     = smc_release_cb,
>>>>>> +     .obj_size                       = sizeof(struct smc6_sock),
>>>>>> +     .h.smc_hash                     = &smc_v6_hashinfo,
>>>>>> +     .slab_flags                     = SLAB_TYPESAFE_BY_RCU,
>>>>>> +     .ipv6_pinfo_offset              = offsetof(struct smc6_sock,
>>>>>> np),
>>>>>>     };
>>>>>>
>>>>>>     static const struct proto_ops smc_inet6_stream_ops = {
>>>>>> --

Jeongjun Park Aug. 14, 2024, 7:30 a.m. UTC | #8

> D. Wythe wrote:
> 
> 
> 
>> On 8/14/24 11:58 AM, Jeongjun Park wrote:
>> Because clcsk_*, like clcsock, is initialized during the smc init process,
>> the code was moved to prevent clcsk_* from having an address like
>> inet_sk(sk)->pinet6, thereby preventing the previously initialized values
>> ​​from being tampered with.
> 
> I don't agree with your approach, but I finally got the problem you described. In fact, the issue here is that smc_sock should also be an inet_sock, whereas currently it's just a sock. Therefore, the best solution would be to embed an inet_sock within smc_sock rather than performing this movement as you suggested.
> 
> struct smc_sock {               /* smc sock container */
>     union {
>         struct sock         sk;
>         struct inet_sock    inet;
>     };
> 
>     ...
> 
> Thanks.
> D. Wythe
> 

Wow, I didn't know this could be done this way. I'll test it with that code 
and get back to you

Regards,
Jeongjun Park

> 
>> 
>> Additionally, if you don't need alignment in smc_inet6_prot , I'll modify
>> the patch to only add the necessary code without alignment.
>> 
>> Regards,
>> Jeongjun Park
> 
> 
>>> 
>>>> Also, regarding alignment, it's okay for me whether it's aligned or
>>>> not，But I checked the styles of other types of
>>>> structures and did not strictly require alignment, so I now feel that
>>>> there is no need to
>>>> modify so much to do alignment.
>>>> 
>>>> D. Wythe
>>> 
>>> 
>>>>>>> +
>>>>>>>    static struct proto smc_inet6_prot = {
>>>>>>> -     .name           = "INET6_SMC",
>>>>>>> -     .owner          = THIS_MODULE,
>>>>>>> -     .init           = smc_inet_init_sock,
>>>>>>> -     .hash           = smc_hash_sk,
>>>>>>> -     .unhash         = smc_unhash_sk,
>>>>>>> -     .release_cb     = smc_release_cb,
>>>>>>> -     .obj_size       = sizeof(struct smc_sock),
>>>>>>> -     .h.smc_hash     = &smc_v6_hashinfo,
>>>>>>> -     .slab_flags     = SLAB_TYPESAFE_BY_RCU,
>>>>>>> +     .name                           = "INET6_SMC",
>>>>>>> +     .owner                          = THIS_MODULE,
>>>>>>> +     .init                           = smc_inet_init_sock,
>>>>>>> +     .hash                           = smc_hash_sk,
>>>>>>> +     .unhash                         = smc_unhash_sk,
>>>>>>> +     .release_cb                     = smc_release_cb,
>>>>>>> +     .obj_size                       = sizeof(struct smc6_sock),
>>>>>>> +     .h.smc_hash                     = &smc_v6_hashinfo,
>>>>>>> +     .slab_flags                     = SLAB_TYPESAFE_BY_RCU,
>>>>>>> +     .ipv6_pinfo_offset              = offsetof(struct smc6_sock,
>>>>>>> np),
>>>>>>>    };
>>>>>>> 
>>>>>>>    static const struct proto_ops smc_inet6_stream_ops = {
>>>>>>> --
>

Jeongjun Park Aug. 14, 2024, 10:37 a.m. UTC | #9

2024년 8월 14일 (수) 오후 3:00, D. Wythe <[e-mail]>님이 작성:
>
>
>
> On 8/14/24 11:58 AM, Jeongjun Park wrote:
> > Because clcsk_*, like clcsock, is initialized during the smc init process,
> > the code was moved to prevent clcsk_* from having an address like
> > inet_sk(sk)->pinet6, thereby preventing the previously initialized values
> > from being tampered with.
>
> I don't agree with your approach, but I finally got the problem you
> described. In fact, the issue here is that smc_sock should also be an
> inet_sock, whereas currently it's just a sock. Therefore, the best
> solution would be to embed an inet_sock within smc_sock rather than
> performing this movement as you suggested.
>
> struct smc_sock {               /* smc sock container */
>      union {
>          struct sock         sk;
>          struct inet_sock    inet;
>      };
>
>      ...
>
> Thanks.
> D. Wythe
>

I tested it myself and it doesn't trigger the existing issue, so
I'll write a v4 patch with this code and send it to you.

Regards,
Jeongjun Park

>
> >
> > Additionally, if you don't need alignment in smc_inet6_prot , I'll modify
> > the patch to only add the necessary code without alignment.
> >
> > Regards,
> > Jeongjun Park
>
>
> >>
> >>> Also, regarding alignment, it's okay for me whether it's aligned or
> >>> not，But I checked the styles of other types of
> >>> structures and did not strictly require alignment, so I now feel that
> >>> there is no need to
> >>> modify so much to do alignment.
> >>>
> >>> D. Wythe
> >>
> >>
> >>>>>> +
> >>>>>>     static struct proto smc_inet6_prot = {
> >>>>>> -     .name           = "INET6_SMC",
> >>>>>> -     .owner          = THIS_MODULE,
> >>>>>> -     .init           = smc_inet_init_sock,
> >>>>>> -     .hash           = smc_hash_sk,
> >>>>>> -     .unhash         = smc_unhash_sk,
> >>>>>> -     .release_cb     = smc_release_cb,
> >>>>>> -     .obj_size       = sizeof(struct smc_sock),
> >>>>>> -     .h.smc_hash     = &smc_v6_hashinfo,
> >>>>>> -     .slab_flags     = SLAB_TYPESAFE_BY_RCU,
> >>>>>> +     .name                           = "INET6_SMC",
> >>>>>> +     .owner                          = THIS_MODULE,
> >>>>>> +     .init                           = smc_inet_init_sock,
> >>>>>> +     .hash                           = smc_hash_sk,
> >>>>>> +     .unhash                         = smc_unhash_sk,
> >>>>>> +     .release_cb                     = smc_release_cb,
> >>>>>> +     .obj_size                       = sizeof(struct smc6_sock),
> >>>>>> +     .h.smc_hash                     = &smc_v6_hashinfo,
> >>>>>> +     .slab_flags                     = SLAB_TYPESAFE_BY_RCU,
> >>>>>> +     .ipv6_pinfo_offset              = offsetof(struct smc6_sock,
> >>>>>> np),
> >>>>>>     };
> >>>>>>
> >>>>>>     static const struct proto_ops smc_inet6_stream_ops = {
> >>>>>> --
>

Patch

diff --git a/net/smc/smc.h b/net/smc/smc.h
index 34b781e463c4..f4d9338b5ed5 100644
--- a/net/smc/smc.h
+++ b/net/smc/smc.h
@@ -284,15 +284,6 @@  struct smc_connection {
 
 struct smc_sock {				/* smc sock container */
 	struct sock		sk;
-	struct socket		*clcsock;	/* internal tcp socket */
-	void			(*clcsk_state_change)(struct sock *sk);
-						/* original stat_change fct. */
-	void			(*clcsk_data_ready)(struct sock *sk);
-						/* original data_ready fct. */
-	void			(*clcsk_write_space)(struct sock *sk);
-						/* original write_space fct. */
-	void			(*clcsk_error_report)(struct sock *sk);
-						/* original error_report fct. */
 	struct smc_connection	conn;		/* smc connection */
 	struct smc_sock		*listen_smc;	/* listen parent */
 	struct work_struct	connect_work;	/* handle non-blocking connect*/
@@ -325,6 +316,16 @@  struct smc_sock {				/* smc sock container */
 						/* protects clcsock of a listen
 						 * socket
 						 * */
+	struct socket		*clcsock;	/* internal tcp socket */
+	void			(*clcsk_state_change)(struct sock *sk);
+						/* original stat_change fct. */
+	void			(*clcsk_data_ready)(struct sock *sk);
+						/* original data_ready fct. */
+	void			(*clcsk_write_space)(struct sock *sk);
+						/* original write_space fct. */
+	void			(*clcsk_error_report)(struct sock *sk);
+						/* original error_report fct. */
+
 };
 
 #define smc_sk(ptr) container_of_const(ptr, struct smc_sock, sk)
diff --git a/net/smc/smc_inet.c b/net/smc/smc_inet.c
index bece346dd8e9..25f34fd65e8d 100644
--- a/net/smc/smc_inet.c
+++ b/net/smc/smc_inet.c
@@ -60,16 +60,22 @@  static struct inet_protosw smc_inet_protosw = {
 };
 
 #if IS_ENABLED(CONFIG_IPV6)
+struct smc6_sock {
+	struct smc_sock smc;
+	struct ipv6_pinfo np;
+};
+
 static struct proto smc_inet6_prot = {
-	.name		= "INET6_SMC",
-	.owner		= THIS_MODULE,
-	.init		= smc_inet_init_sock,
-	.hash		= smc_hash_sk,
-	.unhash		= smc_unhash_sk,
-	.release_cb	= smc_release_cb,
-	.obj_size	= sizeof(struct smc_sock),
-	.h.smc_hash	= &smc_v6_hashinfo,
-	.slab_flags	= SLAB_TYPESAFE_BY_RCU,
+	.name				= "INET6_SMC",
+	.owner				= THIS_MODULE,
+	.init				= smc_inet_init_sock,
+	.hash				= smc_hash_sk,
+	.unhash				= smc_unhash_sk,
+	.release_cb			= smc_release_cb,
+	.obj_size			= sizeof(struct smc6_sock),
+	.h.smc_hash			= &smc_v6_hashinfo,
+	.slab_flags			= SLAB_TYPESAFE_BY_RCU,
+	.ipv6_pinfo_offset		= offsetof(struct smc6_sock, np),
 };
 
 static const struct proto_ops smc_inet6_stream_ops = {