D. Wythe wrote:
> On 8/13/24 4:05 PM, Gerd Bayer wrote:
> > On Sun, 2024-08-11 at 02:22 +0900, 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.
> > I can not argue substantially with that... There's very little IPv6
> > testing that I'm aware of. But do you really need to move that much
> > code around and change whitespace for you fix?
> >
> > [--- snip ---]
> >
> >
> >> Fixes: d25a92ccae6b ("net/smc: Introduce IPPROTO_SMC")
> >> Signed-off-by: Jeongjun Park <aha310510@gmail.com>
> >> ---
> >>   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. */
> >> +
> >>   };
>
> Hi Jeongjun,
>
> I have no problem with this fix, thank you for your assistance.
> But, what this here was for ?

Sorry for the confusion. It looks like the tab character was accidentally 
used. We will fix that and send you a v2 patch.

>
>
> >>   
> >>   #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..3c54faef6042 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),
>
> Since you have done alignment, why not align the  '='.
>
> > The line above together with the definition of struct smc6_sock seem to
> > be the only changes relevant to fixing the issue, IMHO.
> >
> >>   };
> >>   
> >>   static const struct proto_ops smc_inet6_stream_ops = {
> >> --
> >>
> > Thanks, Gerd

Regards,
Jeongjun Park