> -----Original Message----- > From: Richard Alpe > Sent: Friday, 05 February, 2016 10:29 > To: [email protected] > Cc: Jon Maloy; Ying Xue; [email protected]; Richard Alpe > Subject: [PATCH tipc-discussion v1] tipc: refactor node xmit and fix memory > leaks > > Refactor tipc_node_xmit() to fail fast and fail early. This commit fixes > several > potential memory leaks in unexpected error paths. > > -- tipc discussion -- > Two thinks struck me when looking at this. > > 1. We claim to consume buffer chains in a lot of functions, but the error path > is often neglected.
Admitted. It is certainly due to the fact that I changed the model at a late stage. Before, we never consumed the buffer chain at this level. > > 2. Listing the potential return values in the function header is a bad idea. > There is often a discrepancy between the listed and actual return values. In > the case of tail calls for example, someone has updated the called function to > return a new value, but "forgotten" to backtrack all callers and updating > there headers. Mea culpa again, for same reason as above. I don't think it is a bad idea per se; it is useful if we are just able to make it right. After all, this is not a function that we are going to change every month (and hopefully not for a very long time from now on). The only code I see missing now is -ENOBUF as return code from tipc_node_xmit(). If you fix that return code value (remove "errno" so it fits on one line) you can add a "Reviewed-by" from me. ///jon > > Reported-by: Dmitry Vyukov <[email protected]> > Signed-off-by: Richard Alpe <[email protected]> > --- > net/tipc/link.c | 8 ++++++-- > net/tipc/node.c | 52 +++++++++++++++++++++++++++++++------------------ > --- > 2 files changed, 37 insertions(+), 23 deletions(-) > > diff --git a/net/tipc/link.c b/net/tipc/link.c index 0c2944f..d700be4 100644 > --- a/net/tipc/link.c > +++ b/net/tipc/link.c > @@ -904,8 +904,10 @@ int tipc_link_xmit(struct tipc_link *l, struct > sk_buff_head *list, > if (unlikely(l->backlog[i].len >= l->backlog[i].limit)) > return link_schedule_user(l, list); > } > - if (unlikely(msg_size(hdr) > mtu)) > + if (unlikely(msg_size(hdr) > mtu)) { > + skb_queue_purge(list); > return -EMSGSIZE; > + } > > /* Prepare each packet for sending, and add to relevant queue: */ > while (skb_queue_len(list)) { > @@ -917,8 +919,10 @@ int tipc_link_xmit(struct tipc_link *l, struct > sk_buff_head *list, > > if (likely(skb_queue_len(transmq) < maxwin)) { > _skb = skb_clone(skb, GFP_ATOMIC); > - if (!_skb) > + if (!_skb) { > + skb_queue_purge(list); > return -ENOBUFS; > + } > __skb_dequeue(list); > __skb_queue_tail(transmq, skb); > __skb_queue_tail(xmitq, _skb); > diff --git a/net/tipc/node.c b/net/tipc/node.c index fa97d96..61024ec 100644 > --- a/net/tipc/node.c > +++ b/net/tipc/node.c > @@ -1174,33 +1174,43 @@ int tipc_node_xmit(struct net *net, struct > sk_buff_head *list, > struct tipc_link_entry *le = NULL; > struct tipc_node *n; > struct sk_buff_head xmitq; > - int bearer_id = -1; > - int rc = -EHOSTUNREACH; > + int bearer_id; > + int rc; > + > + if (in_own_node(net, dnode)) { > + tipc_sk_rcv(net, list); > + return 0; > + } > > - __skb_queue_head_init(&xmitq); > n = tipc_node_find(net, dnode); > - if (likely(n)) { > - tipc_node_read_lock(n); > - bearer_id = n->active_links[selector & 1]; > - if (bearer_id >= 0) { > - le = &n->links[bearer_id]; > - spin_lock_bh(&le->lock); > - rc = tipc_link_xmit(le->link, list, &xmitq); > - spin_unlock_bh(&le->lock); > - } > + if (unlikely(!n)) { > + skb_queue_purge(list); > + return -EHOSTUNREACH; > + } > + > + tipc_node_read_lock(n); > + bearer_id = n->active_links[selector & 1]; > + if (unlikely(bearer_id == INVALID_BEARER_ID)) { > tipc_node_read_unlock(n); > - if (likely(!rc)) > - tipc_bearer_xmit(net, bearer_id, &xmitq, &le- > >maddr); > - else if (rc == -ENOBUFS) > - tipc_node_link_down(n, bearer_id, false); > tipc_node_put(n); > - return rc; > + skb_queue_purge(list); > + return -EHOSTUNREACH; > } > > - if (likely(in_own_node(net, dnode))) { > - tipc_sk_rcv(net, list); > - return 0; > - } > + __skb_queue_head_init(&xmitq); > + le = &n->links[bearer_id]; > + spin_lock_bh(&le->lock); > + rc = tipc_link_xmit(le->link, list, &xmitq); > + spin_unlock_bh(&le->lock); > + tipc_node_read_unlock(n); > + > + if (likely(rc == 0)) > + tipc_bearer_xmit(net, bearer_id, &xmitq, &le->maddr); > + else if (rc == -ENOBUFS) > + tipc_node_link_down(n, bearer_id, false); > + > + tipc_node_put(n); > + > return rc; > } > > -- > 2.1.4 ------------------------------------------------------------------------------ Site24x7 APM Insight: Get Deep Visibility into Application Performance APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month Monitor end-to-end web transactions and take corrective actions now Troubleshoot faster and improve end-user experience. Signup Now! http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140 _______________________________________________ tipc-discussion mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/tipc-discussion
