| Message ID | 20220303045717.30232-1-tung.q.nguyen@dektech.com.au (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,1/1] tipc: fix kernel panic when enabling bearer | expand |
On Thu, 3 Mar 2022 04:57:17 +0000 Tung Nguyen wrote: > diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c > index 473a790f5894..63460183440d 100644 > --- a/net/tipc/bearer.c > +++ b/net/tipc/bearer.c > @@ -252,7 +252,7 @@ static int tipc_enable_bearer(struct net *net, const char *name, > int with_this_prio = 1; > struct tipc_bearer *b; > struct tipc_media *m; > - struct sk_buff *skb; > + struct sk_buff *skb = NULL; > int bearer_id = 0; > int res = -EINVAL; > char *errstr = ""; This chunk looks unrelated and unnecessary. The had previously trusted skb to be initialized by tipc_disc_create().
On Thu, 3 Mar 2022 04:57:17 +0000 Tung Nguyen wrote: > diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c > index 473a790f5894..63460183440d 100644 > --- a/net/tipc/bearer.c > +++ b/net/tipc/bearer.c > @@ -252,7 +252,7 @@ static int tipc_enable_bearer(struct net *net, const char *name, > int with_this_prio = 1; > struct tipc_bearer *b; > struct tipc_media *m; > - struct sk_buff *skb; > + struct sk_buff *skb = NULL; > int bearer_id = 0; > int res = -EINVAL; > char *errstr = ""; This chunk looks unrelated and unnecessary. The had previously trusted skb to be initialized by tipc_disc_create(). [Tung]: OK. I will remove it in v2.
diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c index 473a790f5894..63460183440d 100644 --- a/net/tipc/bearer.c +++ b/net/tipc/bearer.c @@ -252,7 +252,7 @@ static int tipc_enable_bearer(struct net *net, const char *name, int with_this_prio = 1; struct tipc_bearer *b; struct tipc_media *m; - struct sk_buff *skb; + struct sk_buff *skb = NULL; int bearer_id = 0; int res = -EINVAL; char *errstr = ""; @@ -352,16 +352,18 @@ static int tipc_enable_bearer(struct net *net, const char *name, goto rejected; } - test_and_set_bit_lock(0, &b->up); - rcu_assign_pointer(tn->bearer_list[bearer_id], b); - if (skb) - tipc_bearer_xmit_skb(net, bearer_id, skb, &b->bcast_addr); - + /* Create monitoring data before accepting activate messages */ if (tipc_mon_create(net, bearer_id)) { bearer_disable(net, b); + kfree_skb(skb); return -ENOMEM; } + test_and_set_bit_lock(0, &b->up); + rcu_assign_pointer(tn->bearer_list[bearer_id], b); + if (skb) + tipc_bearer_xmit_skb(net, bearer_id, skb, &b->bcast_addr); + pr_info("Enabled bearer <%s>, priority %u\n", name, prio); return res;
When enabling a bearer on a node, a kernel panic is observed: [ 4.498085] RIP: 0010:tipc_mon_prep+0x4e/0x130 [tipc] ... [ 4.520030] Call Trace: [ 4.520689] <IRQ> [ 4.521236] tipc_link_build_proto_msg+0x375/0x750 [tipc] [ 4.522654] tipc_link_build_state_msg+0x48/0xc0 [tipc] [ 4.524034] __tipc_node_link_up+0xd7/0x290 [tipc] [ 4.525292] tipc_rcv+0x5da/0x730 [tipc] [ 4.526346] ? __netif_receive_skb_core+0xb7/0xfc0 [ 4.527601] tipc_l2_rcv_msg+0x5e/0x90 [tipc] [ 4.528737] __netif_receive_skb_list_core+0x20b/0x260 [ 4.530068] netif_receive_skb_list_internal+0x1bf/0x2e0 [ 4.531450] ? dev_gro_receive+0x4c2/0x680 [ 4.532512] napi_complete_done+0x6f/0x180 [ 4.533570] virtnet_poll+0x29c/0x42e [virtio_net] ... The node in question is receiving activate messages in another thread after changing bearer status to allow message sending/ receiving in current thread: thread 1 | thread 2 -------- | -------- | tipc_enable_bearer() | test_and_set_bit_lock() | tipc_bearer_xmit_skb() | | tipc_l2_rcv_msg() | tipc_rcv() | __tipc_node_link_up() | tipc_link_build_state_msg() | tipc_link_build_proto_msg() | tipc_mon_prep() | { | ... | // null-pointer dereference | u16 gen = mon->dom_gen; | ... | } // Not being executed yet | tipc_mon_create() | { | ... | // allocate | mon = kzalloc(); | ... | } | Monitoring pointer in thread 2 is dereferenced before monitoring data is allocated in thread 1. This causes kernel panic. This commit fixes it by allocating the monitoring data before enabling the bearer to receive messages. Fixes: 35c55c9877f8 ("tipc: add neighbor monitoring framework") Reported-by: Shuang Li <shuali@redhat.com> Acked-by: Jon Maloy <jmaloy@redhat.com> Signed-off-by: Tung Nguyen <tung.q.nguyen@dektech.com.au> --- net/tipc/bearer.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)