[PATCH net] net/smc: fix general protection fault in __smc_diag_dump

[PATCH net] net/smc: fix general protection fault in __smc_diag_dump

[Wang Liang]

Syzbot reported a general protection fault:

  CPU: 0 UID: 0 PID: 5830 Comm: syz-executor600 Not tainted 6.14.0-rc4-syzkaller-00090-gdd83757f6e68 #0
  RIP: 0010:smc_diag_msg_common_fill net/smc/smc_diag.c:44 [inline]
  RIP: 0010:__smc_diag_dump.constprop.0+0x3de/0x23d0 net/smc/smc_diag.c:89
  Call Trace:
   <TASK>
   smc_diag_dump_proto+0x26d/0x420 net/smc/smc_diag.c:217
   smc_diag_dump+0x84/0x90 net/smc/smc_diag.c:236
   netlink_dump+0x53c/0xd00 net/netlink/af_netlink.c:2318
   __netlink_dump_start+0x6ca/0x970 net/netlink/af_netlink.c:2433
   netlink_dump_start include/linux/netlink.h:340 [inline]
   smc_diag_handler_dump+0x1fb/0x240 net/smc/smc_diag.c:251
   __sock_diag_cmd net/core/sock_diag.c:249 [inline]
   sock_diag_rcv_msg+0x437/0x790 net/core/sock_diag.c:287
   netlink_rcv_skb+0x16b/0x440 net/netlink/af_netlink.c:2543
   netlink_unicast_kernel net/netlink/af_netlink.c:1322 [inline]
   netlink_unicast+0x53c/0x7f0 net/netlink/af_netlink.c:1348
   netlink_sendmsg+0x8b8/0xd70 net/netlink/af_netlink.c:1892
   sock_sendmsg_nosec net/socket.c:718 [inline]
   __sock_sendmsg net/socket.c:733 [inline]
   ____sys_sendmsg+0xaaf/0xc90 net/socket.c:2573
   ___sys_sendmsg+0x135/0x1e0 net/socket.c:2627
   __sys_sendmsg+0x16e/0x220 net/socket.c:2659
   do_syscall_x64 arch/x86/entry/common.c:52 [inline]
   do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
   entry_SYSCALL_64_after_hwframe+0x77/0x7f
   </TASK>

When create smc socket, smc_inet_init_sock() first add sk to the smc_hash
by smc_hash_sk(), then create smc->clcsock. it is possible that, after
smc_diag_dump_proto() traverses the smc_hash, smc->clcsock is not created
when the function visit it.

The process like this:

  (CPU1)                         | (CPU2)
  inet6_create()                 |
    smc_inet_init_sock()         |
      smc_sk_init()              |
        smc_hash_sk()            |
          head = &smc_hash->ht;  |
          sk_add_node(sk, head); |
                                 | smc_diag_dump_proto
                                 |   head = &smc_hash->ht;
                                 |     sk_for_each(sk, head)
                                 |       __smc_diag_dump()
                                 |         visit smc->clcsock
      smc_create_clcsk()         |
          set smc->clcsock       |

Fix this by initialize smc->clcsock to NULL before add sk to smc_hash in
smc_sk_init().

Reported-by: syzbot+271fed3ed6f24600c364@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=271fed3ed6f24600c364
Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets")
Signed-off-by: Wang Liang <wangliang74@huawei.com>
---
 net/smc/af_smc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 3e6cb35baf25..454801188514 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -371,6 +371,7 @@ void smc_sk_init(struct net *net, struct sock *sk, int protocol)
 	sk->sk_protocol = protocol;
 	WRITE_ONCE(sk->sk_sndbuf, 2 * READ_ONCE(net->smc.sysctl_wmem));
 	WRITE_ONCE(sk->sk_rcvbuf, 2 * READ_ONCE(net->smc.sysctl_rmem));
+	smc->clcsock = NULL;
 	INIT_WORK(&smc->tcp_listen_work, smc_tcp_listen_work);
 	INIT_WORK(&smc->connect_work, smc_connect_work);
 	INIT_DELAYED_WORK(&smc->conn.tx_work, smc_tx_work);
-- 
2.34.1

Re: [PATCH net] net/smc: fix general protection fault in __smc_diag_dump

[Paolo Abeni]

On 3/31/25 10:10 AM, Wang Liang wrote:
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index 3e6cb35baf25..454801188514 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -371,6 +371,7 @@ void smc_sk_init(struct net *net, struct sock *sk, int protocol)
>  	sk->sk_protocol = protocol;
>  	WRITE_ONCE(sk->sk_sndbuf, 2 * READ_ONCE(net->smc.sysctl_wmem));
>  	WRITE_ONCE(sk->sk_rcvbuf, 2 * READ_ONCE(net->smc.sysctl_rmem));
> +	smc->clcsock = NULL;
>  	INIT_WORK(&smc->tcp_listen_work, smc_tcp_listen_work);
>  	INIT_WORK(&smc->connect_work, smc_connect_work);
>  	INIT_DELAYED_WORK(&smc->conn.tx_work, smc_tx_work);

The syzkaller report has a few reproducers, have you tested this? AFAICS
the smc socket is already zeroed on allocation by sk_alloc().

/P

Re: [PATCH net] net/smc: fix general protection fault in __smc_diag_dump

[Zhu Yanjun]

On 01.04.25 13:01, Paolo Abeni wrote:
> On 3/31/25 10:10 AM, Wang Liang wrote:
>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>> index 3e6cb35baf25..454801188514 100644
>> --- a/net/smc/af_smc.c
>> +++ b/net/smc/af_smc.c
>> @@ -371,6 +371,7 @@ void smc_sk_init(struct net *net, struct sock *sk, int protocol)
>>   	sk->sk_protocol = protocol;
>>   	WRITE_ONCE(sk->sk_sndbuf, 2 * READ_ONCE(net->smc.sysctl_wmem));
>>   	WRITE_ONCE(sk->sk_rcvbuf, 2 * READ_ONCE(net->smc.sysctl_rmem));
>> +	smc->clcsock = NULL;
>>   	INIT_WORK(&smc->tcp_listen_work, smc_tcp_listen_work);
>>   	INIT_WORK(&smc->connect_work, smc_connect_work);
>>   	INIT_DELAYED_WORK(&smc->conn.tx_work, smc_tx_work);
> 
> The syzkaller report has a few reproducers, have you tested this? AFAICS
> the smc socket is already zeroed on allocation by sk_alloc().

Yes. I also agree with you that smc socket should have already been zeroed.

Currently in this commit, this member variable is set to NULL 
explicitly. I am not sure if this can fix this problem or not.

Based on the following, it seems that this problem can be reproduced.
"
syzbot has tested the proposed patch but the reproducer is still 
triggering an issue:
general protection fault in __smc_diag_dump
"

Thus follow the instructions in this link to make tests.

https://groups.google.com/g/syzkaller-bugs/c/YwENRImdcsk/m/wBJo6qGiCAAJ?pli=1, 
the following can trigger the reproducer.

"
If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.
"

Zhu Yanjun

> 
> /P
> 

Re: [PATCH net] net/smc: fix general protection fault in __smc_diag_dump

[Wang Liang]

在 2025/4/1 19:01, Paolo Abeni 写道:
> On 3/31/25 10:10 AM, Wang Liang wrote:
>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>> index 3e6cb35baf25..454801188514 100644
>> --- a/net/smc/af_smc.c
>> +++ b/net/smc/af_smc.c
>> @@ -371,6 +371,7 @@ void smc_sk_init(struct net *net, struct sock *sk, int protocol)
>>   	sk->sk_protocol = protocol;
>>   	WRITE_ONCE(sk->sk_sndbuf, 2 * READ_ONCE(net->smc.sysctl_wmem));
>>   	WRITE_ONCE(sk->sk_rcvbuf, 2 * READ_ONCE(net->smc.sysctl_rmem));
>> +	smc->clcsock = NULL;
>>   	INIT_WORK(&smc->tcp_listen_work, smc_tcp_listen_work);
>>   	INIT_WORK(&smc->connect_work, smc_connect_work);
>>   	INIT_DELAYED_WORK(&smc->conn.tx_work, smc_tx_work);
> The syzkaller report has a few reproducers, have you tested this? AFAICS
> the smc socket is already zeroed on allocation by sk_alloc().


Yes, I test it by the C repro:
https://syzkaller.appspot.com/text?tag=ReproC&x=13d2dc98580000

The C repro is provided by the 2025/02/27 15:16 crash from
   https://syzkaller.appspot.com/bug?extid=271fed3ed6f24600c364

After apply my patch, the crash no longer happens when running the C repro.

"the smc socket is already zeroed on allocation by sk_alloc()", That is 
right.
However, smc->clcsock may be modified indirectly in inet6_create().
The process like this:

   __sys_socket
     __sys_socket_create
       sock_create
         __sock_create
           # pf->create
           inet6_create
             // init smc->clcsock = 0
             sk = sk_alloc()

             // set smc->clcsock to invalid address
             inet = inet_sk(sk);
             inet_assign_bit(IS_ICSK, sk, INET_PROTOSW_ICSK & answer_flags);
             inet6_set_bit(MC6_LOOP, sk);
             inet6_set_bit(MC6_ALL, sk);

             smc_inet_init_sock
               smc_sk_init
                 // add sk to smc_hash
                 smc_hash_sk
                   sk_add_node(sk, head);
               smc_create_clcsk
                 // set smc->clcsock
                 sock_create_kern(..., &smc->clcsock);)

So initialize smc->clcsock to NULL explicitly in smc_sk_init() can fix
this crash scene. If the problem can be reproduced after this patch, I
guess it is not the same reason, and fix it by another patch is more
appropriate.

>
> /P
>
>

Re: [PATCH net] net/smc: fix general protection fault in __smc_diag_dump

[D. Wythe]

On Wed, Apr 02, 2025 at 10:37:24AM +0800, Wang Liang wrote:
> 
> 在 2025/4/1 19:01, Paolo Abeni 写道:
> >On 3/31/25 10:10 AM, Wang Liang wrote:
> >>diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> >>index 3e6cb35baf25..454801188514 100644
> >>--- a/net/smc/af_smc.c
> >>+++ b/net/smc/af_smc.c
> >>@@ -371,6 +371,7 @@ void smc_sk_init(struct net *net, struct sock *sk, int protocol)
> >>  	sk->sk_protocol = protocol;
> >>  	WRITE_ONCE(sk->sk_sndbuf, 2 * READ_ONCE(net->smc.sysctl_wmem));
> >>  	WRITE_ONCE(sk->sk_rcvbuf, 2 * READ_ONCE(net->smc.sysctl_rmem));
> >>+	smc->clcsock = NULL;
> >>  	INIT_WORK(&smc->tcp_listen_work, smc_tcp_listen_work);
> >>  	INIT_WORK(&smc->connect_work, smc_connect_work);
> >>  	INIT_DELAYED_WORK(&smc->conn.tx_work, smc_tx_work);
> >The syzkaller report has a few reproducers, have you tested this? AFAICS
> >the smc socket is already zeroed on allocation by sk_alloc().
> 
> 
> Yes, I test it by the C repro:
> https://syzkaller.appspot.com/text?tag=ReproC&x=13d2dc98580000
> 
> The C repro is provided by the 2025/02/27 15:16 crash from
>   https://syzkaller.appspot.com/bug?extid=271fed3ed6f24600c364
> 
> After apply my patch, the crash no longer happens when running the C repro.
> 
> "the smc socket is already zeroed on allocation by sk_alloc()", That
> is right.
> However, smc->clcsock may be modified indirectly in inet6_create().
> The process like this:
> 
>   __sys_socket
>     __sys_socket_create
>       sock_create
>         __sock_create
>           # pf->create
>           inet6_create
>             // init smc->clcsock = 0
>             sk = sk_alloc()
> 
>             // set smc->clcsock to invalid address
>             inet = inet_sk(sk);
>             inet_assign_bit(IS_ICSK, sk, INET_PROTOSW_ICSK & answer_flags);
>             inet6_set_bit(MC6_LOOP, sk);
>             inet6_set_bit(MC6_ALL, sk);
> 
>             smc_inet_init_sock
>               smc_sk_init
>                 // add sk to smc_hash
>                 smc_hash_sk
>                   sk_add_node(sk, head);
>               smc_create_clcsk
>                 // set smc->clcsock
>                 sock_create_kern(..., &smc->clcsock);)
> 
> So initialize smc->clcsock to NULL explicitly in smc_sk_init() can fix
> this crash scene. If the problem can be reproduced after this patch, I
> guess it is not the same reason, and fix it by another patch is more
> appropriate.
> 

