From: Xin Long <lucien.xin@gmail.com> To: network dev <netdev@vger.kernel.org>, linux-sctp@vger.kernel.org Cc: davem@davemloft.net, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>, Vlad Yasevich <vyasevich@gmail.com>, daniel@iogearbox.net Subject: [PATCHv2 net 6/6] sctp: not return ENOMEM err back in sctp_packet_transmit Date: Wed, 14 Sep 2016 02:04:23 +0800 [thread overview] Message-ID: <099da3a4fed2e30a29326e9ffb2517858d29e288.1473789537.git.lucien.xin@gmail.com> (raw) In-Reply-To: <6599cec72f3ff913d3b99e60d5f2014df26915de.1473789537.git.lucien.xin@gmail.com> In-Reply-To: <cover.1473789537.git.lucien.xin@gmail.com> As David and Marcelo's suggestion, ENOMEM err shouldn't return back to user in transmit path. Instead, sctp's retransmit would take care of the chunks that fail to send because of ENOMEM. This patch is only to do some release job when alloc_skb fails, not to return ENOMEM back any more. Besides, it also cleans up sctp_packet_transmit's err path, and fixes some issues in err path: - It didn't free the head skb in nomem: path. - No need to check nskb in no_route: path. - It should goto err: path if alloc_skb fails for head. - Not all the NOMEMs should free nskb. Signed-off-by: Xin Long <lucien.xin@gmail.com> --- net/sctp/output.c | 47 ++++++++++++++++++++++------------------------- 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/net/sctp/output.c b/net/sctp/output.c index f2597a9..0c605ec 100644 --- a/net/sctp/output.c +++ b/net/sctp/output.c @@ -442,14 +442,14 @@ int sctp_packet_transmit(struct sctp_packet *packet, gfp_t gfp) * time. Application may notice this error. */ pr_err_once("Trying to GSO but underlying device doesn't support it."); - goto nomem; + goto err; } } else { pkt_size = packet->size; } head = alloc_skb(pkt_size + MAX_HEADER, gfp); if (!head) - goto nomem; + goto err; if (gso) { NAPI_GRO_CB(head)->last = head; skb_shinfo(head)->gso_type = sk->sk_gso_type; @@ -470,8 +470,12 @@ int sctp_packet_transmit(struct sctp_packet *packet, gfp_t gfp) } } dst = dst_clone(tp->dst); - if (!dst) - goto no_route; + if (!dst) { + if (asoc) + IP_INC_STATS(sock_net(asoc->base.sk), + IPSTATS_MIB_OUTNOROUTES); + goto nodst; + } skb_dst_set(head, dst); /* Build the SCTP header. */ @@ -622,8 +626,10 @@ int sctp_packet_transmit(struct sctp_packet *packet, gfp_t gfp) if (!gso) break; - if (skb_gro_receive(&head, nskb)) + if (skb_gro_receive(&head, nskb)) { + kfree_skb(nskb); goto nomem; + } nskb = NULL; if (WARN_ON_ONCE(skb_shinfo(head)->gso_segs >= sk->sk_gso_max_segs)) @@ -717,18 +723,13 @@ int sctp_packet_transmit(struct sctp_packet *packet, gfp_t gfp) } head->ignore_df = packet->ipfragok; tp->af_specific->sctp_xmit(head, tp); + goto out; -out: - sctp_packet_reset(packet); - return err; -no_route: - kfree_skb(head); - if (nskb != head) - kfree_skb(nskb); - - if (asoc) - IP_INC_STATS(sock_net(asoc->base.sk), IPSTATS_MIB_OUTNOROUTES); +nomem: + if (packet->auth && list_empty(&packet->auth->list)) + sctp_chunk_free(packet->auth); +nodst: /* FIXME: Returning the 'err' will effect all the associations * associated with a socket, although only one of the paths of the * association is unreachable. @@ -737,22 +738,18 @@ no_route: * required. */ /* err = -EHOSTUNREACH; */ -err: - /* Control chunks are unreliable so just drop them. DATA chunks - * will get resent or dropped later. - */ + kfree_skb(head); +err: list_for_each_entry_safe(chunk, tmp, &packet->chunk_list, list) { list_del_init(&chunk->list); if (!sctp_chunk_is_data(chunk)) sctp_chunk_free(chunk); } - goto out; -nomem: - if (packet->auth && list_empty(&packet->auth->list)) - sctp_chunk_free(packet->auth); - err = -ENOMEM; - goto err; + +out: + sctp_packet_reset(packet); + return err; } /******************************************************************** -- 2.1.0
WARNING: multiple messages have this Message-ID (diff)
From: Xin Long <lucien.xin@gmail.com> To: network dev <netdev@vger.kernel.org>, linux-sctp@vger.kernel.org Cc: davem@davemloft.net, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>, Vlad Yasevich <vyasevich@gmail.com>, daniel@iogearbox.net Subject: [PATCHv2 net 6/6] sctp: not return ENOMEM err back in sctp_packet_transmit Date: Tue, 13 Sep 2016 18:04:23 +0000 [thread overview] Message-ID: <099da3a4fed2e30a29326e9ffb2517858d29e288.1473789537.git.lucien.xin@gmail.com> (raw) In-Reply-To: <6599cec72f3ff913d3b99e60d5f2014df26915de.1473789537.git.lucien.xin@gmail.com> As David and Marcelo's suggestion, ENOMEM err shouldn't return back to user in transmit path. Instead, sctp's retransmit would take care of the chunks that fail to send because of ENOMEM. This patch is only to do some release job when alloc_skb fails, not to return ENOMEM back any more. Besides, it also cleans up sctp_packet_transmit's err path, and fixes some issues in err path: - It didn't free the head skb in nomem: path. - No need to check nskb in no_route: path. - It should goto err: path if alloc_skb fails for head. - Not all the NOMEMs should free nskb. Signed-off-by: Xin Long <lucien.xin@gmail.com> --- net/sctp/output.c | 47 ++++++++++++++++++++++------------------------- 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/net/sctp/output.c b/net/sctp/output.c index f2597a9..0c605ec 100644 --- a/net/sctp/output.c +++ b/net/sctp/output.c @@ -442,14 +442,14 @@ int sctp_packet_transmit(struct sctp_packet *packet, gfp_t gfp) * time. Application may notice this error. */ pr_err_once("Trying to GSO but underlying device doesn't support it."); - goto nomem; + goto err; } } else { pkt_size = packet->size; } head = alloc_skb(pkt_size + MAX_HEADER, gfp); if (!head) - goto nomem; + goto err; if (gso) { NAPI_GRO_CB(head)->last = head; skb_shinfo(head)->gso_type = sk->sk_gso_type; @@ -470,8 +470,12 @@ int sctp_packet_transmit(struct sctp_packet *packet, gfp_t gfp) } } dst = dst_clone(tp->dst); - if (!dst) - goto no_route; + if (!dst) { + if (asoc) + IP_INC_STATS(sock_net(asoc->base.sk), + IPSTATS_MIB_OUTNOROUTES); + goto nodst; + } skb_dst_set(head, dst); /* Build the SCTP header. */ @@ -622,8 +626,10 @@ int sctp_packet_transmit(struct sctp_packet *packet, gfp_t gfp) if (!gso) break; - if (skb_gro_receive(&head, nskb)) + if (skb_gro_receive(&head, nskb)) { + kfree_skb(nskb); goto nomem; + } nskb = NULL; if (WARN_ON_ONCE(skb_shinfo(head)->gso_segs > sk->sk_gso_max_segs)) @@ -717,18 +723,13 @@ int sctp_packet_transmit(struct sctp_packet *packet, gfp_t gfp) } head->ignore_df = packet->ipfragok; tp->af_specific->sctp_xmit(head, tp); + goto out; -out: - sctp_packet_reset(packet); - return err; -no_route: - kfree_skb(head); - if (nskb != head) - kfree_skb(nskb); - - if (asoc) - IP_INC_STATS(sock_net(asoc->base.sk), IPSTATS_MIB_OUTNOROUTES); +nomem: + if (packet->auth && list_empty(&packet->auth->list)) + sctp_chunk_free(packet->auth); +nodst: /* FIXME: Returning the 'err' will effect all the associations * associated with a socket, although only one of the paths of the * association is unreachable. @@ -737,22 +738,18 @@ no_route: * required. */ /* err = -EHOSTUNREACH; */ -err: - /* Control chunks are unreliable so just drop them. DATA chunks - * will get resent or dropped later. - */ + kfree_skb(head); +err: list_for_each_entry_safe(chunk, tmp, &packet->chunk_list, list) { list_del_init(&chunk->list); if (!sctp_chunk_is_data(chunk)) sctp_chunk_free(chunk); } - goto out; -nomem: - if (packet->auth && list_empty(&packet->auth->list)) - sctp_chunk_free(packet->auth); - err = -ENOMEM; - goto err; + +out: + sctp_packet_reset(packet); + return err; } /******************************************************************** -- 2.1.0
next prev parent reply other threads:[~2016-09-13 18:04 UTC|newest] Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-09-13 18:04 [PATCHv2 net 0/6] sctp: fix the transmit err process Xin Long 2016-09-13 18:04 ` Xin Long 2016-09-13 18:04 ` [PATCHv2 net 1/6] sctp: remove the unnecessary state check in sctp_outq_tail Xin Long 2016-09-13 18:04 ` Xin Long 2016-09-13 18:04 ` [PATCHv2 net 2/6] sctp: do not return the transmit err back to sctp_sendmsg Xin Long 2016-09-13 18:04 ` Xin Long 2016-09-13 18:04 ` [PATCHv2 net 3/6] sctp: free msg->chunks when sctp_primitive_SEND return err Xin Long 2016-09-13 18:04 ` Xin Long 2016-09-13 18:04 ` [PATCHv2 net 4/6] sctp: save transmit error to sk_err in sctp_outq_flush Xin Long 2016-09-13 18:04 ` Xin Long 2016-09-13 18:04 ` [PATCHv2 net 5/6] sctp: make sctp_outq_flush/tail/uncork return void Xin Long 2016-09-13 18:04 ` Xin Long 2016-09-13 18:04 ` Xin Long [this message] 2016-09-13 18:04 ` [PATCHv2 net 6/6] sctp: not return ENOMEM err back in sctp_packet_transmit Xin Long 2016-09-14 17:16 ` [PATCHv2 net 0/6] sctp: fix the transmit err process Marcelo Ricardo Leitner 2016-09-14 17:16 ` Marcelo Ricardo Leitner 2016-09-19 2:03 ` David Miller 2016-09-19 2:03 ` David Miller
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=099da3a4fed2e30a29326e9ffb2517858d29e288.1473789537.git.lucien.xin@gmail.com \ --to=lucien.xin@gmail.com \ --cc=daniel@iogearbox.net \ --cc=davem@davemloft.net \ --cc=linux-sctp@vger.kernel.org \ --cc=marcelo.leitner@gmail.com \ --cc=netdev@vger.kernel.org \ --cc=vyasevich@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.