tipc-discussion — User questions & problem reports, project news, developer discussion

[tipc-discussion] [net] tipc: fix use-after-free Read in tipc_named_reinit

[Hoang Le <hoa...@de...>]

syzbot found the following issue on:
==================================================================
BUG: KASAN: use-after-free in tipc_named_reinit+0x94f/0x9b0
net/tipc/name_distr.c:413
Read of size 8 at addr ffff88805299a000 by task kworker/1:9/23764

CPU: 1 PID: 23764 Comm: kworker/1:9 Not tainted
5.18.0-rc4-syzkaller-00878-g17d49e6e8012 #0
Hardware name: Google Compute Engine/Google Compute Engine,
BIOS Google 01/01/2011
Workqueue: events tipc_net_finalize_work
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:88 [inline]
 dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
 print_address_description.constprop.0.cold+0xeb/0x495
mm/kasan/report.c:313
 print_report mm/kasan/report.c:429 [inline]
 kasan_report.cold+0xf4/0x1c6 mm/kasan/report.c:491
 tipc_named_reinit+0x94f/0x9b0 net/tipc/name_distr.c:413
 tipc_net_finalize+0x234/0x3d0 net/tipc/net.c:138
 process_one_work+0x996/0x1610 kernel/workqueue.c:2289
 worker_thread+0x665/0x1080 kernel/workqueue.c:2436
 kthread+0x2e9/0x3a0 kernel/kthread.c:376
 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:298
 </TASK>
[...]
==================================================================

In the commit
d966ddcc3821 ("tipc: fix a deadlock when flushing scheduled work"),
the cancel_work_sync() function just to make sure ONLY the work
tipc_net_finalize_work() is executing/pending on any CPU completed before
tipc namespace is destroyed through tipc_exit_net(). But this function
is not guaranteed the work is the last queued. So, the destroyed instance
may be accessed in the work which will try to enqueue later.

In order to completely fix, we re-order the calling of cancel_work_sync()
to make sure the work tipc_net_finalize_work() was last queued and it
must be completed by calling cancel_work_sync().

Reported-by: syz...@sy...
Fixes: d966ddcc3821 ("tipc: fix a deadlock when flushing scheduled work")
Acked-by: Jon Maloy <jm...@re...>
Signed-off-by: Ying Xue <yin...@wi...>
Signed-off-by: Hoang Le <hoa...@de...>
---
 net/tipc/core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/tipc/core.c b/net/tipc/core.c
index 3f4542e0f065..434e70eabe08 100644
--- a/net/tipc/core.c
+++ b/net/tipc/core.c
@@ -109,10 +109,9 @@ static void __net_exit tipc_exit_net(struct net *net)
 	struct tipc_net *tn = tipc_net(net);
 
 	tipc_detach_loopback(net);
+	tipc_net_stop(net);
 	/* Make sure the tipc_net_finalize_work() finished */
 	cancel_work_sync(&tn->work);
-	tipc_net_stop(net);
-
 	tipc_bcast_stop(net);
 	tipc_nametbl_stop(net);
 	tipc_sk_rht_destroy(net);
-- 
2.30.2

[tipc-discussion] [net-next] tipc: cleanup unused function

[Hoang Le <hoa...@de...>]

tipc_dest_list_len() is not being called anywhere. Clean it up.

Acked-by: Jon Maloy <jm...@re...>
Signed-off-by: Hoang Le <hoa...@de...>
---
 net/tipc/name_table.c | 11 -----------
 net/tipc/name_table.h |  1 -
 2 files changed, 12 deletions(-)

diff --git a/net/tipc/name_table.c b/net/tipc/name_table.c
index 1d8ba233d047..d1180370fdf4 100644
--- a/net/tipc/name_table.c
+++ b/net/tipc/name_table.c
@@ -1202,14 +1202,3 @@ void tipc_dest_list_purge(struct list_head *l)
 		kfree(dst);
 	}
 }
-
-int tipc_dest_list_len(struct list_head *l)
-{
-	struct tipc_dest *dst;
-	int i = 0;
-
-	list_for_each_entry(dst, l, list) {
-		i++;
-	}
-	return i;
-}
diff --git a/net/tipc/name_table.h b/net/tipc/name_table.h
index 259f95e3d99c..3bcd9ef8cee3 100644
--- a/net/tipc/name_table.h
+++ b/net/tipc/name_table.h
@@ -151,6 +151,5 @@ bool tipc_dest_push(struct list_head *l, u32 node, u32 port);
 bool tipc_dest_pop(struct list_head *l, u32 *node, u32 *port);
 bool tipc_dest_del(struct list_head *l, u32 node, u32 port);
 void tipc_dest_list_purge(struct list_head *l);
-int tipc_dest_list_len(struct list_head *l);
 
 #endif
-- 
2.30.2

Re: [tipc-discussion] TIPC Terminal Problem

[Jon M. <jm...@re...>]

On 6/16/22 06:26, Røysland, Jonas Gjendem via tipc-discussion wrote:
> Hey,
>
> We are working with the TIPC in the terminal and trying to connect a client and a server between two different linux machines. We are using the programs hello_client.c and hello_server.c to send and recieve to check if they are responding to each other. We are setting up the nodes and the bearer and are following the Getting Started section.
>
> To the problem: We are not sure how to set up the nodes in different machines and to link them within the same cluster in order to be able to communicate and sometimes even though we enable the bearers we don't get any of them in the link list.
Are you using UDP or bare Ethernet? In the latter case the two 
interfaces must be on the same subnet/vlan and have broadcast enabled.
Can you ping between the two ? Does Wireshark or tcpdump show anything?

> Is this a common hardware problem or problem itself. Have you any idea of this problem.
>
> Is there also a way to create the nodes from c code, like using the tipc api library.
No, not really. You could of course easily make one by invoking the 
'tipc' tool from c.
You could even do 'modprobe' from c if you want to, as long as your 
program has root access.

///jon

>
> Sincerly,
>
> Jonas Gjendem Røysland
>
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
>

Re: [tipc-discussion] [PATCH] tipc: cleanup unused function

[Jon M. <jm...@re...>]

On 6/16/22 05:32, Hoang Le wrote:
> tipc_dest_list_len() is not being called anywhere. Clean it up.
>
> Signed-off-by: Hoang Le <hoa...@de...>
> ---
>   net/tipc/name_table.c | 11 -----------
>   net/tipc/name_table.h |  1 -
>   2 files changed, 12 deletions(-)
>
> diff --git a/net/tipc/name_table.c b/net/tipc/name_table.c
> index 1d8ba233d047..d1180370fdf4 100644
> --- a/net/tipc/name_table.c
> +++ b/net/tipc/name_table.c
> @@ -1202,14 +1202,3 @@ void tipc_dest_list_purge(struct list_head *l)
>   		kfree(dst);
>   	}
>   }
> -
> -int tipc_dest_list_len(struct list_head *l)
> -{
> -	struct tipc_dest *dst;
> -	int i = 0;
> -
> -	list_for_each_entry(dst, l, list) {
> -		i++;
> -	}
> -	return i;
> -}
> diff --git a/net/tipc/name_table.h b/net/tipc/name_table.h
> index 259f95e3d99c..3bcd9ef8cee3 100644
> --- a/net/tipc/name_table.h
> +++ b/net/tipc/name_table.h
> @@ -151,6 +151,5 @@ bool tipc_dest_push(struct list_head *l, u32 node, u32 port);
>   bool tipc_dest_pop(struct list_head *l, u32 *node, u32 *port);
>   bool tipc_dest_del(struct list_head *l, u32 node, u32 port);
>   void tipc_dest_list_purge(struct list_head *l);
> -int tipc_dest_list_len(struct list_head *l);
>   
>   #endif
Acked-by: Jon Maloy <jm...@re...>

Re: [tipc-discussion] TIPC Communication

[Jon M. <jm...@re...>]

Hi,
There are som drawings in the protocol spec too:
http://tipc.io/protocol.html#anchor53

Some of the info in this spec is obsolete, but this one is still correct.

Med vennlig hilsen
///jon


