Hi Dan, Yes, you are right. I don't see any simple fix to this right now, since we at other locations where we call tipc_link_data_input() are expecting exactly this behavior. I'll try to find a consistent solution to this.
BR ///jon > -----Original Message----- > From: Dan Carpenter [mailto:[email protected]] > Sent: Wednesday, April 19, 2017 08:41 AM > To: Jon Maloy <[email protected]> > Cc: [email protected] > Subject: [bug report] tipc: resolve race problem at unicast message reception > > Hello Jon Paul Maloy, > > The patch c637c1035534: "tipc: resolve race problem at unicast message > reception" from Feb 5, 2015, leads to the following static checker warning: > > net/tipc/link.c:1127 tipc_link_input() > error: double free of 'skb' > > net/tipc/link.c > 1073 static int tipc_link_input(struct tipc_link *l, struct sk_buff *skb, > 1074 struct sk_buff_head *inputq) > 1075 { > 1076 struct tipc_msg *hdr = buf_msg(skb); > 1077 struct sk_buff **reasm_skb = &l->reasm_buf; > 1078 struct sk_buff *iskb; > 1079 struct sk_buff_head tmpq; > 1080 int usr = msg_user(hdr); > 1081 int rc = 0; > 1082 int pos = 0; > 1083 int ipos = 0; > 1084 > 1085 if (unlikely(usr == TUNNEL_PROTOCOL)) { > 1086 if (msg_type(hdr) == SYNCH_MSG) { > 1087 __skb_queue_purge(&l->deferdq); > 1088 goto drop; > 1089 } > 1090 if (!tipc_msg_extract(skb, &iskb, &ipos)) > 1091 return rc; > 1092 kfree_skb(skb); > 1093 skb = iskb; > 1094 hdr = buf_msg(skb); > 1095 if (less(msg_seqno(hdr), l->drop_point)) > 1096 goto drop; > 1097 if (tipc_data_input(l, skb, inputq)) > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ If it's an illegal > message > type then we free skb. > > 1098 return rc; > 1099 usr = msg_user(hdr); > 1100 reasm_skb = &l->failover_reasm_skb; > 1101 } > 1102 > 1103 if (usr == MSG_BUNDLER) { > 1104 skb_queue_head_init(&tmpq); > 1105 l->stats.recv_bundles++; > 1106 l->stats.recv_bundled += msg_msgcnt(hdr); > 1107 while (tipc_msg_extract(skb, &iskb, &pos)) > 1108 tipc_data_input(l, iskb, &tmpq); > 1109 tipc_skb_queue_splice_tail(&tmpq, inputq); > 1110 return 0; > 1111 } else if (usr == MSG_FRAGMENTER) { > 1112 l->stats.recv_fragments++; > 1113 if (tipc_buf_append(reasm_skb, &skb)) { > 1114 l->stats.recv_fragmented++; > 1115 tipc_data_input(l, skb, inputq); > 1116 } else if (!*reasm_skb && !link_is_bc_rcvlink(l)) { > 1117 pr_warn_ratelimited("Unable to build fragment > list\n"); > 1118 return tipc_link_fsm_evt(l, LINK_FAILURE_EVT); > 1119 } > 1120 return 0; > 1121 } else if (usr == BCAST_PROTOCOL) { > 1122 tipc_bcast_lock(l->net); > 1123 tipc_link_bc_init_rcv(l->bc_rcvlink, hdr); > 1124 tipc_bcast_unlock(l->net); > 1125 } > 1126 drop: > 1127 kfree_skb(skb); > ^^^^^^^^^^^^^^ > Leading to a double free here. > > 1128 return 0; > 1129 } > > regards, > dan carpenter ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ tipc-discussion mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/tipc-discussion