This is actually because the current smc_sock is not an inet_sock,
leading to two modules simultaneously modifying the same offset in
memory but interpreting its structure differently. I previously proposed
embedding an inet(6)_sock at the beginning of smc_sock, but the
community had some objections...

I'm not sure on the community's current stance on this matter, but if a
fix is absolutely necessary, my recommendation would still be to embed
an inet(6)_sock within the smc_sock structure

D.

> >



> >/P
> >
> >

Re: [PATCH net] net/smc: fix general protection fault in __smc_diag_dump

[Wang Liang]

在 2025/4/2 15:20, D. Wythe 写道:
> On Wed, Apr 02, 2025 at 10:37:24AM +0800, Wang Liang wrote:
>> 在 2025/4/1 19:01, Paolo Abeni 写道:
>>> On 3/31/25 10:10 AM, Wang Liang wrote:
>>>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>>>> index 3e6cb35baf25..454801188514 100644
>>>> --- a/net/smc/af_smc.c
>>>> +++ b/net/smc/af_smc.c
>>>> @@ -371,6 +371,7 @@ void smc_sk_init(struct net *net, struct sock *sk, int protocol)
>>>>   	sk->sk_protocol = protocol;
>>>>   	WRITE_ONCE(sk->sk_sndbuf, 2 * READ_ONCE(net->smc.sysctl_wmem));
>>>>   	WRITE_ONCE(sk->sk_rcvbuf, 2 * READ_ONCE(net->smc.sysctl_rmem));
>>>> +	smc->clcsock = NULL;
>>>>   	INIT_WORK(&smc->tcp_listen_work, smc_tcp_listen_work);
>>>>   	INIT_WORK(&smc->connect_work, smc_connect_work);
>>>>   	INIT_DELAYED_WORK(&smc->conn.tx_work, smc_tx_work);
>>> The syzkaller report has a few reproducers, have you tested this? AFAICS
>>> the smc socket is already zeroed on allocation by sk_alloc().
>>
>> Yes, I test it by the C repro:
>> https://syzkaller.appspot.com/text?tag=ReproC&x=13d2dc98580000
>>
>> The C repro is provided by the 2025/02/27 15:16 crash from
>>    https://syzkaller.appspot.com/bug?extid=271fed3ed6f24600c364
>>
>> After apply my patch, the crash no longer happens when running the C repro.
>>
>> "the smc socket is already zeroed on allocation by sk_alloc()", That
>> is right.
>> However, smc->clcsock may be modified indirectly in inet6_create().
>> The process like this:
>>
>>    __sys_socket
>>      __sys_socket_create
>>        sock_create
>>          __sock_create
>>            # pf->create
>>            inet6_create
>>              // init smc->clcsock = 0
>>              sk = sk_alloc()
>>
>>              // set smc->clcsock to invalid address
>>              inet = inet_sk(sk);
>>              inet_assign_bit(IS_ICSK, sk, INET_PROTOSW_ICSK & answer_flags);
>>              inet6_set_bit(MC6_LOOP, sk);
>>              inet6_set_bit(MC6_ALL, sk);
>>
>>              smc_inet_init_sock
>>                smc_sk_init
>>                  // add sk to smc_hash
>>                  smc_hash_sk
>>                    sk_add_node(sk, head);
>>                smc_create_clcsk
>>                  // set smc->clcsock
>>                  sock_create_kern(..., &smc->clcsock);)
>>
>> So initialize smc->clcsock to NULL explicitly in smc_sk_init() can fix
>> this crash scene. If the problem can be reproduced after this patch, I
>> guess it is not the same reason, and fix it by another patch is more
>> appropriate.
>>
> This is actually because the current smc_sock is not an inet_sock,
> leading to two modules simultaneously modifying the same offset in
> memory but interpreting its structure differently. I previously proposed
> embedding an inet(6)_sock at the beginning of smc_sock, but the
> community had some objections...
>
> I'm not sure on the community's current stance on this matter, but if a
> fix is absolutely necessary, my recommendation would still be to embed
> an inet(6)_sock within the smc_sock structure
>
> D.

At present, I think initializing the smc in smc_sk_init() may be the 
most simple and effective method. :P

>
>>> /P
>>>
>>>

Re: [PATCH net] net/smc: fix general protection fault in __smc_diag_dump

[Wenjia Zhang]

On 31.03.25 10:10, Wang Liang wrote:
> Syzbot reported a general protection fault:
> 
>    CPU: 0 UID: 0 PID: 5830 Comm: syz-executor600 Not tainted 6.14.0-rc4-syzkaller-00090-gdd83757f6e68 #0
>    RIP: 0010:smc_diag_msg_common_fill net/smc/smc_diag.c:44 [inline]
>    RIP: 0010:__smc_diag_dump.constprop.0+0x3de/0x23d0 net/smc/smc_diag.c:89
>    Call Trace:
>     <TASK>
>     smc_diag_dump_proto+0x26d/0x420 net/smc/smc_diag.c:217
>     smc_diag_dump+0x84/0x90 net/smc/smc_diag.c:236
>     netlink_dump+0x53c/0xd00 net/netlink/af_netlink.c:2318
>     __netlink_dump_start+0x6ca/0x970 net/netlink/af_netlink.c:2433
>     netlink_dump_start include/linux/netlink.h:340 [inline]
>     smc_diag_handler_dump+0x1fb/0x240 net/smc/smc_diag.c:251
>     __sock_diag_cmd net/core/sock_diag.c:249 [inline]
>     sock_diag_rcv_msg+0x437/0x790 net/core/sock_diag.c:287
>     netlink_rcv_skb+0x16b/0x440 net/netlink/af_netlink.c:2543
>     netlink_unicast_kernel net/netlink/af_netlink.c:1322 [inline]
>     netlink_unicast+0x53c/0x7f0 net/netlink/af_netlink.c:1348
>     netlink_sendmsg+0x8b8/0xd70 net/netlink/af_netlink.c:1892
>     sock_sendmsg_nosec net/socket.c:718 [inline]
>     __sock_sendmsg net/socket.c:733 [inline]
>     ____sys_sendmsg+0xaaf/0xc90 net/socket.c:2573
>     ___sys_sendmsg+0x135/0x1e0 net/socket.c:2627
>     __sys_sendmsg+0x16e/0x220 net/socket.c:2659
>     do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>     do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
>     entry_SYSCALL_64_after_hwframe+0x77/0x7f
>     </TASK>
> 
> When create smc socket, smc_inet_init_sock() first add sk to the smc_hash
> by smc_hash_sk(), then create smc->clcsock. it is possible that, after
> smc_diag_dump_proto() traverses the smc_hash, smc->clcsock is not created
> when the function visit it.
> 
> The process like this:
> 
>    (CPU1)                         | (CPU2)
>    inet6_create()                 |
>      smc_inet_init_sock()         |
>        smc_sk_init()              |
>          smc_hash_sk()            |
>            head = &smc_hash->ht;  |
>            sk_add_node(sk, head); |
>                                   | smc_diag_dump_proto
>                                   |   head = &smc_hash->ht;
>                                   |     sk_for_each(sk, head)
>                                   |       __smc_diag_dump()
>                                   |         visit smc->clcsock
>        smc_create_clcsk()         |
>            set smc->clcsock       |
> 
> Fix this by initialize smc->clcsock to NULL before add sk to smc_hash in
> smc_sk_init().
> 
> Reported-by: syzbot+271fed3ed6f24600c364@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=271fed3ed6f24600c364
> Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets")
> Signed-off-by: Wang Liang <wangliang74@huawei.com>
> ---
>   net/smc/af_smc.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index 3e6cb35baf25..454801188514 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -371,6 +371,7 @@ void smc_sk_init(struct net *net, struct sock *sk, int protocol)
>   	sk->sk_protocol = protocol;
>   	WRITE_ONCE(sk->sk_sndbuf, 2 * READ_ONCE(net->smc.sysctl_wmem));
>   	WRITE_ONCE(sk->sk_rcvbuf, 2 * READ_ONCE(net->smc.sysctl_rmem));
> +	smc->clcsock = NULL;
>   	INIT_WORK(&smc->tcp_listen_work, smc_tcp_listen_work);
>   	INIT_WORK(&smc->connect_work, smc_connect_work);
>   	INIT_DELAYED_WORK(&smc->conn.tx_work, smc_tx_work);

I have to agree with this workaround, even though I see that is not the 
best solution. Thus, I'd like to give my R-b:

Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com>

Btw. @D. Wythe, would you mind sending me the link of your proposal you 
mentioned please? Let me have a look. It seems like I missed it.

Thanks,
Wenjia

Re: [PATCH net] net/smc: fix general protection fault in __smc_diag_dump

[Wang Liang]