On 6/15/22 04:10, Hoang Huu Le wrote:
> Hi,
>
> Please take an example at:
> https://sourceforge.net/p/tipc/tipcutils/ci/master/tree/demos/connection_demo/
>
> Regards,
> Hoang
>> -----Original Message-----
>> From: Røysland, Jonas Gjendem via tipc-discussion <tip...@li...>
>> Sent: Tuesday, June 14, 2022 8:22 PM
>> To: tip...@li...
>> Subject: [tipc-discussion] TIPC Communication
>>
>> Hey,
>>
>> We are some summer students that are working with the TIPC protocol for a project. We like to make a sequence diagram of TIPC to
>> better understand how the protocol communicate from the client to the server. Like in TCP it is using 3-way handshake to
>> communicate before sending data from one another. We really appreciete the help we could get to better understand the protocol.
>>
>> Sincerly,
>>
>> Jonas Gjendem Røysland
>>
>> _______________________________________________
>> tipc-discussion mailing list
>> tip...@li...
>> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
>
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
>

Re: [tipc-discussion] [PATCH] tipc: fix use-after-free Read in tipc_named_reinit

[Jon M. <jm...@re...>]

On 6/13/22 00:00, Hoang Huu Le wrote:
> Hi Jon, Ying,
>
> Just remind in case you guys missed this email thread.

Yes, I had missed it. It looks good to me.
///jon

>
> Thanks,
> Hoang
>> -----Original Message-----
>> From: Hoang Le <hoa...@de...>
>> Sent: Tuesday, June 7, 2022 2:35 PM
>> To: jm...@re...; ma...@do...; yin...@wi...; Tung Quang Nguyen <tun...@de...>;
>> tip...@li...
>> Cc: syz...@sy...
>> Subject: [tipc-discussion] [PATCH] tipc: fix use-after-free Read in tipc_named_reinit
>>
>> syzbot found the following issue on:
>> ==================================================================
>> BUG: KASAN: use-after-free in tipc_named_reinit+0x94f/0x9b0
>> net/tipc/name_distr.c:413
>> Read of size 8 at addr ffff88805299a000 by task kworker/1:9/23764
>>
>> CPU: 1 PID: 23764 Comm: kworker/1:9 Not tainted
>> 5.18.0-rc4-syzkaller-00878-g17d49e6e8012 #0
>> Hardware name: Google Compute Engine/Google Compute Engine,
>> BIOS Google 01/01/2011
>> Workqueue: events tipc_net_finalize_work
>> Call Trace:
>>   <TASK>
>>   __dump_stack lib/dump_stack.c:88 [inline]
>>   dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
>>   print_address_description.constprop.0.cold+0xeb/0x495
>> mm/kasan/report.c:313
>>   print_report mm/kasan/report.c:429 [inline]
>>   kasan_report.cold+0xf4/0x1c6 mm/kasan/report.c:491
>>   tipc_named_reinit+0x94f/0x9b0 net/tipc/name_distr.c:413
>>   tipc_net_finalize+0x234/0x3d0 net/tipc/net.c:138
>>   process_one_work+0x996/0x1610 kernel/workqueue.c:2289
>>   worker_thread+0x665/0x1080 kernel/workqueue.c:2436
>>   kthread+0x2e9/0x3a0 kernel/kthread.c:376
>>   ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:298
>>   </TASK>
>> [...]
>> ==================================================================
>>
>> In the commit
>> d966ddcc3821 ("tipc: fix a deadlock when flushing scheduled work"),
>> the cancel_work_sync() function just to make sure ONLY the work
>> tipc_net_finalize_work() is executing/pending on any CPU completed before
>> tipc namespace is destroyed through tipc_exit_net(). But this function
>> is not guaranteed the work is the last queued. So, the destroyed instance
>> may be accessed in the work which will try to enqueue later.
>>
>> In order to completely fix, we re-order the calling of cancel_work_sync()
>> to make sure the work tipc_net_finalize_work() was last queued and it
>> must be completed by calling cancel_work_sync().
>>
>> Reported-by: syz...@sy...
>> Fixes: d966ddcc3821 ("tipc: fix a deadlock when flushing scheduled work")
>> Signed-off-by: Ying Xue <yin...@wi...>
>> Signed-off-by: Hoang Le <hoa...@de...>
>> ---
>>   net/tipc/core.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/net/tipc/core.c b/net/tipc/core.c
>> index 3f4542e0f065..434e70eabe08 100644
>> --- a/net/tipc/core.c
>> +++ b/net/tipc/core.c
>> @@ -109,10 +109,9 @@ static void __net_exit tipc_exit_net(struct net *net)
>>   	struct tipc_net *tn = tipc_net(net);
>>
>>   	tipc_detach_loopback(net);
>> +	tipc_net_stop(net);
>>   	/* Make sure the tipc_net_finalize_work() finished */
>>   	cancel_work_sync(&tn->work);
>> -	tipc_net_stop(net);
>> -
>>   	tipc_bcast_stop(net);
>>   	tipc_nametbl_stop(net);
>>   	tipc_sk_rht_destroy(net);
>> --
>> 2.30.2
>>
>>
>>
>> _______________________________________________
>> tipc-discussion mailing list
>> tip...@li...
>> https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] TIPC Terminal Problem

[Røysland, J. G. <Jon...@ca...>]

Hey,

We are working with the TIPC in the terminal and trying to connect a client and a server between two different linux machines. We are using the programs hello_client.c and hello_server.c to send and recieve to check if they are responding to each other. We are setting up the nodes and the bearer and are following the Getting Started section.

To the problem: We are not sure how to set up the nodes in different machines and to link them within the same cluster in order to be able to communicate and sometimes even though we enable the bearers we don't get any of them in the link list. Is this a common hardware problem or problem itself. Have you any idea of this problem.

Is there also a way to create the nodes from c code, like using the tipc api library.

Sincerly,

Jonas Gjendem Røysland

[tipc-discussion] [PATCH] tipc: cleanup unused function

[Hoang Le <hoa...@de...>]

tipc_dest_list_len() is not being called anywhere. Clean it up.

Signed-off-by: Hoang Le <hoa...@de...>
---
 net/tipc/name_table.c | 11 -----------
 net/tipc/name_table.h |  1 -
 2 files changed, 12 deletions(-)

diff --git a/net/tipc/name_table.c b/net/tipc/name_table.c
index 1d8ba233d047..d1180370fdf4 100644
--- a/net/tipc/name_table.c
+++ b/net/tipc/name_table.c
@@ -1202,14 +1202,3 @@ void tipc_dest_list_purge(struct list_head *l)
 		kfree(dst);
 	}
 }
-
-int tipc_dest_list_len(struct list_head *l)
-{
-	struct tipc_dest *dst;
-	int i = 0;
-
-	list_for_each_entry(dst, l, list) {
-		i++;
-	}
-	return i;
-}
diff --git a/net/tipc/name_table.h b/net/tipc/name_table.h
index 259f95e3d99c..3bcd9ef8cee3 100644
--- a/net/tipc/name_table.h
+++ b/net/tipc/name_table.h
@@ -151,6 +151,5 @@ bool tipc_dest_push(struct list_head *l, u32 node, u32 port);
 bool tipc_dest_pop(struct list_head *l, u32 *node, u32 *port);
 bool tipc_dest_del(struct list_head *l, u32 node, u32 port);
 void tipc_dest_list_purge(struct list_head *l);
-int tipc_dest_list_len(struct list_head *l);
 
 #endif
-- 
2.30.2

Re: [tipc-discussion] TIPC Communication

[Hoang H. Le <hoa...@de...>]

Hi,

Please take an example at:
https://sourceforge.net/p/tipc/tipcutils/ci/master/tree/demos/connection_demo/

Regards,
Hoang
> -----Original Message-----
> From: Røysland, Jonas Gjendem via tipc-discussion <tip...@li...>
> Sent: Tuesday, June 14, 2022 8:22 PM
> To: tip...@li...
> Subject: [tipc-discussion] TIPC Communication
> 
> Hey,
> 
> We are some summer students that are working with the TIPC protocol for a project. We like to make a sequence diagram of TIPC to
> better understand how the protocol communicate from the client to the server. Like in TCP it is using 3-way handshake to
> communicate before sending data from one another. We really appreciete the help we could get to better understand the protocol.
> 
> Sincerly,
> 
> Jonas Gjendem Røysland
> 
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] TIPC Communication

[Røysland, J. G. <Jon...@ca...>]

Hey,

We are some summer students that are working with the TIPC protocol for a project. We like to make a sequence diagram of TIPC to better understand how the protocol communicate from the client to the server. Like in TCP it is using 3-way handshake to communicate before sending data from one another. We really appreciete the help we could get to better understand the protocol.

Sincerly,

Jonas Gjendem Røysland

Re: [tipc-discussion] [PATCH] tipc: fix use-after-free Read in tipc_named_reinit

[Hoang H. Le <hoa...@de...>]

Hi Jon, Ying,

Just remind in case you guys missed this email thread.

Thanks,
Hoang
> -----Original Message-----
> From: Hoang Le <hoa...@de...>
> Sent: Tuesday, June 7, 2022 2:35 PM
> To: jm...@re...; ma...@do...; yin...@wi...; Tung Quang Nguyen <tun...@de...>;
> tip...@li...
> Cc: syz...@sy...
> Subject: [tipc-discussion] [PATCH] tipc: fix use-after-free Read in tipc_named_reinit
> 
> syzbot found the following issue on:
> ==================================================================
> BUG: KASAN: use-after-free in tipc_named_reinit+0x94f/0x9b0
> net/tipc/name_distr.c:413
> Read of size 8 at addr ffff88805299a000 by task kworker/1:9/23764
> 
> CPU: 1 PID: 23764 Comm: kworker/1:9 Not tainted
> 5.18.0-rc4-syzkaller-00878-g17d49e6e8012 #0
> Hardware name: Google Compute Engine/Google Compute Engine,
> BIOS Google 01/01/2011
> Workqueue: events tipc_net_finalize_work
> Call Trace:
>  <TASK>
>  __dump_stack lib/dump_stack.c:88 [inline]
>  dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
>  print_address_description.constprop.0.cold+0xeb/0x495
> mm/kasan/report.c:313
>  print_report mm/kasan/report.c:429 [inline]
>  kasan_report.cold+0xf4/0x1c6 mm/kasan/report.c:491
>  tipc_named_reinit+0x94f/0x9b0 net/tipc/name_distr.c:413
>  tipc_net_finalize+0x234/0x3d0 net/tipc/net.c:138
>  process_one_work+0x996/0x1610 kernel/workqueue.c:2289
>  worker_thread+0x665/0x1080 kernel/workqueue.c:2436
>  kthread+0x2e9/0x3a0 kernel/kthread.c:376
>  ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:298
>  </TASK>
> [...]
> ==================================================================
> 
> In the commit
> d966ddcc3821 ("tipc: fix a deadlock when flushing scheduled work"),
> the cancel_work_sync() function just to make sure ONLY the work
> tipc_net_finalize_work() is executing/pending on any CPU completed before
> tipc namespace is destroyed through tipc_exit_net(). But this function
> is not guaranteed the work is the last queued. So, the destroyed instance
> may be accessed in the work which will try to enqueue later.
> 
> In order to completely fix, we re-order the calling of cancel_work_sync()
> to make sure the work tipc_net_finalize_work() was last queued and it
> must be completed by calling cancel_work_sync().
> 
> Reported-by: syz...@sy...
> Fixes: d966ddcc3821 ("tipc: fix a deadlock when flushing scheduled work")
> Signed-off-by: Ying Xue <yin...@wi...>
> Signed-off-by: Hoang Le <hoa...@de...>
> ---
>  net/tipc/core.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/net/tipc/core.c b/net/tipc/core.c
> index 3f4542e0f065..434e70eabe08 100644
> --- a/net/tipc/core.c
> +++ b/net/tipc/core.c
> @@ -109,10 +109,9 @@ static void __net_exit tipc_exit_net(struct net *net)
>  	struct tipc_net *tn = tipc_net(net);
> 
>  	tipc_detach_loopback(net);
> +	tipc_net_stop(net);
>  	/* Make sure the tipc_net_finalize_work() finished */
>  	cancel_work_sync(&tn->work);
> -	tipc_net_stop(net);
> -
>  	tipc_bcast_stop(net);
>  	tipc_nametbl_stop(net);
>  	tipc_sk_rht_destroy(net);
> --
> 2.30.2
> 
> 
> 
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [PATCH net-next] tipc: remove inputq from tipc_bc_base

[Xin L. <luc...@gm...>]

On Mon, Jun 6, 2022 at 11:20 PM Tung Quang Nguyen
<tun...@de...> wrote:
>
> > -----Original Message-----
> > From: Xin Long <luc...@gm...>
> > Sent: Tuesday, June 7, 2022 12:57 AM
> > To: tip...@li...
> > Subject: Re: [tipc-discussion] [PATCH net-next] tipc: remove inputq from tipc_bc_base
> >
> > fix Jon's email address.
> >
> > On Mon, Jun 6, 2022 at 1:52 PM Xin Long <luc...@gm...> wrote:
> > >
> > > After Commit 2af5ae372a4b ("tipc: clean up unused code and structures"),
> > > there is no place really using tn->bcbase->inputq. This patch is to
> > > delete this member from struct tipc_bc_base.
>
> We cannot delete this queue because it is currently used to contain wakeup messages for broadcast send link.
> See this calling flow: tipc_rcv() --> tipc_node_bc_sync_rcv() --> tipc_bcast_sync_rcv() --> tipc_link_bc_ack_rcv() --> link_prepare_wakeup()
> link_prepare_wakeup() copies wakeup messages from the wakeup queue to the tipc_bc_base->inputq. Then, the wakeup is done in tipc_bcast_sync_rcv()
>
You're right, thanks!

[tipc-discussion] [PATCH] tipc: fix use-after-free Read in tipc_named_reinit

[Hoang Le <hoa...@de...>]

syzbot found the following issue on:
==================================================================
BUG: KASAN: use-after-free in tipc_named_reinit+0x94f/0x9b0
net/tipc/name_distr.c:413
Read of size 8 at addr ffff88805299a000 by task kworker/1:9/23764

CPU: 1 PID: 23764 Comm: kworker/1:9 Not tainted
5.18.0-rc4-syzkaller-00878-g17d49e6e8012 #0
Hardware name: Google Compute Engine/Google Compute Engine,
BIOS Google 01/01/2011
Workqueue: events tipc_net_finalize_work
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:88 [inline]
 dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
 print_address_description.constprop.0.cold+0xeb/0x495
mm/kasan/report.c:313
 print_report mm/kasan/report.c:429 [inline]
 kasan_report.cold+0xf4/0x1c6 mm/kasan/report.c:491
 tipc_named_reinit+0x94f/0x9b0 net/tipc/name_distr.c:413
 tipc_net_finalize+0x234/0x3d0 net/tipc/net.c:138
 process_one_work+0x996/0x1610 kernel/workqueue.c:2289
 worker_thread+0x665/0x1080 kernel/workqueue.c:2436
 kthread+0x2e9/0x3a0 kernel/kthread.c:376
 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:298
 </TASK>
[...]
==================================================================

In the commit
d966ddcc3821 ("tipc: fix a deadlock when flushing scheduled work"),
the cancel_work_sync() function just to make sure ONLY the work
tipc_net_finalize_work() is executing/pending on any CPU completed before
tipc namespace is destroyed through tipc_exit_net(). But this function
is not guaranteed the work is the last queued. So, the destroyed instance
may be accessed in the work which will try to enqueue later.

In order to completely fix, we re-order the calling of cancel_work_sync()
to make sure the work tipc_net_finalize_work() was last queued and it
must be completed by calling cancel_work_sync().

Reported-by: syz...@sy...
Fixes: d966ddcc3821 ("tipc: fix a deadlock when flushing scheduled work")
Signed-off-by: Ying Xue <yin...@wi...>
Signed-off-by: Hoang Le <hoa...@de...>
---
 net/tipc/core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/tipc/core.c b/net/tipc/core.c
index 3f4542e0f065..434e70eabe08 100644
--- a/net/tipc/core.c
+++ b/net/tipc/core.c
@@ -109,10 +109,9 @@ static void __net_exit tipc_exit_net(struct net *net)
 	struct tipc_net *tn = tipc_net(net);
 
 	tipc_detach_loopback(net);
+	tipc_net_stop(net);
 	/* Make sure the tipc_net_finalize_work() finished */
 	cancel_work_sync(&tn->work);
-	tipc_net_stop(net);
-
 	tipc_bcast_stop(net);
 	tipc_nametbl_stop(net);
 	tipc_sk_rht_destroy(net);
-- 
2.30.2

Re: [tipc-discussion] [PATCH net-next] tipc: remove inputq from tipc_bc_base

[Tung Q. N. <tun...@de...>]