在 2025/4/3 19:55, Wenjia Zhang 写道:
>
>
> On 31.03.25 10:10, Wang Liang wrote:
>> Syzbot reported a general protection fault:
>>
>>    CPU: 0 UID: 0 PID: 5830 Comm: syz-executor600 Not tainted 
>> 6.14.0-rc4-syzkaller-00090-gdd83757f6e68 #0
>>    RIP: 0010:smc_diag_msg_common_fill net/smc/smc_diag.c:44 [inline]
>>    RIP: 0010:__smc_diag_dump.constprop.0+0x3de/0x23d0 
>> net/smc/smc_diag.c:89
>>    Call Trace:
>>     <TASK>
>>     smc_diag_dump_proto+0x26d/0x420 net/smc/smc_diag.c:217
>>     smc_diag_dump+0x84/0x90 net/smc/smc_diag.c:236
>>     netlink_dump+0x53c/0xd00 net/netlink/af_netlink.c:2318
>>     __netlink_dump_start+0x6ca/0x970 net/netlink/af_netlink.c:2433
>>     netlink_dump_start include/linux/netlink.h:340 [inline]
>>     smc_diag_handler_dump+0x1fb/0x240 net/smc/smc_diag.c:251
>>     __sock_diag_cmd net/core/sock_diag.c:249 [inline]
>>     sock_diag_rcv_msg+0x437/0x790 net/core/sock_diag.c:287
>>     netlink_rcv_skb+0x16b/0x440 net/netlink/af_netlink.c:2543
>>     netlink_unicast_kernel net/netlink/af_netlink.c:1322 [inline]
>>     netlink_unicast+0x53c/0x7f0 net/netlink/af_netlink.c:1348
>>     netlink_sendmsg+0x8b8/0xd70 net/netlink/af_netlink.c:1892
>>     sock_sendmsg_nosec net/socket.c:718 [inline]
>>     __sock_sendmsg net/socket.c:733 [inline]
>>     ____sys_sendmsg+0xaaf/0xc90 net/socket.c:2573
>>     ___sys_sendmsg+0x135/0x1e0 net/socket.c:2627
>>     __sys_sendmsg+0x16e/0x220 net/socket.c:2659
>>     do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>>     do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
>>     entry_SYSCALL_64_after_hwframe+0x77/0x7f
>>     </TASK>
>>
>> When create smc socket, smc_inet_init_sock() first add sk to the 
>> smc_hash
>> by smc_hash_sk(), then create smc->clcsock. it is possible that, after
>> smc_diag_dump_proto() traverses the smc_hash, smc->clcsock is not 
>> created
>> when the function visit it.
>>
>> The process like this:
>>
>>    (CPU1)                         | (CPU2)
>>    inet6_create()                 |
>>      smc_inet_init_sock()         |
>>        smc_sk_init()              |
>>          smc_hash_sk()            |
>>            head = &smc_hash->ht;  |
>>            sk_add_node(sk, head); |
>>                                   | smc_diag_dump_proto
>>                                   |   head = &smc_hash->ht;
>>                                   |     sk_for_each(sk, head)
>>                                   |       __smc_diag_dump()
>>                                   |         visit smc->clcsock
>>        smc_create_clcsk()         |
>>            set smc->clcsock       |
>>
>> Fix this by initialize smc->clcsock to NULL before add sk to smc_hash in
>> smc_sk_init().
>>
>> Reported-by: syzbot+271fed3ed6f24600c364@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=271fed3ed6f24600c364
>> Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets")
>> Signed-off-by: Wang Liang <wangliang74@huawei.com>
>> ---
>>   net/smc/af_smc.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>> index 3e6cb35baf25..454801188514 100644
>> --- a/net/smc/af_smc.c
>> +++ b/net/smc/af_smc.c
>> @@ -371,6 +371,7 @@ void smc_sk_init(struct net *net, struct sock 
>> *sk, int protocol)
>>       sk->sk_protocol = protocol;
>>       WRITE_ONCE(sk->sk_sndbuf, 2 * READ_ONCE(net->smc.sysctl_wmem));
>>       WRITE_ONCE(sk->sk_rcvbuf, 2 * READ_ONCE(net->smc.sysctl_rmem));
>> +    smc->clcsock = NULL;
>>       INIT_WORK(&smc->tcp_listen_work, smc_tcp_listen_work);
>>       INIT_WORK(&smc->connect_work, smc_connect_work);
>>       INIT_DELAYED_WORK(&smc->conn.tx_work, smc_tx_work);
>
> I have to agree with this workaround, even though I see that is not 
> the best solution. Thus, I'd like to give my R-b:
>
> Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com>
>
> Btw. @D. Wythe, would you mind sending me the link of your proposal 
> you mentioned please? Let me have a look. It seems like I missed it.
>
> Thanks,
> Wenjia
>

Hello, is this patch rejected?
If there are some new fix patchs, please let me know.
Thanks.

>
>

[PATCH net] net/smc: fix general protection fault in __smc_diag_dump

[Wang Liang]