> -----Original Message-----
> From: Xin Long <luc...@gm...>
> Sent: Tuesday, June 7, 2022 12:57 AM
> To: tip...@li...
> Subject: Re: [tipc-discussion] [PATCH net-next] tipc: remove inputq from tipc_bc_base
> 
> fix Jon's email address.
> 
> On Mon, Jun 6, 2022 at 1:52 PM Xin Long <luc...@gm...> wrote:
> >
> > After Commit 2af5ae372a4b ("tipc: clean up unused code and structures"),
> > there is no place really using tn->bcbase->inputq. This patch is to
> > delete this member from struct tipc_bc_base.

We cannot delete this queue because it is currently used to contain wakeup messages for broadcast send link.
See this calling flow: tipc_rcv() --> tipc_node_bc_sync_rcv() --> tipc_bcast_sync_rcv() --> tipc_link_bc_ack_rcv() --> link_prepare_wakeup()
link_prepare_wakeup() copies wakeup messages from the wakeup queue to the tipc_bc_base->inputq. Then, the wakeup is done in tipc_bcast_sync_rcv()

Re: [tipc-discussion] [PATCH net-next] tipc: remove inputq from tipc_bc_base

[Hoang H. Le <hoa...@de...>]

> -----Original Message-----
> From: Xin Long <luc...@gm...>
> Sent: Tuesday, June 7, 2022 12:57 AM
> To: tip...@li...
> Subject: Re: [tipc-discussion] [PATCH net-next] tipc: remove inputq from tipc_bc_base
> 
> fix Jon's email address.
> 
> On Mon, Jun 6, 2022 at 1:52 PM Xin Long <luc...@gm...> wrote:
> >
> > After Commit 2af5ae372a4b ("tipc: clean up unused code and structures"),
> > there is no place really using tn->bcbase->inputq. This patch is to
> > delete this member from struct tipc_bc_base.
> >
> > Signed-off-by: Xin Long <luc...@gm...>
> > ---
> >  net/tipc/bcast.c | 22 +---------------------
> >  1 file changed, 1 insertion(+), 21 deletions(-)
> >
> > diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
> > index 593846d25214..2293f6caa682 100644
> > --- a/net/tipc/bcast.c
> > +++ b/net/tipc/bcast.c
> > @@ -63,7 +63,6 @@ unsigned long sysctl_tipc_bc_retruni __read_mostly;
> >   */
> >  struct tipc_bc_base {
> >         struct tipc_link *link;
> > -       struct sk_buff_head inputq;
> >         int dests[MAX_BEARERS];
> >         int primary_bearer;
> >         bool bcast_support;
> > @@ -436,7 +435,6 @@ int tipc_mcast_xmit(struct net *net, struct sk_buff_head *pkts,
> >  int tipc_bcast_rcv(struct net *net, struct tipc_link *l, struct sk_buff *skb)
> >  {
> >         struct tipc_msg *hdr = buf_msg(skb);
> > -       struct sk_buff_head *inputq = &tipc_bc_base(net)->inputq;
> >         struct sk_buff_head xmitq;
> >         int rc;
> >
> > @@ -456,10 +454,6 @@ int tipc_bcast_rcv(struct net *net, struct tipc_link *l, struct sk_buff *skb)
> >
> >         tipc_bcbase_xmit(net, &xmitq);
> >
> > -       /* Any socket wakeup messages ? */
> > -       if (!skb_queue_empty(inputq))
> > -               tipc_sk_rcv(net, inputq);
> > -
> >         return rc;
> >  }
> >
> > @@ -470,7 +464,6 @@ int tipc_bcast_rcv(struct net *net, struct tipc_link *l, struct sk_buff *skb)
> >  void tipc_bcast_ack_rcv(struct net *net, struct tipc_link *l,
> >                         struct tipc_msg *hdr)
> >  {
> > -       struct sk_buff_head *inputq = &tipc_bc_base(net)->inputq;
> >         u16 acked = msg_bcast_ack(hdr);
> >         struct sk_buff_head xmitq;
> >
> > @@ -485,10 +478,6 @@ void tipc_bcast_ack_rcv(struct net *net, struct tipc_link *l,
> >         tipc_bcast_unlock(net);
> >
> >         tipc_bcbase_xmit(net, &xmitq);
> > -
> > -       /* Any socket wakeup messages ? */
> > -       if (!skb_queue_empty(inputq))
> > -               tipc_sk_rcv(net, inputq);
> >  }
> >
> >  /* tipc_bcast_synch_rcv -  check and update rcv link with peer's send state
> > @@ -499,7 +488,6 @@ int tipc_bcast_sync_rcv(struct net *net, struct tipc_link *l,
> >                         struct tipc_msg *hdr,
> >                         struct sk_buff_head *retrq)
> >  {
> > -       struct sk_buff_head *inputq = &tipc_bc_base(net)->inputq;
> >         struct tipc_gap_ack_blks *ga;
> >         struct sk_buff_head xmitq;
> >         int rc = 0;
> > @@ -522,9 +510,6 @@ int tipc_bcast_sync_rcv(struct net *net, struct tipc_link *l,
> >
> >         tipc_bcbase_xmit(net, &xmitq);
> >
> > -       /* Any socket wakeup messages ? */
> > -       if (!skb_queue_empty(inputq))
> > -               tipc_sk_rcv(net, inputq);
> >         return rc;
> >  }
> >
> > @@ -551,7 +536,6 @@ void tipc_bcast_add_peer(struct net *net, struct tipc_link *uc_l,
> >  void tipc_bcast_remove_peer(struct net *net, struct tipc_link *rcv_l)
> >  {
> >         struct tipc_link *snd_l = tipc_bc_sndlink(net);
> > -       struct sk_buff_head *inputq = &tipc_bc_base(net)->inputq;
> >         struct sk_buff_head xmitq;
> >
> >         __skb_queue_head_init(&xmitq);
> > @@ -563,10 +547,6 @@ void tipc_bcast_remove_peer(struct net *net, struct tipc_link *rcv_l)
> >         tipc_bcast_unlock(net);
> >
> >         tipc_bcbase_xmit(net, &xmitq);
> > -
> > -       /* Any socket wakeup messages ? */
> > -       if (!skb_queue_empty(inputq))
> > -               tipc_sk_rcv(net, inputq);
> >  }
> >
> >  int tipc_bclink_reset_stats(struct net *net, struct tipc_link *l)
> > @@ -703,7 +683,7 @@ int tipc_bcast_init(struct net *net)
> >                                  BCLINK_WIN_DEFAULT,
> >                                  BCLINK_WIN_DEFAULT,
> >                                  0,
> > -                                &bb->inputq,
> > +                                NULL,
> >                                  NULL,
> >                                  NULL,
> >                                  &l))
> > --
> > 2.31.1
> >
> 
> 
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Please also remove kernel-doc comment for this member too.
Regards,
Hoang

Re: [tipc-discussion] [PATCH net-next] tipc: remove inputq from tipc_bc_base

[Xin L. <luc...@gm...>]

fix Jon's email address.

On Mon, Jun 6, 2022 at 1:52 PM Xin Long <luc...@gm...> wrote:
>
> After Commit 2af5ae372a4b ("tipc: clean up unused code and structures"),
> there is no place really using tn->bcbase->inputq. This patch is to
> delete this member from struct tipc_bc_base.
>
> Signed-off-by: Xin Long <luc...@gm...>
> ---
>  net/tipc/bcast.c | 22 +---------------------
>  1 file changed, 1 insertion(+), 21 deletions(-)
>
> diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
> index 593846d25214..2293f6caa682 100644
> --- a/net/tipc/bcast.c
> +++ b/net/tipc/bcast.c
> @@ -63,7 +63,6 @@ unsigned long sysctl_tipc_bc_retruni __read_mostly;
>   */
>  struct tipc_bc_base {
>         struct tipc_link *link;
> -       struct sk_buff_head inputq;
>         int dests[MAX_BEARERS];
>         int primary_bearer;
>         bool bcast_support;
> @@ -436,7 +435,6 @@ int tipc_mcast_xmit(struct net *net, struct sk_buff_head *pkts,
>  int tipc_bcast_rcv(struct net *net, struct tipc_link *l, struct sk_buff *skb)
>  {
>         struct tipc_msg *hdr = buf_msg(skb);
> -       struct sk_buff_head *inputq = &tipc_bc_base(net)->inputq;
>         struct sk_buff_head xmitq;
>         int rc;
>
> @@ -456,10 +454,6 @@ int tipc_bcast_rcv(struct net *net, struct tipc_link *l, struct sk_buff *skb)
>
>         tipc_bcbase_xmit(net, &xmitq);
>
> -       /* Any socket wakeup messages ? */
> -       if (!skb_queue_empty(inputq))
> -               tipc_sk_rcv(net, inputq);
> -
>         return rc;
>  }
>
> @@ -470,7 +464,6 @@ int tipc_bcast_rcv(struct net *net, struct tipc_link *l, struct sk_buff *skb)
>  void tipc_bcast_ack_rcv(struct net *net, struct tipc_link *l,
>                         struct tipc_msg *hdr)
>  {
> -       struct sk_buff_head *inputq = &tipc_bc_base(net)->inputq;
>         u16 acked = msg_bcast_ack(hdr);
>         struct sk_buff_head xmitq;
>
> @@ -485,10 +478,6 @@ void tipc_bcast_ack_rcv(struct net *net, struct tipc_link *l,
>         tipc_bcast_unlock(net);
>
>         tipc_bcbase_xmit(net, &xmitq);
> -
> -       /* Any socket wakeup messages ? */
> -       if (!skb_queue_empty(inputq))
> -               tipc_sk_rcv(net, inputq);
>  }
>
>  /* tipc_bcast_synch_rcv -  check and update rcv link with peer's send state
> @@ -499,7 +488,6 @@ int tipc_bcast_sync_rcv(struct net *net, struct tipc_link *l,
>                         struct tipc_msg *hdr,
>                         struct sk_buff_head *retrq)
>  {
> -       struct sk_buff_head *inputq = &tipc_bc_base(net)->inputq;
>         struct tipc_gap_ack_blks *ga;
>         struct sk_buff_head xmitq;
>         int rc = 0;
> @@ -522,9 +510,6 @@ int tipc_bcast_sync_rcv(struct net *net, struct tipc_link *l,
>
>         tipc_bcbase_xmit(net, &xmitq);
>
> -       /* Any socket wakeup messages ? */
> -       if (!skb_queue_empty(inputq))
> -               tipc_sk_rcv(net, inputq);
>         return rc;
>  }
>
> @@ -551,7 +536,6 @@ void tipc_bcast_add_peer(struct net *net, struct tipc_link *uc_l,
>  void tipc_bcast_remove_peer(struct net *net, struct tipc_link *rcv_l)
>  {
>         struct tipc_link *snd_l = tipc_bc_sndlink(net);
> -       struct sk_buff_head *inputq = &tipc_bc_base(net)->inputq;
>         struct sk_buff_head xmitq;
>
>         __skb_queue_head_init(&xmitq);
> @@ -563,10 +547,6 @@ void tipc_bcast_remove_peer(struct net *net, struct tipc_link *rcv_l)
>         tipc_bcast_unlock(net);
>
>         tipc_bcbase_xmit(net, &xmitq);
> -
> -       /* Any socket wakeup messages ? */
> -       if (!skb_queue_empty(inputq))
> -               tipc_sk_rcv(net, inputq);
>  }
>
>  int tipc_bclink_reset_stats(struct net *net, struct tipc_link *l)
> @@ -703,7 +683,7 @@ int tipc_bcast_init(struct net *net)
>                                  BCLINK_WIN_DEFAULT,
>                                  BCLINK_WIN_DEFAULT,
>                                  0,
> -                                &bb->inputq,
> +                                NULL,
>                                  NULL,
>                                  NULL,
>                                  &l))
> --
> 2.31.1
>

Re: [tipc-discussion] [PATCH net] tipc: move bc link creation back to tipc_node_create

[Xin L. <luc...@gm...>]

fix Jon's email address.

On Mon, Jun 6, 2022 at 11:24 AM Xin Long <luc...@gm...> wrote:
>
> Shuang Li reported a NULL pointer dereference crash:
>
>   [] BUG: kernel NULL pointer dereference, address: 0000000000000068
>   [] RIP: 0010:tipc_link_is_up+0x5/0x10 [tipc]
>   [] Call Trace:
>   []  <IRQ>
>   []  tipc_bcast_rcv+0xa2/0x190 [tipc]
>   []  tipc_node_bc_rcv+0x8b/0x200 [tipc]
>   []  tipc_rcv+0x3af/0x5b0 [tipc]
>   []  tipc_udp_recv+0xc7/0x1e0 [tipc]
>
> It was caused by the 'l' passed into tipc_bcast_rcv() is NULL. When it
> creates a node in tipc_node_check_dest(), after inserting the new node
> into hashtable in tipc_node_create(), it creates the bc link. However,
> there is a gap between this insert and bc link creation, a bc packet
> may come in and get the node from the hashtable then try to dereference
> its bc link, which is NULL.
>
> This patch is to fix it by moving the bc link creation before inserting
> into the hashtable.
>
> Note that for a preliminary node becoming "real", the bc link creation
> should also be called before it's rehashed, as we don't create it for
> preliminary nodes.
>
> Fixes: 4cbf8ac2fe5a ("tipc: enable creating a "preliminary" node")
> Reported-by: Shuang Li <sh...@re...>
> Signed-off-by: Xin Long <luc...@gm...>
> ---
>  net/tipc/node.c | 41 ++++++++++++++++++++++-------------------
>  1 file changed, 22 insertions(+), 19 deletions(-)
>
> diff --git a/net/tipc/node.c b/net/tipc/node.c
> index 6ef95ce565bd..b48d97cbbe29 100644
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -472,8 +472,8 @@ struct tipc_node *tipc_node_create(struct net *net, u32 addr, u8 *peer_id,
>                                    bool preliminary)
>  {
>         struct tipc_net *tn = net_generic(net, tipc_net_id);
> +       struct tipc_link *l, *snd_l = tipc_bc_sndlink(net);
>         struct tipc_node *n, *temp_node;
> -       struct tipc_link *l;
>         unsigned long intv;
>         int bearer_id;
>         int i;
> @@ -488,6 +488,16 @@ struct tipc_node *tipc_node_create(struct net *net, u32 addr, u8 *peer_id,
>                         goto exit;
>                 /* A preliminary node becomes "real" now, refresh its data */
>                 tipc_node_write_lock(n);
> +               if (!tipc_link_bc_create(net, tipc_own_addr(net), addr, peer_id, U16_MAX,
> +                                        tipc_link_min_win(snd_l), tipc_link_max_win(snd_l),
> +                                        n->capabilities, &n->bc_entry.inputq1,
> +                                        &n->bc_entry.namedq, snd_l, &n->bc_entry.link)) {
> +                       pr_warn("Broadcast rcv link refresh failed, no memory\n");
> +                       tipc_node_write_unlock_fast(n);
> +                       tipc_node_put(n);
> +                       n = NULL;
> +                       goto exit;
> +               }
>                 n->preliminary = false;
>                 n->addr = addr;
>                 hlist_del_rcu(&n->hash);
> @@ -567,7 +577,16 @@ struct tipc_node *tipc_node_create(struct net *net, u32 addr, u8 *peer_id,
>         n->signature = INVALID_NODE_SIG;
>         n->active_links[0] = INVALID_BEARER_ID;
>         n->active_links[1] = INVALID_BEARER_ID;
> -       n->bc_entry.link = NULL;
> +       if (!preliminary &&
> +           !tipc_link_bc_create(net, tipc_own_addr(net), addr, peer_id, U16_MAX,
> +                                tipc_link_min_win(snd_l), tipc_link_max_win(snd_l),
> +                                n->capabilities, &n->bc_entry.inputq1,
> +                                &n->bc_entry.namedq, snd_l, &n->bc_entry.link)) {
> +               pr_warn("Broadcast rcv link creation failed, no memory\n");
> +               kfree(n);
> +               n = NULL;
> +               goto exit;
> +       }
>         tipc_node_get(n);
>         timer_setup(&n->timer, tipc_node_timeout, 0);
>         /* Start a slow timer anyway, crypto needs it */
> @@ -1155,7 +1174,7 @@ void tipc_node_check_dest(struct net *net, u32 addr,
>                           bool *respond, bool *dupl_addr)
>  {
>         struct tipc_node *n;
> -       struct tipc_link *l, *snd_l;
> +       struct tipc_link *l;
>         struct tipc_link_entry *le;
>         bool addr_match = false;
>         bool sign_match = false;
> @@ -1175,22 +1194,6 @@ void tipc_node_check_dest(struct net *net, u32 addr,
>                 return;
>
>         tipc_node_write_lock(n);
> -       if (unlikely(!n->bc_entry.link)) {
> -               snd_l = tipc_bc_sndlink(net);
> -               if (!tipc_link_bc_create(net, tipc_own_addr(net),
> -                                        addr, peer_id, U16_MAX,
> -                                        tipc_link_min_win(snd_l),
> -                                        tipc_link_max_win(snd_l),
> -                                        n->capabilities,
> -                                        &n->bc_entry.inputq1,
> -                                        &n->bc_entry.namedq, snd_l,
> -                                        &n->bc_entry.link)) {
> -                       pr_warn("Broadcast rcv link creation failed, no mem\n");
> -                       tipc_node_write_unlock_fast(n);
> -                       tipc_node_put(n);
> -                       return;
> -               }
> -       }
>
>         le = &n->links[b->identity];
>
> --
> 2.31.1
>

[tipc-discussion] [PATCH net-next] tipc: remove inputq from tipc_bc_base

[Xin L. <luc...@gm...>]

After Commit 2af5ae372a4b ("tipc: clean up unused code and structures"),
there is no place really using tn->bcbase->inputq. This patch is to
delete this member from struct tipc_bc_base.

Signed-off-by: Xin Long <luc...@gm...>
---
 net/tipc/bcast.c | 22 +---------------------
 1 file changed, 1 insertion(+), 21 deletions(-)

diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
index 593846d25214..2293f6caa682 100644
--- a/net/tipc/bcast.c
+++ b/net/tipc/bcast.c
@@ -63,7 +63,6 @@ unsigned long sysctl_tipc_bc_retruni __read_mostly;
  */
 struct tipc_bc_base {
 	struct tipc_link *link;
-	struct sk_buff_head inputq;
 	int dests[MAX_BEARERS];
 	int primary_bearer;
 	bool bcast_support;
@@ -436,7 +435,6 @@ int tipc_mcast_xmit(struct net *net, struct sk_buff_head *pkts,
 int tipc_bcast_rcv(struct net *net, struct tipc_link *l, struct sk_buff *skb)
 {
 	struct tipc_msg *hdr = buf_msg(skb);
-	struct sk_buff_head *inputq = &tipc_bc_base(net)->inputq;
 	struct sk_buff_head xmitq;
 	int rc;
 
@@ -456,10 +454,6 @@ int tipc_bcast_rcv(struct net *net, struct tipc_link *l, struct sk_buff *skb)
 
 	tipc_bcbase_xmit(net, &xmitq);
 
-	/* Any socket wakeup messages ? */
-	if (!skb_queue_empty(inputq))
-		tipc_sk_rcv(net, inputq);
-
 	return rc;
 }
 
@@ -470,7 +464,6 @@ int tipc_bcast_rcv(struct net *net, struct tipc_link *l, struct sk_buff *skb)
 void tipc_bcast_ack_rcv(struct net *net, struct tipc_link *l,
 			struct tipc_msg *hdr)
 {
-	struct sk_buff_head *inputq = &tipc_bc_base(net)->inputq;
 	u16 acked = msg_bcast_ack(hdr);
 	struct sk_buff_head xmitq;
 
@@ -485,10 +478,6 @@ void tipc_bcast_ack_rcv(struct net *net, struct tipc_link *l,
 	tipc_bcast_unlock(net);
 
 	tipc_bcbase_xmit(net, &xmitq);
-
-	/* Any socket wakeup messages ? */
-	if (!skb_queue_empty(inputq))
-		tipc_sk_rcv(net, inputq);
 }
 
 /* tipc_bcast_synch_rcv -  check and update rcv link with peer's send state
@@ -499,7 +488,6 @@ int tipc_bcast_sync_rcv(struct net *net, struct tipc_link *l,
 			struct tipc_msg *hdr,
 			struct sk_buff_head *retrq)
 {
-	struct sk_buff_head *inputq = &tipc_bc_base(net)->inputq;
 	struct tipc_gap_ack_blks *ga;
 	struct sk_buff_head xmitq;
 	int rc = 0;
@@ -522,9 +510,6 @@ int tipc_bcast_sync_rcv(struct net *net, struct tipc_link *l,
 
 	tipc_bcbase_xmit(net, &xmitq);
 
-	/* Any socket wakeup messages ? */
-	if (!skb_queue_empty(inputq))
-		tipc_sk_rcv(net, inputq);
 	return rc;
 }
 
@@ -551,7 +536,6 @@ void tipc_bcast_add_peer(struct net *net, struct tipc_link *uc_l,
 void tipc_bcast_remove_peer(struct net *net, struct tipc_link *rcv_l)
 {
 	struct tipc_link *snd_l = tipc_bc_sndlink(net);
-	struct sk_buff_head *inputq = &tipc_bc_base(net)->inputq;
 	struct sk_buff_head xmitq;
 
 	__skb_queue_head_init(&xmitq);
@@ -563,10 +547,6 @@ void tipc_bcast_remove_peer(struct net *net, struct tipc_link *rcv_l)
 	tipc_bcast_unlock(net);
 
 	tipc_bcbase_xmit(net, &xmitq);
-
-	/* Any socket wakeup messages ? */
-	if (!skb_queue_empty(inputq))
-		tipc_sk_rcv(net, inputq);
 }
 
 int tipc_bclink_reset_stats(struct net *net, struct tipc_link *l)
@@ -703,7 +683,7 @@ int tipc_bcast_init(struct net *net)
 				 BCLINK_WIN_DEFAULT,
 				 BCLINK_WIN_DEFAULT,
 				 0,
-				 &bb->inputq,
+				 NULL,
 				 NULL,
 				 NULL,
 				 &l))
-- 
2.31.1

[tipc-discussion] [PATCH net] tipc: move bc link creation back to tipc_node_create

[Xin L. <luc...@gm...>]

Shuang Li reported a NULL pointer dereference crash:

  [] BUG: kernel NULL pointer dereference, address: 0000000000000068
  [] RIP: 0010:tipc_link_is_up+0x5/0x10 [tipc]
  [] Call Trace:
  []  <IRQ>
  []  tipc_bcast_rcv+0xa2/0x190 [tipc]
  []  tipc_node_bc_rcv+0x8b/0x200 [tipc]
  []  tipc_rcv+0x3af/0x5b0 [tipc]
  []  tipc_udp_recv+0xc7/0x1e0 [tipc]

It was caused by the 'l' passed into tipc_bcast_rcv() is NULL. When it
creates a node in tipc_node_check_dest(), after inserting the new node
into hashtable in tipc_node_create(), it creates the bc link. However,
there is a gap between this insert and bc link creation, a bc packet
may come in and get the node from the hashtable then try to dereference
its bc link, which is NULL.

This patch is to fix it by moving the bc link creation before inserting
into the hashtable.

Note that for a preliminary node becoming "real", the bc link creation
should also be called before it's rehashed, as we don't create it for
preliminary nodes.

Fixes: 4cbf8ac2fe5a ("tipc: enable creating a "preliminary" node")
Reported-by: Shuang Li <sh...@re...>
Signed-off-by: Xin Long <luc...@gm...>
---
 net/tipc/node.c | 41 ++++++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/net/tipc/node.c b/net/tipc/node.c
index 6ef95ce565bd..b48d97cbbe29 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -472,8 +472,8 @@ struct tipc_node *tipc_node_create(struct net *net, u32 addr, u8 *peer_id,
 				   bool preliminary)
 {
 	struct tipc_net *tn = net_generic(net, tipc_net_id);
+	struct tipc_link *l, *snd_l = tipc_bc_sndlink(net);
 	struct tipc_node *n, *temp_node;
-	struct tipc_link *l;
 	unsigned long intv;
 	int bearer_id;
 	int i;
@@ -488,6 +488,16 @@ struct tipc_node *tipc_node_create(struct net *net, u32 addr, u8 *peer_id,
 			goto exit;
 		/* A preliminary node becomes "real" now, refresh its data */
 		tipc_node_write_lock(n);
+		if (!tipc_link_bc_create(net, tipc_own_addr(net), addr, peer_id, U16_MAX,
+					 tipc_link_min_win(snd_l), tipc_link_max_win(snd_l),
+					 n->capabilities, &n->bc_entry.inputq1,
+					 &n->bc_entry.namedq, snd_l, &n->bc_entry.link)) {
+			pr_warn("Broadcast rcv link refresh failed, no memory\n");
+			tipc_node_write_unlock_fast(n);
+			tipc_node_put(n);
+			n = NULL;
+			goto exit;
+		}
 		n->preliminary = false;
 		n->addr = addr;
 		hlist_del_rcu(&n->hash);
@@ -567,7 +577,16 @@ struct tipc_node *tipc_node_create(struct net *net, u32 addr, u8 *peer_id,
 	n->signature = INVALID_NODE_SIG;
 	n->active_links[0] = INVALID_BEARER_ID;
 	n->active_links[1] = INVALID_BEARER_ID;
-	n->bc_entry.link = NULL;
+	if (!preliminary &&
+	    !tipc_link_bc_create(net, tipc_own_addr(net), addr, peer_id, U16_MAX,
+				 tipc_link_min_win(snd_l), tipc_link_max_win(snd_l),
+				 n->capabilities, &n->bc_entry.inputq1,
+				 &n->bc_entry.namedq, snd_l, &n->bc_entry.link)) {
+		pr_warn("Broadcast rcv link creation failed, no memory\n");
+		kfree(n);
+		n = NULL;
+		goto exit;
+	}
 	tipc_node_get(n);
 	timer_setup(&n->timer, tipc_node_timeout, 0);
 	/* Start a slow timer anyway, crypto needs it */
@@ -1155,7 +1174,7 @@ void tipc_node_check_dest(struct net *net, u32 addr,
 			  bool *respond, bool *dupl_addr)
 {
 	struct tipc_node *n;
-	struct tipc_link *l, *snd_l;
+	struct tipc_link *l;
 	struct tipc_link_entry *le;
 	bool addr_match = false;
 	bool sign_match = false;
@@ -1175,22 +1194,6 @@ void tipc_node_check_dest(struct net *net, u32 addr,
 		return;
 
 	tipc_node_write_lock(n);
-	if (unlikely(!n->bc_entry.link)) {
-		snd_l = tipc_bc_sndlink(net);
-		if (!tipc_link_bc_create(net, tipc_own_addr(net),
-					 addr, peer_id, U16_MAX,
-					 tipc_link_min_win(snd_l),
-					 tipc_link_max_win(snd_l),
-					 n->capabilities,
-					 &n->bc_entry.inputq1,
-					 &n->bc_entry.namedq, snd_l,
-					 &n->bc_entry.link)) {
-			pr_warn("Broadcast rcv link creation failed, no mem\n");
-			tipc_node_write_unlock_fast(n);
-			tipc_node_put(n);
-			return;
-		}
-	}
 
 	le = &n->links[b->identity];
 
-- 
2.31.1

[tipc-discussion] [net v3] tipc: check attribute length for bearer name

[Hoang Le <hoa...@de...>]

syzbot reported uninit-value:
=====================================================
BUG: KMSAN: uninit-value in string_nocheck lib/vsprintf.c:644 [inline]
BUG: KMSAN: uninit-value in string+0x4f9/0x6f0 lib/vsprintf.c:725
 string_nocheck lib/vsprintf.c:644 [inline]
 string+0x4f9/0x6f0 lib/vsprintf.c:725
 vsnprintf+0x2222/0x3650 lib/vsprintf.c:2806
 vprintk_store+0x537/0x2150 kernel/printk/printk.c:2158
 vprintk_emit+0x28b/0xab0 kernel/printk/printk.c:2256
 vprintk_default+0x86/0xa0 kernel/printk/printk.c:2283
 vprintk+0x15f/0x180 kernel/printk/printk_safe.c:50
 _printk+0x18d/0x1cf kernel/printk/printk.c:2293
 tipc_enable_bearer net/tipc/bearer.c:371 [inline]
 __tipc_nl_bearer_enable+0x2022/0x22a0 net/tipc/bearer.c:1033
 tipc_nl_bearer_enable+0x6c/0xb0 net/tipc/bearer.c:1042
 genl_family_rcv_msg_doit net/netlink/genetlink.c:731 [inline]

- Do sanity check the attribute length for TIPC_NLA_BEARER_NAME.
- Do not use 'illegal name' in printing message.

Reported-by: syz...@sy...
Fixes: cb30a63384bc ("tipc: refactor function tipc_enable_bearer()")
Acked-by: Jon Maloy <jm...@re...>
Signed-off-by: Hoang Le <hoa...@de...>
---
v3: add Fixes tag in commit message.
v2: remove unnecessary sanity check as Jakub's comment.
---
 net/tipc/bearer.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 6d39ca05f249..932c87b98eca 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -259,9 +259,8 @@ static int tipc_enable_bearer(struct net *net, const char *name,
 	u32 i;
 
 	if (!bearer_name_validate(name, &b_names)) {
-		errstr = "illegal name";
 		NL_SET_ERR_MSG(extack, "Illegal name");
-		goto rejected;
+		return res;
 	}
 
 	if (prio > TIPC_MAX_LINK_PRI && prio != TIPC_MEDIA_LINK_PRI) {
-- 
2.30.2

[tipc-discussion] [net v3] tipc: check attribute length for bearer name

[Hoang Le <hoa...@de...>]

syzbot reported uninit-value:
=====================================================
BUG: KMSAN: uninit-value in string_nocheck lib/vsprintf.c:644 [inline]
BUG: KMSAN: uninit-value in string+0x4f9/0x6f0 lib/vsprintf.c:725
 string_nocheck lib/vsprintf.c:644 [inline]
 string+0x4f9/0x6f0 lib/vsprintf.c:725
 vsnprintf+0x2222/0x3650 lib/vsprintf.c:2806
 vprintk_store+0x537/0x2150 kernel/printk/printk.c:2158
 vprintk_emit+0x28b/0xab0 kernel/printk/printk.c:2256
 vprintk_default+0x86/0xa0 kernel/printk/printk.c:2283
 vprintk+0x15f/0x180 kernel/printk/printk_safe.c:50
 _printk+0x18d/0x1cf kernel/printk/printk.c:2293
 tipc_enable_bearer net/tipc/bearer.c:371 [inline]
 __tipc_nl_bearer_enable+0x2022/0x22a0 net/tipc/bearer.c:1033
 tipc_nl_bearer_enable+0x6c/0xb0 net/tipc/bearer.c:1042
 genl_family_rcv_msg_doit net/netlink/genetlink.c:731 [inline]

- Do sanity check the attribute length for TIPC_NLA_BEARER_NAME.
- Do not use 'illegal name' in printing message.

v3: add Fixes tag in commit message.
v2: remove unnecessary sanity check as Jakub's comment.

Reported-by: syz...@sy...
Fixes: cb30a63384bc ("tipc: refactor function tipc_enable_bearer()")
Acked-by: Jon Maloy <jm...@re...>
Signed-off-by: Hoang Le <hoa...@de...>
---
 net/tipc/bearer.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 6d39ca05f249..932c87b98eca 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -259,9 +259,8 @@ static int tipc_enable_bearer(struct net *net, const char *name,
 	u32 i;
 
 	if (!bearer_name_validate(name, &b_names)) {
-		errstr = "illegal name";
 		NL_SET_ERR_MSG(extack, "Illegal name");
-		goto rejected;
+		return res;
 	}
 
 	if (prio > TIPC_MAX_LINK_PRI && prio != TIPC_MEDIA_LINK_PRI) {
-- 
2.30.2

[tipc-discussion] [net v2] tipc: check attribute length for bearer name

[Hoang Le <hoa...@de...>]

syzbot reported uninit-value:
=====================================================
BUG: KMSAN: uninit-value in string_nocheck lib/vsprintf.c:644 [inline]
BUG: KMSAN: uninit-value in string+0x4f9/0x6f0 lib/vsprintf.c:725
 string_nocheck lib/vsprintf.c:644 [inline]
 string+0x4f9/0x6f0 lib/vsprintf.c:725
 vsnprintf+0x2222/0x3650 lib/vsprintf.c:2806
 vprintk_store+0x537/0x2150 kernel/printk/printk.c:2158
 vprintk_emit+0x28b/0xab0 kernel/printk/printk.c:2256
 vprintk_default+0x86/0xa0 kernel/printk/printk.c:2283
 vprintk+0x15f/0x180 kernel/printk/printk_safe.c:50
 _printk+0x18d/0x1cf kernel/printk/printk.c:2293
 tipc_enable_bearer net/tipc/bearer.c:371 [inline]
 __tipc_nl_bearer_enable+0x2022/0x22a0 net/tipc/bearer.c:1033
 tipc_nl_bearer_enable+0x6c/0xb0 net/tipc/bearer.c:1042
 genl_family_rcv_msg_doit net/netlink/genetlink.c:731 [inline]

- Do sanity check the attribute length for TIPC_NLA_BEARER_NAME.
- Do not use 'illegal name' in printing message.

v2: remove unnecessary sanity check as Jakub's comment

Reported-by: syz...@sy...
Acked-by: Jon Maloy <jm...@re...>
Signed-off-by: Hoang Le <hoa...@de...>
---
 net/tipc/bearer.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 6d39ca05f249..932c87b98eca 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -259,9 +259,8 @@ static int tipc_enable_bearer(struct net *net, const char *name,
 	u32 i;
 
 	if (!bearer_name_validate(name, &b_names)) {
-		errstr = "illegal name";
 		NL_SET_ERR_MSG(extack, "Illegal name");
-		goto rejected;
+		return res;
 	}
 
 	if (prio > TIPC_MAX_LINK_PRI && prio != TIPC_MEDIA_LINK_PRI) {
-- 
2.30.2

[tipc-discussion] [net] tipc: check attribute length for bearer name

[Hoang Le <hoa...@de...>]

syzbot reported uninit-value:
=====================================================
BUG: KMSAN: uninit-value in string_nocheck lib/vsprintf.c:644 [inline]
BUG: KMSAN: uninit-value in string+0x4f9/0x6f0 lib/vsprintf.c:725
 string_nocheck lib/vsprintf.c:644 [inline]
 string+0x4f9/0x6f0 lib/vsprintf.c:725
 vsnprintf+0x2222/0x3650 lib/vsprintf.c:2806
 vprintk_store+0x537/0x2150 kernel/printk/printk.c:2158
 vprintk_emit+0x28b/0xab0 kernel/printk/printk.c:2256
 vprintk_default+0x86/0xa0 kernel/printk/printk.c:2283
 vprintk+0x15f/0x180 kernel/printk/printk_safe.c:50
 _printk+0x18d/0x1cf kernel/printk/printk.c:2293
 tipc_enable_bearer net/tipc/bearer.c:371 [inline]
 __tipc_nl_bearer_enable+0x2022/0x22a0 net/tipc/bearer.c:1033
 tipc_nl_bearer_enable+0x6c/0xb0 net/tipc/bearer.c:1042
 genl_family_rcv_msg_doit net/netlink/genetlink.c:731 [inline]

- Do sanity check the attribute length for TIPC_NLA_BEARER_NAME.
- Do not use 'illegal name' in printing message.

Reported-by: syz...@sy...
Acked-by: Jon Maloy <jm...@re...>
Signed-off-by: Hoang Le <hoa...@de...>
---
 net/tipc/bearer.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 6d39ca05f249..0fd7554c7cde 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -258,10 +258,10 @@ static int tipc_enable_bearer(struct net *net, const char *name,
 	char *errstr = "";
 	u32 i;
 
-	if (!bearer_name_validate(name, &b_names)) {
-		errstr = "illegal name";
+	if (strlen(name) > TIPC_MAX_BEARER_NAME ||
+	    !bearer_name_validate(name, &b_names)) {
 		NL_SET_ERR_MSG(extack, "Illegal name");
-		goto rejected;
+		return res;
 	}
 
 	if (prio > TIPC_MAX_LINK_PRI && prio != TIPC_MEDIA_LINK_PRI) {
-- 
2.30.2

Re: [tipc-discussion] [net] tipc: check attribute length for bearer name

[Jon M. <jm...@re...>]

Acked-by: Jon Maloy <jm...@re...>

On 5/26/22 07:02, Hoang Le wrote:
> syzbot reported uninit-value:
> =====================================================
> BUG: KMSAN: uninit-value in string_nocheck lib/vsprintf.c:644 [inline]
> BUG: KMSAN: uninit-value in string+0x4f9/0x6f0 lib/vsprintf.c:725
>   string_nocheck lib/vsprintf.c:644 [inline]
>   string+0x4f9/0x6f0 lib/vsprintf.c:725
>   vsnprintf+0x2222/0x3650 lib/vsprintf.c:2806
>   vprintk_store+0x537/0x2150 kernel/printk/printk.c:2158
>   vprintk_emit+0x28b/0xab0 kernel/printk/printk.c:2256
>   vprintk_default+0x86/0xa0 kernel/printk/printk.c:2283
>   vprintk+0x15f/0x180 kernel/printk/printk_safe.c:50
>   _printk+0x18d/0x1cf kernel/printk/printk.c:2293
>   tipc_enable_bearer net/tipc/bearer.c:371 [inline]
>   __tipc_nl_bearer_enable+0x2022/0x22a0 net/tipc/bearer.c:1033
>   tipc_nl_bearer_enable+0x6c/0xb0 net/tipc/bearer.c:1042
>   genl_family_rcv_msg_doit net/netlink/genetlink.c:731 [inline]
>
> - Do sanity check the attribute length for TIPC_NLA_BEARER_NAME.
> - Do not use 'illegal name' in printing message.
>
> Reported-by: syz...@sy...
> Signed-off-by: Hoang Le <hoa...@de...>
> ---
>   net/tipc/bearer.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
> index 6d39ca05f249..0fd7554c7cde 100644
> --- a/net/tipc/bearer.c
> +++ b/net/tipc/bearer.c
> @@ -258,10 +258,10 @@ static int tipc_enable_bearer(struct net *net, const char *name,
>   	char *errstr = "";
>   	u32 i;
>   
> -	if (!bearer_name_validate(name, &b_names)) {
> -		errstr = "illegal name";
> +	if (strlen(name) > TIPC_MAX_BEARER_NAME ||
> +	    !bearer_name_validate(name, &b_names)) {
>   		NL_SET_ERR_MSG(extack, "Illegal name");
> -		goto rejected;
> +		return res;
>   	}
>   
>   	if (prio > TIPC_MAX_LINK_PRI && prio != TIPC_MEDIA_LINK_PRI) {

[tipc-discussion] [net] tipc: check attribute length for bearer name

[Hoang Le <hoa...@de...>]

syzbot reported uninit-value:
=====================================================
BUG: KMSAN: uninit-value in string_nocheck lib/vsprintf.c:644 [inline]
BUG: KMSAN: uninit-value in string+0x4f9/0x6f0 lib/vsprintf.c:725
 string_nocheck lib/vsprintf.c:644 [inline]
 string+0x4f9/0x6f0 lib/vsprintf.c:725
 vsnprintf+0x2222/0x3650 lib/vsprintf.c:2806
 vprintk_store+0x537/0x2150 kernel/printk/printk.c:2158
 vprintk_emit+0x28b/0xab0 kernel/printk/printk.c:2256
 vprintk_default+0x86/0xa0 kernel/printk/printk.c:2283
 vprintk+0x15f/0x180 kernel/printk/printk_safe.c:50
 _printk+0x18d/0x1cf kernel/printk/printk.c:2293
 tipc_enable_bearer net/tipc/bearer.c:371 [inline]
 __tipc_nl_bearer_enable+0x2022/0x22a0 net/tipc/bearer.c:1033
 tipc_nl_bearer_enable+0x6c/0xb0 net/tipc/bearer.c:1042
 genl_family_rcv_msg_doit net/netlink/genetlink.c:731 [inline]

- Do sanity check the attribute length for TIPC_NLA_BEARER_NAME.
- Do not use 'illegal name' in printing message.

Reported-by: syz...@sy...
Signed-off-by: Hoang Le <hoa...@de...>
---
 net/tipc/bearer.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 6d39ca05f249..0fd7554c7cde 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -258,10 +258,10 @@ static int tipc_enable_bearer(struct net *net, const char *name,
 	char *errstr = "";
 	u32 i;
 
-	if (!bearer_name_validate(name, &b_names)) {
-		errstr = "illegal name";
+	if (strlen(name) > TIPC_MAX_BEARER_NAME ||
+	    !bearer_name_validate(name, &b_names)) {
 		NL_SET_ERR_MSG(extack, "Illegal name");
-		goto rejected;
+		return res;
 	}
 
 	if (prio > TIPC_MAX_LINK_PRI && prio != TIPC_MEDIA_LINK_PRI) {
-- 
2.30.2