The syzbot report a crash:

  Oops: general protection fault, probably for non-canonical address 0xfbd5a5d5a0000003: 0000 [#1] SMP KASAN NOPTI
  KASAN: maybe wild-memory-access in range [0xdead4ead00000018-0xdead4ead0000001f]
  CPU: 1 UID: 0 PID: 6949 Comm: syz.0.335 Not tainted syzkaller #0 PREEMPT(full)
  Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/18/2025
  RIP: 0010:smc_diag_msg_common_fill net/smc/smc_diag.c:44 [inline]
  RIP: 0010:__smc_diag_dump.constprop.0+0x3ca/0x2550 net/smc/smc_diag.c:89
  Call Trace:
   <TASK>
   smc_diag_dump_proto+0x26d/0x420 net/smc/smc_diag.c:217
   smc_diag_dump+0x27/0x90 net/smc/smc_diag.c:234
   netlink_dump+0x539/0xd30 net/netlink/af_netlink.c:2327
   __netlink_dump_start+0x6d6/0x990 net/netlink/af_netlink.c:2442
   netlink_dump_start include/linux/netlink.h:341 [inline]
   smc_diag_handler_dump+0x1f9/0x240 net/smc/smc_diag.c:251
   __sock_diag_cmd net/core/sock_diag.c:249 [inline]
   sock_diag_rcv_msg+0x438/0x790 net/core/sock_diag.c:285
   netlink_rcv_skb+0x158/0x420 net/netlink/af_netlink.c:2552
   netlink_unicast_kernel net/netlink/af_netlink.c:1320 [inline]
   netlink_unicast+0x5a7/0x870 net/netlink/af_netlink.c:1346
   netlink_sendmsg+0x8d1/0xdd0 net/netlink/af_netlink.c:1896
   sock_sendmsg_nosec net/socket.c:714 [inline]
   __sock_sendmsg net/socket.c:729 [inline]
   ____sys_sendmsg+0xa95/0xc70 net/socket.c:2614
   ___sys_sendmsg+0x134/0x1d0 net/socket.c:2668
   __sys_sendmsg+0x16d/0x220 net/socket.c:2700
   do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
   do_syscall_64+0xcd/0x4e0 arch/x86/entry/syscall_64.c:94
   entry_SYSCALL_64_after_hwframe+0x77/0x7f
   </TASK>

The process like this:

               (CPU1)              |             (CPU2)
  ---------------------------------|-------------------------------
  inet_create()                    |
    // init clcsock to NULL        |
    sk = sk_alloc()                |
                                   |
    // unexpectedly change clcsock |
    inet_init_csk_locks()          |
                                   |
    // add sk to hash table        |
    smc_inet_init_sock()           |
      smc_sk_init()                |
        smc_hash_sk()              |
                                   | // traverse the hash table
                                   | smc_diag_dump_proto
                                   |   __smc_diag_dump()
                                   |     // visit wrong clcsock
                                   |     smc_diag_msg_common_fill()
    // alloc clcsock               |
    smc_create_clcsk               |
      sock_create_kern             |

With CONFIG_DEBUG_LOCK_ALLOC=y, the smc->clcsock is unexpectedly changed
in inet_init_csk_locks(), because the struct smc_sock does not have struct
inet_connection_sock as the first member.

Previous commit 60ada4fe644e ("smc: Fix various oops due to inet_sock type
confusion.") add inet_sock as the first member of smc_sock. For protocol
with INET_PROTOSW_ICSK, use inet_connection_sock instead of inet_sock is
more appropriate.

Reported-by: syzbot+f775be4458668f7d220e@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=f775be4458668f7d220e
Tested-by: syzbot+f775be4458668f7d220e@syzkaller.appspotmail.com
Fixes: d25a92ccae6b ("net/smc: Introduce IPPROTO_SMC")
Signed-off-by: Wang Liang <wangliang74@huawei.com>
---
 net/smc/smc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/smc/smc.h b/net/smc/smc.h
index 2c9084963739..1b20f0c927d3 100644
--- a/net/smc/smc.h
+++ b/net/smc/smc.h
@@ -285,7 +285,7 @@ struct smc_connection {
 struct smc_sock {				/* smc sock container */
 	union {
 		struct sock		sk;
-		struct inet_sock	icsk_inet;
+		struct inet_connection_sock	inet_conn;
 	};
 	struct socket		*clcsock;	/* internal tcp socket */
 	void			(*clcsk_state_change)(struct sock *sk);
-- 
2.34.1

Re: [PATCH net] net/smc: fix general protection fault in __smc_diag_dump

[Eric Dumazet]

On Mon, Sep 22, 2025 at 4:57 AM Wang Liang <wangliang74@huawei.com> wrote:
>
> The syzbot report a crash:
>
>   Oops: general protection fault, probably for non-canonical address 0xfbd5a5d5a0000003: 0000 [#1] SMP KASAN NOPTI
>   KASAN: maybe wild-memory-access in range [0xdead4ead00000018-0xdead4ead0000001f]
>   CPU: 1 UID: 0 PID: 6949 Comm: syz.0.335 Not tainted syzkaller #0 PREEMPT(full)
>   Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/18/2025
>   RIP: 0010:smc_diag_msg_common_fill net/smc/smc_diag.c:44 [inline]
>   RIP: 0010:__smc_diag_dump.constprop.0+0x3ca/0x2550 net/smc/smc_diag.c:89
>   Call Trace:
>    <TASK>
>    smc_diag_dump_proto+0x26d/0x420 net/smc/smc_diag.c:217
>    smc_diag_dump+0x27/0x90 net/smc/smc_diag.c:234
>    netlink_dump+0x539/0xd30 net/netlink/af_netlink.c:2327
>    __netlink_dump_start+0x6d6/0x990 net/netlink/af_netlink.c:2442
>    netlink_dump_start include/linux/netlink.h:341 [inline]
>    smc_diag_handler_dump+0x1f9/0x240 net/smc/smc_diag.c:251
>    __sock_diag_cmd net/core/sock_diag.c:249 [inline]
>    sock_diag_rcv_msg+0x438/0x790 net/core/sock_diag.c:285
>    netlink_rcv_skb+0x158/0x420 net/netlink/af_netlink.c:2552
>    netlink_unicast_kernel net/netlink/af_netlink.c:1320 [inline]
>    netlink_unicast+0x5a7/0x870 net/netlink/af_netlink.c:1346
>    netlink_sendmsg+0x8d1/0xdd0 net/netlink/af_netlink.c:1896
>    sock_sendmsg_nosec net/socket.c:714 [inline]
>    __sock_sendmsg net/socket.c:729 [inline]
>    ____sys_sendmsg+0xa95/0xc70 net/socket.c:2614
>    ___sys_sendmsg+0x134/0x1d0 net/socket.c:2668
>    __sys_sendmsg+0x16d/0x220 net/socket.c:2700
>    do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
>    do_syscall_64+0xcd/0x4e0 arch/x86/entry/syscall_64.c:94
>    entry_SYSCALL_64_after_hwframe+0x77/0x7f
>    </TASK>
>
> The process like this:
>
>                (CPU1)              |             (CPU2)
>   ---------------------------------|-------------------------------
>   inet_create()                    |
>     // init clcsock to NULL        |
>     sk = sk_alloc()                |
>                                    |
>     // unexpectedly change clcsock |
>     inet_init_csk_locks()          |
>                                    |
>     // add sk to hash table        |
>     smc_inet_init_sock()           |
>       smc_sk_init()                |
>         smc_hash_sk()              |
>                                    | // traverse the hash table
>                                    | smc_diag_dump_proto
>                                    |   __smc_diag_dump()
>                                    |     // visit wrong clcsock
>                                    |     smc_diag_msg_common_fill()
>     // alloc clcsock               |
>     smc_create_clcsk               |
>       sock_create_kern             |
>
> With CONFIG_DEBUG_LOCK_ALLOC=y, the smc->clcsock is unexpectedly changed
> in inet_init_csk_locks(), because the struct smc_sock does not have struct
> inet_connection_sock as the first member.
>
> Previous commit 60ada4fe644e ("smc: Fix various oops due to inet_sock type
> confusion.") add inet_sock as the first member of smc_sock. For protocol
> with INET_PROTOSW_ICSK, use inet_connection_sock instead of inet_sock is
> more appropriate.
>
> Reported-by: syzbot+f775be4458668f7d220e@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=f775be4458668f7d220e
> Tested-by: syzbot+f775be4458668f7d220e@syzkaller.appspotmail.com
> Fixes: d25a92ccae6b ("net/smc: Introduce IPPROTO_SMC")
> Signed-off-by: Wang Liang <wangliang74@huawei.com>
> ---
>  net/smc/smc.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/smc/smc.h b/net/smc/smc.h
> index 2c9084963739..1b20f0c927d3 100644
> --- a/net/smc/smc.h
> +++ b/net/smc/smc.h
> @@ -285,7 +285,7 @@ struct smc_connection {
>  struct smc_sock {                              /* smc sock container */
>         union {
>                 struct sock             sk;
> -               struct inet_sock        icsk_inet;
> +               struct inet_connection_sock     inet_conn;
>         };
>         struct socket           *clcsock;       /* internal tcp socket */
>         void                    (*clcsk_state_change)(struct sock *sk);
> --
> 2.34.1
>

Kuniyuki, can you please review, I think you had a related fix recently.

Thanks.

commit 60ada4fe644edaa6c2da97364184b0425e8aeaf5
Author: Kuniyuki Iwashima <kuniyu@google.com>
Date:   Fri Jul 11 06:07:52 2025 +0000

    smc: Fix various oops due to inet_sock type confusion.

Re: [PATCH net] net/smc: fix general protection fault in __smc_diag_dump

[Kuniyuki Iwashima]

Thanks Eric for CCing me.

On Thu, Sep 25, 2025 at 7:32 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Mon, Sep 22, 2025 at 4:57 AM Wang Liang <wangliang74@huawei.com> wrote:
> >
> > The syzbot report a crash:
> >
> >   Oops: general protection fault, probably for non-canonical address 0xfbd5a5d5a0000003: 0000 [#1] SMP KASAN NOPTI
> >   KASAN: maybe wild-memory-access in range [0xdead4ead00000018-0xdead4ead0000001f]
> >   CPU: 1 UID: 0 PID: 6949 Comm: syz.0.335 Not tainted syzkaller #0 PREEMPT(full)
> >   Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/18/2025
> >   RIP: 0010:smc_diag_msg_common_fill net/smc/smc_diag.c:44 [inline]
> >   RIP: 0010:__smc_diag_dump.constprop.0+0x3ca/0x2550 net/smc/smc_diag.c:89
> >   Call Trace:
> >    <TASK>
> >    smc_diag_dump_proto+0x26d/0x420 net/smc/smc_diag.c:217
> >    smc_diag_dump+0x27/0x90 net/smc/smc_diag.c:234
> >    netlink_dump+0x539/0xd30 net/netlink/af_netlink.c:2327
> >    __netlink_dump_start+0x6d6/0x990 net/netlink/af_netlink.c:2442
> >    netlink_dump_start include/linux/netlink.h:341 [inline]
> >    smc_diag_handler_dump+0x1f9/0x240 net/smc/smc_diag.c:251
> >    __sock_diag_cmd net/core/sock_diag.c:249 [inline]
> >    sock_diag_rcv_msg+0x438/0x790 net/core/sock_diag.c:285
> >    netlink_rcv_skb+0x158/0x420 net/netlink/af_netlink.c:2552
> >    netlink_unicast_kernel net/netlink/af_netlink.c:1320 [inline]
> >    netlink_unicast+0x5a7/0x870 net/netlink/af_netlink.c:1346
> >    netlink_sendmsg+0x8d1/0xdd0 net/netlink/af_netlink.c:1896
> >    sock_sendmsg_nosec net/socket.c:714 [inline]
> >    __sock_sendmsg net/socket.c:729 [inline]
> >    ____sys_sendmsg+0xa95/0xc70 net/socket.c:2614
> >    ___sys_sendmsg+0x134/0x1d0 net/socket.c:2668
> >    __sys_sendmsg+0x16d/0x220 net/socket.c:2700
> >    do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
> >    do_syscall_64+0xcd/0x4e0 arch/x86/entry/syscall_64.c:94
> >    entry_SYSCALL_64_after_hwframe+0x77/0x7f
> >    </TASK>
> >
> > The process like this:
> >
> >                (CPU1)              |             (CPU2)
> >   ---------------------------------|-------------------------------
> >   inet_create()                    |
> >     // init clcsock to NULL        |
> >     sk = sk_alloc()                |
> >                                    |
> >     // unexpectedly change clcsock |
> >     inet_init_csk_locks()          |
> >                                    |
> >     // add sk to hash table        |
> >     smc_inet_init_sock()           |
> >       smc_sk_init()                |
> >         smc_hash_sk()              |
> >                                    | // traverse the hash table
> >                                    | smc_diag_dump_proto
> >                                    |   __smc_diag_dump()
> >                                    |     // visit wrong clcsock
> >                                    |     smc_diag_msg_common_fill()
> >     // alloc clcsock               |
> >     smc_create_clcsk               |
> >       sock_create_kern             |
> >
> > With CONFIG_DEBUG_LOCK_ALLOC=y, the smc->clcsock is unexpectedly changed
> > in inet_init_csk_locks(), because the struct smc_sock does not have struct
> > inet_connection_sock as the first member.
> >
> > Previous commit 60ada4fe644e ("smc: Fix various oops due to inet_sock type
> > confusion.") add inet_sock as the first member of smc_sock. For protocol
> > with INET_PROTOSW_ICSK, use inet_connection_sock instead of inet_sock is
> > more appropriate.

Why is INET_PROTOSW_ICSK necessary in the first place ?

I don't see a clear reason because smc_clcsock_accept() allocates
a new sock by smc_sock_alloc() and does not use inet_accept().

Or is there any other path where smc_sock is cast to
inet_connection_sock ?


> >
> > Reported-by: syzbot+f775be4458668f7d220e@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=f775be4458668f7d220e
> > Tested-by: syzbot+f775be4458668f7d220e@syzkaller.appspotmail.com
> > Fixes: d25a92ccae6b ("net/smc: Introduce IPPROTO_SMC")
> > Signed-off-by: Wang Liang <wangliang74@huawei.com>
> > ---
> >  net/smc/smc.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/smc/smc.h b/net/smc/smc.h
> > index 2c9084963739..1b20f0c927d3 100644
> > --- a/net/smc/smc.h
> > +++ b/net/smc/smc.h
> > @@ -285,7 +285,7 @@ struct smc_connection {
> >  struct smc_sock {                              /* smc sock container */
> >         union {
> >                 struct sock             sk;
> > -               struct inet_sock        icsk_inet;
> > +               struct inet_connection_sock     inet_conn;
> >         };
> >         struct socket           *clcsock;       /* internal tcp socket */
> >         void                    (*clcsk_state_change)(struct sock *sk);
> > --
> > 2.34.1
> >
>
> Kuniyuki, can you please review, I think you had a related fix recently.
>
> Thanks.
>
> commit 60ada4fe644edaa6c2da97364184b0425e8aeaf5
> Author: Kuniyuki Iwashima <kuniyu@google.com>
> Date:   Fri Jul 11 06:07:52 2025 +0000
>
>     smc: Fix various oops due to inet_sock type confusion.

Re: [PATCH net] net/smc: fix general protection fault in __smc_diag_dump

[Eric Dumazet]

On Thu, Sep 25, 2025 at 11:46 AM Kuniyuki Iwashima <kuniyu@google.com> wrote:
>
> Thanks Eric for CCing me.
>
> On Thu, Sep 25, 2025 at 7:32 AM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Mon, Sep 22, 2025 at 4:57 AM Wang Liang <wangliang74@huawei.com> wrote:
> > >
> > > The syzbot report a crash:
> > >
> > >   Oops: general protection fault, probably for non-canonical address 0xfbd5a5d5a0000003: 0000 [#1] SMP KASAN NOPTI
> > >   KASAN: maybe wild-memory-access in range [0xdead4ead00000018-0xdead4ead0000001f]
> > >   CPU: 1 UID: 0 PID: 6949 Comm: syz.0.335 Not tainted syzkaller #0 PREEMPT(full)
> > >   Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/18/2025
> > >   RIP: 0010:smc_diag_msg_common_fill net/smc/smc_diag.c:44 [inline]
> > >   RIP: 0010:__smc_diag_dump.constprop.0+0x3ca/0x2550 net/smc/smc_diag.c:89
> > >   Call Trace:
> > >    <TASK>
> > >    smc_diag_dump_proto+0x26d/0x420 net/smc/smc_diag.c:217
> > >    smc_diag_dump+0x27/0x90 net/smc/smc_diag.c:234
> > >    netlink_dump+0x539/0xd30 net/netlink/af_netlink.c:2327
> > >    __netlink_dump_start+0x6d6/0x990 net/netlink/af_netlink.c:2442
> > >    netlink_dump_start include/linux/netlink.h:341 [inline]
> > >    smc_diag_handler_dump+0x1f9/0x240 net/smc/smc_diag.c:251
> > >    __sock_diag_cmd net/core/sock_diag.c:249 [inline]
> > >    sock_diag_rcv_msg+0x438/0x790 net/core/sock_diag.c:285
> > >    netlink_rcv_skb+0x158/0x420 net/netlink/af_netlink.c:2552
> > >    netlink_unicast_kernel net/netlink/af_netlink.c:1320 [inline]
> > >    netlink_unicast+0x5a7/0x870 net/netlink/af_netlink.c:1346
> > >    netlink_sendmsg+0x8d1/0xdd0 net/netlink/af_netlink.c:1896
> > >    sock_sendmsg_nosec net/socket.c:714 [inline]
> > >    __sock_sendmsg net/socket.c:729 [inline]
> > >    ____sys_sendmsg+0xa95/0xc70 net/socket.c:2614
> > >    ___sys_sendmsg+0x134/0x1d0 net/socket.c:2668
> > >    __sys_sendmsg+0x16d/0x220 net/socket.c:2700
> > >    do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
> > >    do_syscall_64+0xcd/0x4e0 arch/x86/entry/syscall_64.c:94
> > >    entry_SYSCALL_64_after_hwframe+0x77/0x7f
> > >    </TASK>
> > >
> > > The process like this:
> > >
> > >                (CPU1)              |             (CPU2)
> > >   ---------------------------------|-------------------------------
> > >   inet_create()                    |
> > >     // init clcsock to NULL        |
> > >     sk = sk_alloc()                |
> > >                                    |
> > >     // unexpectedly change clcsock |
> > >     inet_init_csk_locks()          |
> > >                                    |
> > >     // add sk to hash table        |
> > >     smc_inet_init_sock()           |
> > >       smc_sk_init()                |
> > >         smc_hash_sk()              |
> > >                                    | // traverse the hash table
> > >                                    | smc_diag_dump_proto
> > >                                    |   __smc_diag_dump()
> > >                                    |     // visit wrong clcsock
> > >                                    |     smc_diag_msg_common_fill()
> > >     // alloc clcsock               |
> > >     smc_create_clcsk               |
> > >       sock_create_kern             |
> > >
> > > With CONFIG_DEBUG_LOCK_ALLOC=y, the smc->clcsock is unexpectedly changed
> > > in inet_init_csk_locks(), because the struct smc_sock does not have struct
> > > inet_connection_sock as the first member.
> > >
> > > Previous commit 60ada4fe644e ("smc: Fix various oops due to inet_sock type
> > > confusion.") add inet_sock as the first member of smc_sock. For protocol
> > > with INET_PROTOSW_ICSK, use inet_connection_sock instead of inet_sock is
> > > more appropriate.
>
> Why is INET_PROTOSW_ICSK necessary in the first place ?
>
> I don't see a clear reason because smc_clcsock_accept() allocates
> a new sock by smc_sock_alloc() and does not use inet_accept().
>
> Or is there any other path where smc_sock is cast to
> inet_connection_sock ?

What I saw in this code was a missing protection.

smc_diag_msg_common_fill() runs without socket lock being held.

I was thinking of this fix, but apparently syzbot still got crashes.

diff --git a/net/smc/smc_close.c b/net/smc/smc_close.c
index 10219f55aad14d795dabe4331458bd1b73c22789..b6abd0efea22c0c9726090b5de60e648b86e09a0
100644
--- a/net/smc/smc_close.c
+++ b/net/smc/smc_close.c
@@ -30,7 +30,8 @@ void smc_clcsock_release(struct smc_sock *smc)
        mutex_lock(&smc->clcsock_release_lock);
        if (smc->clcsock) {
                tcp = smc->clcsock;
-               smc->clcsock = NULL;
+               WRITE_ONCE(smc->clcsock, NULL);
+               synchronize_rcu();
                sock_release(tcp);
        }
        mutex_unlock(&smc->clcsock_release_lock);
diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
index bf0beaa23bdb63edfe0c37515aa17a04bb648c08..069607c1db9aff76d1d4f23b47dfeb5177c433d8
100644
--- a/net/smc/smc_diag.c
+++ b/net/smc/smc_diag.c
@@ -35,26 +35,32 @@ static struct smc_diag_dump_ctx
*smc_dump_context(struct netlink_callback *cb)
 static void smc_diag_msg_common_fill(struct smc_diag_msg *r, struct sock *sk)
 {
        struct smc_sock *smc = smc_sk(sk);
+       struct socket *clcsock;

        memset(r, 0, sizeof(*r));
        r->diag_family = sk->sk_family;
        sock_diag_save_cookie(sk, r->id.idiag_cookie);
-       if (!smc->clcsock)
-               return;
-       r->id.idiag_sport = htons(smc->clcsock->sk->sk_num);
-       r->id.idiag_dport = smc->clcsock->sk->sk_dport;
-       r->id.idiag_if = smc->clcsock->sk->sk_bound_dev_if;
+
+       rcu_read_lock();
+       clcsock = READ_ONCE(smc->clcsock);
+       if (!clcsock)
+               goto unlock;
+       r->id.idiag_sport = htons(clcsock->sk->sk_num);
+       r->id.idiag_dport = clcsock->sk->sk_dport;
+       r->id.idiag_if = clcsock->sk->sk_bound_dev_if;
        if (sk->sk_protocol == SMCPROTO_SMC) {
-               r->id.idiag_src[0] = smc->clcsock->sk->sk_rcv_saddr;
-               r->id.idiag_dst[0] = smc->clcsock->sk->sk_daddr;
+               r->id.idiag_src[0] = clcsock->sk->sk_rcv_saddr;
+               r->id.idiag_dst[0] = clcsock->sk->sk_daddr;
 #if IS_ENABLED(CONFIG_IPV6)
        } else if (sk->sk_protocol == SMCPROTO_SMC6) {
-               memcpy(&r->id.idiag_src, &smc->clcsock->sk->sk_v6_rcv_saddr,
-                      sizeof(smc->clcsock->sk->sk_v6_rcv_saddr));
-               memcpy(&r->id.idiag_dst, &smc->clcsock->sk->sk_v6_daddr,
-                      sizeof(smc->clcsock->sk->sk_v6_daddr));
+               memcpy(&r->id.idiag_src, &clcsock->sk->sk_v6_rcv_saddr,
+                      sizeof(clcsock->sk->sk_v6_rcv_saddr));
+               memcpy(&r->id.idiag_dst, &clcsock->sk->sk_v6_daddr,
+                      sizeof(clcsock->sk->sk_v6_daddr));
 #endif
        }
+unlock:
+       rcu_read_unlock();
 }

Re: [PATCH net] net/smc: fix general protection fault in __smc_diag_dump

[Kuniyuki Iwashima]

On Thu, Sep 25, 2025 at 11:54 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Sep 25, 2025 at 11:46 AM Kuniyuki Iwashima <kuniyu@google.com> wrote:
> >
> > Thanks Eric for CCing me.
> >
> > On Thu, Sep 25, 2025 at 7:32 AM Eric Dumazet <edumazet@google.com> wrote:
> > >
> > > On Mon, Sep 22, 2025 at 4:57 AM Wang Liang <wangliang74@huawei.com> wrote:
> > > >
> > > > The syzbot report a crash:
> > > >
> > > >   Oops: general protection fault, probably for non-canonical address 0xfbd5a5d5a0000003: 0000 [#1] SMP KASAN NOPTI
> > > >   KASAN: maybe wild-memory-access in range [0xdead4ead00000018-0xdead4ead0000001f]
> > > >   CPU: 1 UID: 0 PID: 6949 Comm: syz.0.335 Not tainted syzkaller #0 PREEMPT(full)
> > > >   Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/18/2025
> > > >   RIP: 0010:smc_diag_msg_common_fill net/smc/smc_diag.c:44 [inline]
> > > >   RIP: 0010:__smc_diag_dump.constprop.0+0x3ca/0x2550 net/smc/smc_diag.c:89
> > > >   Call Trace:
> > > >    <TASK>
> > > >    smc_diag_dump_proto+0x26d/0x420 net/smc/smc_diag.c:217
> > > >    smc_diag_dump+0x27/0x90 net/smc/smc_diag.c:234
> > > >    netlink_dump+0x539/0xd30 net/netlink/af_netlink.c:2327
> > > >    __netlink_dump_start+0x6d6/0x990 net/netlink/af_netlink.c:2442
> > > >    netlink_dump_start include/linux/netlink.h:341 [inline]
> > > >    smc_diag_handler_dump+0x1f9/0x240 net/smc/smc_diag.c:251
> > > >    __sock_diag_cmd net/core/sock_diag.c:249 [inline]
> > > >    sock_diag_rcv_msg+0x438/0x790 net/core/sock_diag.c:285
> > > >    netlink_rcv_skb+0x158/0x420 net/netlink/af_netlink.c:2552
> > > >    netlink_unicast_kernel net/netlink/af_netlink.c:1320 [inline]
> > > >    netlink_unicast+0x5a7/0x870 net/netlink/af_netlink.c:1346
> > > >    netlink_sendmsg+0x8d1/0xdd0 net/netlink/af_netlink.c:1896
> > > >    sock_sendmsg_nosec net/socket.c:714 [inline]
> > > >    __sock_sendmsg net/socket.c:729 [inline]
> > > >    ____sys_sendmsg+0xa95/0xc70 net/socket.c:2614
> > > >    ___sys_sendmsg+0x134/0x1d0 net/socket.c:2668
> > > >    __sys_sendmsg+0x16d/0x220 net/socket.c:2700
> > > >    do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
> > > >    do_syscall_64+0xcd/0x4e0 arch/x86/entry/syscall_64.c:94
> > > >    entry_SYSCALL_64_after_hwframe+0x77/0x7f
> > > >    </TASK>
> > > >
> > > > The process like this:
> > > >
> > > >                (CPU1)              |             (CPU2)
> > > >   ---------------------------------|-------------------------------
> > > >   inet_create()                    |
> > > >     // init clcsock to NULL        |
> > > >     sk = sk_alloc()                |
> > > >                                    |
> > > >     // unexpectedly change clcsock |
> > > >     inet_init_csk_locks()          |
> > > >                                    |
> > > >     // add sk to hash table        |
> > > >     smc_inet_init_sock()           |
> > > >       smc_sk_init()                |
> > > >         smc_hash_sk()              |
> > > >                                    | // traverse the hash table
> > > >                                    | smc_diag_dump_proto
> > > >                                    |   __smc_diag_dump()
> > > >                                    |     // visit wrong clcsock
> > > >                                    |     smc_diag_msg_common_fill()
> > > >     // alloc clcsock               |
> > > >     smc_create_clcsk               |
> > > >       sock_create_kern             |
> > > >
> > > > With CONFIG_DEBUG_LOCK_ALLOC=y, the smc->clcsock is unexpectedly changed
> > > > in inet_init_csk_locks(), because the struct smc_sock does not have struct
> > > > inet_connection_sock as the first member.
> > > >
> > > > Previous commit 60ada4fe644e ("smc: Fix various oops due to inet_sock type
> > > > confusion.") add inet_sock as the first member of smc_sock. For protocol
> > > > with INET_PROTOSW_ICSK, use inet_connection_sock instead of inet_sock is
> > > > more appropriate.
> >
> > Why is INET_PROTOSW_ICSK necessary in the first place ?
> >
> > I don't see a clear reason because smc_clcsock_accept() allocates
> > a new sock by smc_sock_alloc() and does not use inet_accept().
> >
> > Or is there any other path where smc_sock is cast to
> > inet_connection_sock ?
>
> What I saw in this code was a missing protection.
>
> smc_diag_msg_common_fill() runs without socket lock being held.
>
> I was thinking of this fix, but apparently syzbot still got crashes.

Looking at the test result,

https://syzkaller.appspot.com/x/report.txt?x=15944c7c580000
KASAN: maybe wild-memory-access in range [0xdead4ead00000018-0xdead4ead0000001f]

the top half of the address is SPINLOCK_MAGIC (0xdead4ead),
so the type confusion mentioned in the commit message makes
sense to me.

$ pahole -C inet_connection_sock vmlinux
struct inet_connection_sock {
...
    struct request_sock_queue  icsk_accept_queue;    /*   992    80 */

$ pahole -C smc_sock vmlinux
struct smc_sock {
...
    struct socket *            clcsock;              /*   992     8 */

The option is 1) let inet_init_csk_locks() init inet_connection_sock
or 2) avoid inet_init_csk_locks(), and I guess 2) could be better to
avoid potential issues in IS_ICSK branches.


>
> diff --git a/net/smc/smc_close.c b/net/smc/smc_close.c
> index 10219f55aad14d795dabe4331458bd1b73c22789..b6abd0efea22c0c9726090b5de60e648b86e09a0
> 100644
> --- a/net/smc/smc_close.c
> +++ b/net/smc/smc_close.c
> @@ -30,7 +30,8 @@ void smc_clcsock_release(struct smc_sock *smc)
>         mutex_lock(&smc->clcsock_release_lock);
>         if (smc->clcsock) {
>                 tcp = smc->clcsock;
> -               smc->clcsock = NULL;
> +               WRITE_ONCE(smc->clcsock, NULL);
> +               synchronize_rcu();
>                 sock_release(tcp);
>         }
>         mutex_unlock(&smc->clcsock_release_lock);
> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
> index bf0beaa23bdb63edfe0c37515aa17a04bb648c08..069607c1db9aff76d1d4f23b47dfeb5177c433d8
> 100644
> --- a/net/smc/smc_diag.c
> +++ b/net/smc/smc_diag.c
> @@ -35,26 +35,32 @@ static struct smc_diag_dump_ctx
> *smc_dump_context(struct netlink_callback *cb)
>  static void smc_diag_msg_common_fill(struct smc_diag_msg *r, struct sock *sk)
>  {
>         struct smc_sock *smc = smc_sk(sk);
> +       struct socket *clcsock;
>
>         memset(r, 0, sizeof(*r));
>         r->diag_family = sk->sk_family;
>         sock_diag_save_cookie(sk, r->id.idiag_cookie);
> -       if (!smc->clcsock)
> -               return;
> -       r->id.idiag_sport = htons(smc->clcsock->sk->sk_num);
> -       r->id.idiag_dport = smc->clcsock->sk->sk_dport;
> -       r->id.idiag_if = smc->clcsock->sk->sk_bound_dev_if;
> +
> +       rcu_read_lock();
> +       clcsock = READ_ONCE(smc->clcsock);
> +       if (!clcsock)
> +               goto unlock;
> +       r->id.idiag_sport = htons(clcsock->sk->sk_num);
> +       r->id.idiag_dport = clcsock->sk->sk_dport;
> +       r->id.idiag_if = clcsock->sk->sk_bound_dev_if;
>         if (sk->sk_protocol == SMCPROTO_SMC) {
> -               r->id.idiag_src[0] = smc->clcsock->sk->sk_rcv_saddr;
> -               r->id.idiag_dst[0] = smc->clcsock->sk->sk_daddr;
> +               r->id.idiag_src[0] = clcsock->sk->sk_rcv_saddr;
> +               r->id.idiag_dst[0] = clcsock->sk->sk_daddr;
>  #if IS_ENABLED(CONFIG_IPV6)
>         } else if (sk->sk_protocol == SMCPROTO_SMC6) {
> -               memcpy(&r->id.idiag_src, &smc->clcsock->sk->sk_v6_rcv_saddr,
> -                      sizeof(smc->clcsock->sk->sk_v6_rcv_saddr));
> -               memcpy(&r->id.idiag_dst, &smc->clcsock->sk->sk_v6_daddr,
> -                      sizeof(smc->clcsock->sk->sk_v6_daddr));
> +               memcpy(&r->id.idiag_src, &clcsock->sk->sk_v6_rcv_saddr,
> +                      sizeof(clcsock->sk->sk_v6_rcv_saddr));
> +               memcpy(&r->id.idiag_dst, &clcsock->sk->sk_v6_daddr,
> +                      sizeof(clcsock->sk->sk_v6_daddr));
>  #endif
>         }
> +unlock:
> +       rcu_read_unlock();
>  }

Re: [PATCH net] net/smc: fix general protection fault in __smc_diag_dump

[Eric Dumazet]

On Thu, Sep 25, 2025 at 12:25 PM Kuniyuki Iwashima <kuniyu@google.com> wrote:
>
> On Thu, Sep 25, 2025 at 11:54 AM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Thu, Sep 25, 2025 at 11:46 AM Kuniyuki Iwashima <kuniyu@google.com> wrote:
> > >
> > > Thanks Eric for CCing me.
> > >
> > > On Thu, Sep 25, 2025 at 7:32 AM Eric Dumazet <edumazet@google.com> wrote:
> > > >
> > > > On Mon, Sep 22, 2025 at 4:57 AM Wang Liang <wangliang74@huawei.com> wrote:
> > > > >
> > > > > The syzbot report a crash:
> > > > >
> > > > >   Oops: general protection fault, probably for non-canonical address 0xfbd5a5d5a0000003: 0000 [#1] SMP KASAN NOPTI
> > > > >   KASAN: maybe wild-memory-access in range [0xdead4ead00000018-0xdead4ead0000001f]
> > > > >   CPU: 1 UID: 0 PID: 6949 Comm: syz.0.335 Not tainted syzkaller #0 PREEMPT(full)
> > > > >   Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/18/2025
> > > > >   RIP: 0010:smc_diag_msg_common_fill net/smc/smc_diag.c:44 [inline]
> > > > >   RIP: 0010:__smc_diag_dump.constprop.0+0x3ca/0x2550 net/smc/smc_diag.c:89
> > > > >   Call Trace:
> > > > >    <TASK>
> > > > >    smc_diag_dump_proto+0x26d/0x420 net/smc/smc_diag.c:217
> > > > >    smc_diag_dump+0x27/0x90 net/smc/smc_diag.c:234
> > > > >    netlink_dump+0x539/0xd30 net/netlink/af_netlink.c:2327
> > > > >    __netlink_dump_start+0x6d6/0x990 net/netlink/af_netlink.c:2442
> > > > >    netlink_dump_start include/linux/netlink.h:341 [inline]
> > > > >    smc_diag_handler_dump+0x1f9/0x240 net/smc/smc_diag.c:251
> > > > >    __sock_diag_cmd net/core/sock_diag.c:249 [inline]
> > > > >    sock_diag_rcv_msg+0x438/0x790 net/core/sock_diag.c:285
> > > > >    netlink_rcv_skb+0x158/0x420 net/netlink/af_netlink.c:2552
> > > > >    netlink_unicast_kernel net/netlink/af_netlink.c:1320 [inline]
> > > > >    netlink_unicast+0x5a7/0x870 net/netlink/af_netlink.c:1346
> > > > >    netlink_sendmsg+0x8d1/0xdd0 net/netlink/af_netlink.c:1896
> > > > >    sock_sendmsg_nosec net/socket.c:714 [inline]
> > > > >    __sock_sendmsg net/socket.c:729 [inline]
> > > > >    ____sys_sendmsg+0xa95/0xc70 net/socket.c:2614
> > > > >    ___sys_sendmsg+0x134/0x1d0 net/socket.c:2668
> > > > >    __sys_sendmsg+0x16d/0x220 net/socket.c:2700
> > > > >    do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
> > > > >    do_syscall_64+0xcd/0x4e0 arch/x86/entry/syscall_64.c:94
> > > > >    entry_SYSCALL_64_after_hwframe+0x77/0x7f
> > > > >    </TASK>
> > > > >
> > > > > The process like this:
> > > > >
> > > > >                (CPU1)              |             (CPU2)
> > > > >   ---------------------------------|-------------------------------
> > > > >   inet_create()                    |
> > > > >     // init clcsock to NULL        |
> > > > >     sk = sk_alloc()                |
> > > > >                                    |
> > > > >     // unexpectedly change clcsock |
> > > > >     inet_init_csk_locks()          |
> > > > >                                    |
> > > > >     // add sk to hash table        |
> > > > >     smc_inet_init_sock()           |
> > > > >       smc_sk_init()                |
> > > > >         smc_hash_sk()              |
> > > > >                                    | // traverse the hash table
> > > > >                                    | smc_diag_dump_proto
> > > > >                                    |   __smc_diag_dump()
> > > > >                                    |     // visit wrong clcsock
> > > > >                                    |     smc_diag_msg_common_fill()
> > > > >     // alloc clcsock               |
> > > > >     smc_create_clcsk               |
> > > > >       sock_create_kern             |
> > > > >
> > > > > With CONFIG_DEBUG_LOCK_ALLOC=y, the smc->clcsock is unexpectedly changed
> > > > > in inet_init_csk_locks(), because the struct smc_sock does not have struct
> > > > > inet_connection_sock as the first member.
> > > > >
> > > > > Previous commit 60ada4fe644e ("smc: Fix various oops due to inet_sock type
> > > > > confusion.") add inet_sock as the first member of smc_sock. For protocol
> > > > > with INET_PROTOSW_ICSK, use inet_connection_sock instead of inet_sock is
> > > > > more appropriate.
> > >
> > > Why is INET_PROTOSW_ICSK necessary in the first place ?
> > >
> > > I don't see a clear reason because smc_clcsock_accept() allocates
> > > a new sock by smc_sock_alloc() and does not use inet_accept().
> > >
> > > Or is there any other path where smc_sock is cast to
> > > inet_connection_sock ?
> >
> > What I saw in this code was a missing protection.
> >
> > smc_diag_msg_common_fill() runs without socket lock being held.
> >
> > I was thinking of this fix, but apparently syzbot still got crashes.
>
> Looking at the test result,
>
> https://syzkaller.appspot.com/x/report.txt?x=15944c7c580000
> KASAN: maybe wild-memory-access in range [0xdead4ead00000018-0xdead4ead0000001f]
>
> the top half of the address is SPINLOCK_MAGIC (0xdead4ead),
> so the type confusion mentioned in the commit message makes
> sense to me.
>
> $ pahole -C inet_connection_sock vmlinux
> struct inet_connection_sock {
> ...
>     struct request_sock_queue  icsk_accept_queue;    /*   992    80 */
>
> $ pahole -C smc_sock vmlinux
> struct smc_sock {
> ...
>     struct socket *            clcsock;              /*   992     8 */
>
> The option is 1) let inet_init_csk_locks() init inet_connection_sock
> or 2) avoid inet_init_csk_locks(), and I guess 2) could be better to
> avoid potential issues in IS_ICSK branches.
>

I definitely vote to remove INET_PROTOSW_ICSK from smc.

We want to reserve inet_connection_sock to TCP only, so that we can
move fields to better
cache friendly locations in tcp_sock hopefully for linux-6.19

Re: [PATCH net] net/smc: fix general protection fault in __smc_diag_dump

[Kuniyuki Iwashima]

On Thu, Sep 25, 2025 at 12:37 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Sep 25, 2025 at 12:25 PM Kuniyuki Iwashima <kuniyu@google.com> wrote:
> >
> > On Thu, Sep 25, 2025 at 11:54 AM Eric Dumazet <edumazet@google.com> wrote:
> > >
> > > On Thu, Sep 25, 2025 at 11:46 AM Kuniyuki Iwashima <kuniyu@google.com> wrote:
> > > >
> > > > Thanks Eric for CCing me.
> > > >
> > > > On Thu, Sep 25, 2025 at 7:32 AM Eric Dumazet <edumazet@google.com> wrote:
> > > > >
> > > > > On Mon, Sep 22, 2025 at 4:57 AM Wang Liang <wangliang74@huawei.com> wrote:
> > > > > >
> > > > > > The syzbot report a crash:
> > > > > >
> > > > > >   Oops: general protection fault, probably for non-canonical address 0xfbd5a5d5a0000003: 0000 [#1] SMP KASAN NOPTI
> > > > > >   KASAN: maybe wild-memory-access in range [0xdead4ead00000018-0xdead4ead0000001f]
> > > > > >   CPU: 1 UID: 0 PID: 6949 Comm: syz.0.335 Not tainted syzkaller #0 PREEMPT(full)
> > > > > >   Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/18/2025
> > > > > >   RIP: 0010:smc_diag_msg_common_fill net/smc/smc_diag.c:44 [inline]
> > > > > >   RIP: 0010:__smc_diag_dump.constprop.0+0x3ca/0x2550 net/smc/smc_diag.c:89
> > > > > >   Call Trace:
> > > > > >    <TASK>
> > > > > >    smc_diag_dump_proto+0x26d/0x420 net/smc/smc_diag.c:217
> > > > > >    smc_diag_dump+0x27/0x90 net/smc/smc_diag.c:234
> > > > > >    netlink_dump+0x539/0xd30 net/netlink/af_netlink.c:2327
> > > > > >    __netlink_dump_start+0x6d6/0x990 net/netlink/af_netlink.c:2442
> > > > > >    netlink_dump_start include/linux/netlink.h:341 [inline]
> > > > > >    smc_diag_handler_dump+0x1f9/0x240 net/smc/smc_diag.c:251
> > > > > >    __sock_diag_cmd net/core/sock_diag.c:249 [inline]
> > > > > >    sock_diag_rcv_msg+0x438/0x790 net/core/sock_diag.c:285
> > > > > >    netlink_rcv_skb+0x158/0x420 net/netlink/af_netlink.c:2552
> > > > > >    netlink_unicast_kernel net/netlink/af_netlink.c:1320 [inline]
> > > > > >    netlink_unicast+0x5a7/0x870 net/netlink/af_netlink.c:1346
> > > > > >    netlink_sendmsg+0x8d1/0xdd0 net/netlink/af_netlink.c:1896
> > > > > >    sock_sendmsg_nosec net/socket.c:714 [inline]
> > > > > >    __sock_sendmsg net/socket.c:729 [inline]
> > > > > >    ____sys_sendmsg+0xa95/0xc70 net/socket.c:2614
> > > > > >    ___sys_sendmsg+0x134/0x1d0 net/socket.c:2668
> > > > > >    __sys_sendmsg+0x16d/0x220 net/socket.c:2700
> > > > > >    do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
> > > > > >    do_syscall_64+0xcd/0x4e0 arch/x86/entry/syscall_64.c:94
> > > > > >    entry_SYSCALL_64_after_hwframe+0x77/0x7f
> > > > > >    </TASK>
> > > > > >
> > > > > > The process like this:
> > > > > >
> > > > > >                (CPU1)              |             (CPU2)
> > > > > >   ---------------------------------|-------------------------------
> > > > > >   inet_create()                    |
> > > > > >     // init clcsock to NULL        |
> > > > > >     sk = sk_alloc()                |
> > > > > >                                    |
> > > > > >     // unexpectedly change clcsock |
> > > > > >     inet_init_csk_locks()          |
> > > > > >                                    |
> > > > > >     // add sk to hash table        |
> > > > > >     smc_inet_init_sock()           |
> > > > > >       smc_sk_init()                |
> > > > > >         smc_hash_sk()              |
> > > > > >                                    | // traverse the hash table
> > > > > >                                    | smc_diag_dump_proto
> > > > > >                                    |   __smc_diag_dump()
> > > > > >                                    |     // visit wrong clcsock
> > > > > >                                    |     smc_diag_msg_common_fill()
> > > > > >     // alloc clcsock               |
> > > > > >     smc_create_clcsk               |
> > > > > >       sock_create_kern             |
> > > > > >
> > > > > > With CONFIG_DEBUG_LOCK_ALLOC=y, the smc->clcsock is unexpectedly changed
> > > > > > in inet_init_csk_locks(), because the struct smc_sock does not have struct
> > > > > > inet_connection_sock as the first member.
> > > > > >
> > > > > > Previous commit 60ada4fe644e ("smc: Fix various oops due to inet_sock type
> > > > > > confusion.") add inet_sock as the first member of smc_sock. For protocol
> > > > > > with INET_PROTOSW_ICSK, use inet_connection_sock instead of inet_sock is
> > > > > > more appropriate.
> > > >
> > > > Why is INET_PROTOSW_ICSK necessary in the first place ?
> > > >
> > > > I don't see a clear reason because smc_clcsock_accept() allocates
> > > > a new sock by smc_sock_alloc() and does not use inet_accept().
> > > >
> > > > Or is there any other path where smc_sock is cast to
> > > > inet_connection_sock ?
> > >
> > > What I saw in this code was a missing protection.
> > >
> > > smc_diag_msg_common_fill() runs without socket lock being held.
> > >
> > > I was thinking of this fix, but apparently syzbot still got crashes.
> >
> > Looking at the test result,
> >
> > https://syzkaller.appspot.com/x/report.txt?x=15944c7c580000
> > KASAN: maybe wild-memory-access in range [0xdead4ead00000018-0xdead4ead0000001f]
> >
> > the top half of the address is SPINLOCK_MAGIC (0xdead4ead),
> > so the type confusion mentioned in the commit message makes
> > sense to me.
> >
> > $ pahole -C inet_connection_sock vmlinux
> > struct inet_connection_sock {
> > ...
> >     struct request_sock_queue  icsk_accept_queue;    /*   992    80 */
> >
> > $ pahole -C smc_sock vmlinux
> > struct smc_sock {
> > ...
> >     struct socket *            clcsock;              /*   992     8 */
> >
> > The option is 1) let inet_init_csk_locks() init inet_connection_sock
> > or 2) avoid inet_init_csk_locks(), and I guess 2) could be better to
> > avoid potential issues in IS_ICSK branches.
> >
>
> I definitely vote to remove INET_PROTOSW_ICSK from smc.
>
> We want to reserve inet_connection_sock to TCP only, so that we can
> move fields to better
> cache friendly locations in tcp_sock hopefully for linux-6.19

Fully agreed.

Wang: please squash the revert of 6fd27ea183c2 for
INET_PROTOSW_ICSK removal.  This is for one of
IS_ICSK branches.

Re: [PATCH net] net/smc: fix general protection fault in __smc_diag_dump

[Wang Liang]

在 2025/9/26 3:51, Kuniyuki Iwashima 写道:
> On Thu, Sep 25, 2025 at 12:37 PM Eric Dumazet <edumazet@google.com> wrote:
>> On Thu, Sep 25, 2025 at 12:25 PM Kuniyuki Iwashima <kuniyu@google.com> wrote:
>>> On Thu, Sep 25, 2025 at 11:54 AM Eric Dumazet <edumazet@google.com> wrote:
>>>> On Thu, Sep 25, 2025 at 11:46 AM Kuniyuki Iwashima <kuniyu@google.com> wrote:
>>>>> Thanks Eric for CCing me.
>>>>>
>>>>> On Thu, Sep 25, 2025 at 7:32 AM Eric Dumazet <edumazet@google.com> wrote:
>>>>>> On Mon, Sep 22, 2025 at 4:57 AM Wang Liang <wangliang74@huawei.com> wrote:
>>>>>>> The syzbot report a crash:
>>>>>>>
>>>>>>>    Oops: general protection fault, probably for non-canonical address 0xfbd5a5d5a0000003: 0000 [#1] SMP KASAN NOPTI
>>>>>>>    KASAN: maybe wild-memory-access in range [0xdead4ead00000018-0xdead4ead0000001f]
>>>>>>>    CPU: 1 UID: 0 PID: 6949 Comm: syz.0.335 Not tainted syzkaller #0 PREEMPT(full)
>>>>>>>    Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/18/2025
>>>>>>>    RIP: 0010:smc_diag_msg_common_fill net/smc/smc_diag.c:44 [inline]
>>>>>>>    RIP: 0010:__smc_diag_dump.constprop.0+0x3ca/0x2550 net/smc/smc_diag.c:89
>>>>>>>    Call Trace:
>>>>>>>     <TASK>
>>>>>>>     smc_diag_dump_proto+0x26d/0x420 net/smc/smc_diag.c:217
>>>>>>>     smc_diag_dump+0x27/0x90 net/smc/smc_diag.c:234
>>>>>>>     netlink_dump+0x539/0xd30 net/netlink/af_netlink.c:2327
>>>>>>>     __netlink_dump_start+0x6d6/0x990 net/netlink/af_netlink.c:2442
>>>>>>>     netlink_dump_start include/linux/netlink.h:341 [inline]
>>>>>>>     smc_diag_handler_dump+0x1f9/0x240 net/smc/smc_diag.c:251
>>>>>>>     __sock_diag_cmd net/core/sock_diag.c:249 [inline]
>>>>>>>     sock_diag_rcv_msg+0x438/0x790 net/core/sock_diag.c:285
>>>>>>>     netlink_rcv_skb+0x158/0x420 net/netlink/af_netlink.c:2552
>>>>>>>     netlink_unicast_kernel net/netlink/af_netlink.c:1320 [inline]
>>>>>>>     netlink_unicast+0x5a7/0x870 net/netlink/af_netlink.c:1346
>>>>>>>     netlink_sendmsg+0x8d1/0xdd0 net/netlink/af_netlink.c:1896
>>>>>>>     sock_sendmsg_nosec net/socket.c:714 [inline]
>>>>>>>     __sock_sendmsg net/socket.c:729 [inline]
>>>>>>>     ____sys_sendmsg+0xa95/0xc70 net/socket.c:2614
>>>>>>>     ___sys_sendmsg+0x134/0x1d0 net/socket.c:2668
>>>>>>>     __sys_sendmsg+0x16d/0x220 net/socket.c:2700
>>>>>>>     do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
>>>>>>>     do_syscall_64+0xcd/0x4e0 arch/x86/entry/syscall_64.c:94
>>>>>>>     entry_SYSCALL_64_after_hwframe+0x77/0x7f
>>>>>>>     </TASK>
>>>>>>>
>>>>>>> The process like this:
>>>>>>>
>>>>>>>                 (CPU1)              |             (CPU2)
>>>>>>>    ---------------------------------|-------------------------------
>>>>>>>    inet_create()                    |
>>>>>>>      // init clcsock to NULL        |
>>>>>>>      sk = sk_alloc()                |
>>>>>>>                                     |
>>>>>>>      // unexpectedly change clcsock |
>>>>>>>      inet_init_csk_locks()          |
>>>>>>>                                     |
>>>>>>>      // add sk to hash table        |
>>>>>>>      smc_inet_init_sock()           |
>>>>>>>        smc_sk_init()                |
>>>>>>>          smc_hash_sk()              |
>>>>>>>                                     | // traverse the hash table
>>>>>>>                                     | smc_diag_dump_proto
>>>>>>>                                     |   __smc_diag_dump()
>>>>>>>                                     |     // visit wrong clcsock
>>>>>>>                                     |     smc_diag_msg_common_fill()
>>>>>>>      // alloc clcsock               |
>>>>>>>      smc_create_clcsk               |
>>>>>>>        sock_create_kern             |
>>>>>>>
>>>>>>> With CONFIG_DEBUG_LOCK_ALLOC=y, the smc->clcsock is unexpectedly changed
>>>>>>> in inet_init_csk_locks(), because the struct smc_sock does not have struct
>>>>>>> inet_connection_sock as the first member.
>>>>>>>
>>>>>>> Previous commit 60ada4fe644e ("smc: Fix various oops due to inet_sock type
>>>>>>> confusion.") add inet_sock as the first member of smc_sock. For protocol
>>>>>>> with INET_PROTOSW_ICSK, use inet_connection_sock instead of inet_sock is
>>>>>>> more appropriate.
>>>>> Why is INET_PROTOSW_ICSK necessary in the first place ?
>>>>>
>>>>> I don't see a clear reason because smc_clcsock_accept() allocates
>>>>> a new sock by smc_sock_alloc() and does not use inet_accept().
>>>>>
>>>>> Or is there any other path where smc_sock is cast to
>>>>> inet_connection_sock ?
>>>> What I saw in this code was a missing protection.
>>>>
>>>> smc_diag_msg_common_fill() runs without socket lock being held.
>>>>
>>>> I was thinking of this fix, but apparently syzbot still got crashes.
>>> Looking at the test result,
>>>
>>> https://syzkaller.appspot.com/x/report.txt?x=15944c7c580000
>>> KASAN: maybe wild-memory-access in range [0xdead4ead00000018-0xdead4ead0000001f]
>>>
>>> the top half of the address is SPINLOCK_MAGIC (0xdead4ead),
>>> so the type confusion mentioned in the commit message makes
>>> sense to me.
>>>
>>> $ pahole -C inet_connection_sock vmlinux
>>> struct inet_connection_sock {
>>> ...
>>>      struct request_sock_queue  icsk_accept_queue;    /*   992    80 */
>>>
>>> $ pahole -C smc_sock vmlinux
>>> struct smc_sock {
>>> ...
>>>      struct socket *            clcsock;              /*   992     8 */
>>>
>>> The option is 1) let inet_init_csk_locks() init inet_connection_sock
>>> or 2) avoid inet_init_csk_locks(), and I guess 2) could be better to
>>> avoid potential issues in IS_ICSK branches.
>>>
>> I definitely vote to remove INET_PROTOSW_ICSK from smc.
>>
>> We want to reserve inet_connection_sock to TCP only, so that we can
>> move fields to better
>> cache friendly locations in tcp_sock hopefully for linux-6.19
> Fully agreed.
>
> Wang: please squash the revert of 6fd27ea183c2 for
> INET_PROTOSW_ICSK removal.  This is for one of
> IS_ICSK branches.


Thanks for your suggestions, they are helpful!

I will remove INET_PROTOSW_ICSK from smc_inet_protosw and smc_inet6_protosw,

and revert 6fd27ea183c2 ("net/smc: fix lacks of icsk_syn_mss with 
IPPROTO_SMC")

in one patchset later.

------
Best regards
Wang Liang

Re: [PATCH net] net/smc: fix general protection fault in __smc_diag_dump

[D. Wythe]

On Fri, Sep 26, 2025 at 04:42:35PM +0800, Wang Liang wrote:
> 
> 在 2025/9/26 3:51, Kuniyuki Iwashima 写道:
> >On Thu, Sep 25, 2025 at 12:37 PM Eric Dumazet <edumazet@google.com> wrote:
> >>On Thu, Sep 25, 2025 at 12:25 PM Kuniyuki Iwashima <kuniyu@google.com> wrote:
> >>>On Thu, Sep 25, 2025 at 11:54 AM Eric Dumazet <edumazet@google.com> wrote:
> >>>>On Thu, Sep 25, 2025 at 11:46 AM Kuniyuki Iwashima <kuniyu@google.com> wrote:
> >>>>>Thanks Eric for CCing me.
> >>>>>
> >>>>>On Thu, Sep 25, 2025 at 7:32 AM Eric Dumazet <edumazet@google.com> wrote:
> >>>>>>On Mon, Sep 22, 2025 at 4:57 AM Wang Liang <wangliang74@huawei.com> wrote:
> >>>>>>>The syzbot report a crash:
> >>>>>>>
> >>>>>>>   Oops: general protection fault, probably for non-canonical address 0xfbd5a5d5a0000003: 0000 [#1] SMP KASAN NOPTI
> >>>>>>>   KASAN: maybe wild-memory-access in range [0xdead4ead00000018-0xdead4ead0000001f]
> >>>>>>>   CPU: 1 UID: 0 PID: 6949 Comm: syz.0.335 Not tainted syzkaller #0 PREEMPT(full)
> >>>>>>>   Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/18/2025
> >>>>>>>   RIP: 0010:smc_diag_msg_common_fill net/smc/smc_diag.c:44 [inline]
> >>>>>>>   RIP: 0010:__smc_diag_dump.constprop.0+0x3ca/0x2550 net/smc/smc_diag.c:89
> >>>>>>>   Call Trace:
> >>>>>>>    <TASK>
> >>>>>>>    smc_diag_dump_proto+0x26d/0x420 net/smc/smc_diag.c:217
> >>>>>>>    smc_diag_dump+0x27/0x90 net/smc/smc_diag.c:234
> >>>>>>>    netlink_dump+0x539/0xd30 net/netlink/af_netlink.c:2327
> >>>>>>>    __netlink_dump_start+0x6d6/0x990 net/netlink/af_netlink.c:2442
> >>>>>>>    netlink_dump_start include/linux/netlink.h:341 [inline]
> >>>>>>>    smc_diag_handler_dump+0x1f9/0x240 net/smc/smc_diag.c:251
> >>>>>>>    __sock_diag_cmd net/core/sock_diag.c:249 [inline]
> >>>>>>>    sock_diag_rcv_msg+0x438/0x790 net/core/sock_diag.c:285
> >>>>>>>    netlink_rcv_skb+0x158/0x420 net/netlink/af_netlink.c:2552
> >>>>>>>    netlink_unicast_kernel net/netlink/af_netlink.c:1320 [inline]
> >>>>>>>    netlink_unicast+0x5a7/0x870 net/netlink/af_netlink.c:1346
> >>>>>>>    netlink_sendmsg+0x8d1/0xdd0 net/netlink/af_netlink.c:1896
> >>>>>>>    sock_sendmsg_nosec net/socket.c:714 [inline]
> >>>>>>>    __sock_sendmsg net/socket.c:729 [inline]
> >>>>>>>    ____sys_sendmsg+0xa95/0xc70 net/socket.c:2614
> >>>>>>>    ___sys_sendmsg+0x134/0x1d0 net/socket.c:2668
> >>>>>>>    __sys_sendmsg+0x16d/0x220 net/socket.c:2700
> >>>>>>>    do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
> >>>>>>>    do_syscall_64+0xcd/0x4e0 arch/x86/entry/syscall_64.c:94
> >>>>>>>    entry_SYSCALL_64_after_hwframe+0x77/0x7f
> >>>>>>>    </TASK>
> >>>>>>>
> >>>>>>>The process like this:
> >>>>>>>
> >>>>>>>                (CPU1)              |             (CPU2)
> >>>>>>>   ---------------------------------|-------------------------------
> >>>>>>>   inet_create()                    |
> >>>>>>>     // init clcsock to NULL        |
> >>>>>>>     sk = sk_alloc()                |
> >>>>>>>                                    |
> >>>>>>>     // unexpectedly change clcsock |
> >>>>>>>     inet_init_csk_locks()          |
> >>>>>>>                                    |
> >>>>>>>     // add sk to hash table        |
> >>>>>>>     smc_inet_init_sock()           |
> >>>>>>>       smc_sk_init()                |
> >>>>>>>         smc_hash_sk()              |
> >>>>>>>                                    | // traverse the hash table
> >>>>>>>                                    | smc_diag_dump_proto
> >>>>>>>                                    |   __smc_diag_dump()
> >>>>>>>                                    |     // visit wrong clcsock
> >>>>>>>                                    |     smc_diag_msg_common_fill()
> >>>>>>>     // alloc clcsock               |
> >>>>>>>     smc_create_clcsk               |
> >>>>>>>       sock_create_kern             |
> >>>>>>>
> >>>>>>>With CONFIG_DEBUG_LOCK_ALLOC=y, the smc->clcsock is unexpectedly changed
> >>>>>>>in inet_init_csk_locks(), because the struct smc_sock does not have struct
> >>>>>>>inet_connection_sock as the first member.
> >>>>>>>
> >>>>>>>Previous commit 60ada4fe644e ("smc: Fix various oops due to inet_sock type
> >>>>>>>confusion.") add inet_sock as the first member of smc_sock. For protocol
> >>>>>>>with INET_PROTOSW_ICSK, use inet_connection_sock instead of inet_sock is
> >>>>>>>more appropriate.
> >>>>>Why is INET_PROTOSW_ICSK necessary in the first place ?
> >>>>>
> >>>>>I don't see a clear reason because smc_clcsock_accept() allocates
> >>>>>a new sock by smc_sock_alloc() and does not use inet_accept().
> >>>>>
> >>>>>Or is there any other path where smc_sock is cast to
> >>>>>inet_connection_sock ?
> >>>>What I saw in this code was a missing protection.
> >>>>
> >>>>smc_diag_msg_common_fill() runs without socket lock being held.
> >>>>
> >>>>I was thinking of this fix, but apparently syzbot still got crashes.
> >>>Looking at the test result,
> >>>
> >>>https://syzkaller.appspot.com/x/report.txt?x=15944c7c580000
> >>>KASAN: maybe wild-memory-access in range [0xdead4ead00000018-0xdead4ead0000001f]
> >>>
> >>>the top half of the address is SPINLOCK_MAGIC (0xdead4ead),
> >>>so the type confusion mentioned in the commit message makes
> >>>sense to me.
> >>>
> >>>$ pahole -C inet_connection_sock vmlinux
> >>>struct inet_connection_sock {
> >>>...
> >>>     struct request_sock_queue  icsk_accept_queue;    /*   992    80 */
> >>>
> >>>$ pahole -C smc_sock vmlinux
> >>>struct smc_sock {
> >>>...
> >>>     struct socket *            clcsock;              /*   992     8 */
> >>>
> >>>The option is 1) let inet_init_csk_locks() init inet_connection_sock
> >>>or 2) avoid inet_init_csk_locks(), and I guess 2) could be better to
> >>>avoid potential issues in IS_ICSK branches.
> >>>
> >>I definitely vote to remove INET_PROTOSW_ICSK from smc.
> >>
> >>We want to reserve inet_connection_sock to TCP only, so that we can
> >>move fields to better
> >>cache friendly locations in tcp_sock hopefully for linux-6.19
> >Fully agreed.
> >
> >Wang: please squash the revert of 6fd27ea183c2 for
> >INET_PROTOSW_ICSK removal.  This is for one of
> >IS_ICSK branches.
> 
> 
> Thanks for your suggestions, they are helpful!
> 
> I will remove INET_PROTOSW_ICSK from smc_inet_protosw and smc_inet6_protosw,
> 

This is a bit of a long story. The INET_PROTOSW_ICSK flag was originally
introduced for an effort to merge smc_sock and clcsock. The goal was to
allow the merged socket to behave like a standard tcp_sock during
fallback, thus avoiding the need for any special handling.

However, this approach was later abandoned. Since the original reason
for this flag no longer exists, so the removal makes sense.

>