sctp: allow GSO frags to access the chunk too
Subsystems (3)
Marcelo Ricardo Leitner
maintainercover sctp: allow GSO frags to access the chunk too
Patchset is named after the most important fix in it. First two patches are preparing the grounds for the 3rd patch.
After the 3rd, they are not strictly logically related to the patchset, but I kept them together as they depend on each other.
More details on patch changelogs.
Thanks!
Marcelo Ricardo Leitner (6):
sctp: allow others to use sctp_input_cb
sctp: reorder sctp_ulpevent and shrink msg_flags
sctp: allow GSO frags to access the chunk too
sctp: avoid identifying address family many times for a chunk
sctp: do not clear chunk->ecn_ce_done flag
sctp: only check for ECN if peer is using it
include/net/sctp/structs.h | 23 +++++++++++++++++++++++ include/net/sctp/ulpevent.h | 12 ++++++------ net/sctp/input.c | 12 +----------- net/sctp/inqueue.c | 9 ++++++++- net/sctp/ipv6.c | 9 ++++----- net/sctp/protocol.c | 1 + net/sctp/sm_make_chunk.c | 20 ++++---------------- net/sctp/sm_statefuns.c | 9 +++------ net/sctp/socket.c | 10 +++++++--- net/sctp/ulpevent.c | 14 +++++++++++--- 10 files changed, 68 insertions(+), 51 deletions(-)
Signature · 1 line
2.7.4
Marcelo Ricardo Leitner
maintainerPATCH 1/6 sctp: allow others to use sctp_input_cb
We process input path in other files too and having access to it is nice, so move it to a header where it's shared.
--- include/net/sctp/structs.h | 15 +++++++++++++++ net/sctp/input.c | 11 ----------- 2 files changed, 15 insertions(+), 11 deletions(-)
include/net/sctp/structs.h +15 -0
@@ -59,6 +59,7 @@#include <linux/workqueue.h> /* We need tq_struct. */ #include <linux/sctp.h> /* We need sctp* header structs. */ #include <net/sctp/auth.h> /* We need auth specific structs */ +#include <net/ip.h> /* For inet_skb_parm */ /* A convenience structure for handling sockaddr structures. * We should wean ourselves off this.
@@ -1092,6 +1093,20 @@ static inline void sctp_outq_cork(struct sctp_outq *q) q->cork =1 ; } +/* SCTP skb control block. +* sctp_input_cb is currently used on rx and sock rx queue +*/ +struct sctp_input_cb { +union { +struct inet_skb_parm h4; +#if IS_ENABLED (CONFIG_IPV6 ) +struct inet6_skb_parm h6; +#endif + } header; +struct sctp_chunk *chunk; +}; +#define SCTP_INPUT_CB (__skb) ((struct sctp_input_cb *)&((__skb)->cb[0])) +/* These bind address data fields common between endpoints and associations */ struct sctp_bind_addr {
net/sctp/input.c +0 -11
@@ -90,17 +90,6 @@ static inline int sctp_rcv_checksum(struct net *net, struct sk_buff *skb)return 0 ; } -struct sctp_input_cb { -union { -struct inet_skb_parm h4; -#if IS_ENABLED (CONFIG_IPV6 ) -struct inet6_skb_parm h6; -#endif - } header; -struct sctp_chunk *chunk; -}; -#define SCTP_INPUT_CB (__skb) ((struct sctp_input_cb *)&((__skb)->cb[0])) -/* * This is the routine which IP calls when receiving an SCTP packet. */
Signature · 1 line
2.7.4
Marcelo Ricardo Leitner
maintainerPATCH 2/6 sctp: reorder sctp_ulpevent and shrink msg_flags
In reply to Marcelo Ricardo LeitnerThe next patch needs 8 bytes in there. sctp_ulpevent has a hole due to bad alignment; msg_flags is using 4 bytes while it actually uses only 2, so we shrink it, and iif member (4 bytes) which can be easily fetched from another place once the next patch is there, so we remove it and thus creating space for 8 bytes.
--- include/net/sctp/ulpevent.h | 10 +++++----- net/sctp/ulpevent.c | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-)
include/net/sctp/ulpevent.h +5 -5
@@ -48,15 +48,15 @@*/ struct sctp_ulpevent {struct sctp_association *asoc; -__u16 stream; -__u16 ssn; -__u16 flags; +unsigned int rmem_len;__u32 ppid;__u32 tsn;__u32 cumtsn; -int msg_flags;int iif; -unsigned int rmem_len; +__u16 stream; +__u16 ssn; +__u16 flags; +__u16 msg_flags; };/* Retrieve the skb this event sits inside of. */
net/sctp/ulpevent.c +2 -2
@@ -51,7 +51,7 @@ static void sctp_ulpevent_release_frag_data(struct sctp_ulpevent *event);/* Initialize an ULP event from an given skb. */ static void sctp_ulpevent_init (struct sctp_ulpevent *event, -int msg_flags, +__u16 msg_flags,unsigned int len) {memset (event,0 ,sizeof (struct sctp_ulpevent ));
@@ -60,7 +60,7 @@ static void sctp_ulpevent_init(struct sctp_ulpevent *event, }/* Create a new sctp_ulpevent. */ -static struct sctp_ulpevent *sctp_ulpevent_new (int size,int msg_flags, +static struct sctp_ulpevent *sctp_ulpevent_new (int size,__u16 msg_flags,gfp_t gfp) {struct sctp_ulpevent *event;
Signature · 1 line
2.7.4
Marcelo Ricardo Leitner
maintainerPATCH 3/6 sctp: allow GSO frags to access the chunk too
In reply to Marcelo Ricardo LeitnerSCTP will try to access original IP headers on sctp_recvmsg in order to copy the addresses used. There are also other places that do similar access to IP or even SCTP headers. But after 90017accff61 ("sctp: Add GSO support") they aren't always there because they are only present in the header skb.
SCTP handles the queueing of incoming data by cloning the incoming skb and limiting to only the relevant payload. This clone has its cb updated to something different and it's then queued on socket rx queue. Thus we need to fix this in two moments.
For rx path, not related to socket queue yet, this patch uses a partially copied sctp_input_cb to such GSO frags. This restores the ability to access the headers for this part of the code.
Regarding the socket rx queue, it removes iif member from sctp_event and also add a chunk pointer on it.
With these changes we're always able to reach the headers again.
The biggest change here is that now the sctp_chunk struct and the original skb are only freed after the application consumed the buffer. Note however that the original payload was already like this due to the skb cloning.
For iif, SCTP's IPv4 code doesn't use it, so no change is necessary. IPv6 now can fetch it directly from original's IPv6 CB as the original skb is still accessible.
In the future we probably can simplify sctp_v*_skb_iif() stuff, as sctp_v4_skb_iif() was called but it's return value not used, and now it's not even called, but such cleanup is out of scope for this change.
--- include/net/sctp/structs.h | 7 +++++++ include/net/sctp/ulpevent.h | 2 +- net/sctp/inqueue.c | 7 +++++++ net/sctp/ipv6.c | 9 ++++----- net/sctp/protocol.c | 1 + net/sctp/sm_statefuns.c | 3 ++- net/sctp/socket.c | 10 +++++++--- net/sctp/ulpevent.c | 10 +++++++++- 8 files changed, 38 insertions(+), 11 deletions(-)
include/net/sctp/structs.h +7 -0
@@ -1107,6 +1107,13 @@ struct sctp_input_cb { };#define SCTP_INPUT_CB (__skb) ((struct sctp_input_cb *)&((__skb)->cb[0])) +static inline const struct sk_buff *sctp_gso_headskb (const struct sk_buff *skb) +{ +const struct sctp_chunk *chunk =SCTP_INPUT_CB (skb)->chunk; + +return chunk->head_skb ? : skb; +} +/* These bind address data fields common between endpoints and associations */ struct sctp_bind_addr {
include/net/sctp/ulpevent.h +1 -1
@@ -48,11 +48,11 @@*/ struct sctp_ulpevent {struct sctp_association *asoc; +struct sctp_chunk *chunk;unsigned int rmem_len;__u32 ppid;__u32 tsn;__u32 cumtsn; -int iif;__u16 stream;__u16 ssn;__u16 flags;
net/sctp/inqueue.c +7 -0
@@ -218,6 +218,13 @@ new_skb: chunk->has_asconf =0 ; chunk->end_of_packet =0 ; chunk->ecn_ce_done =0 ; +if (chunk->head_skb) { +struct sctp_input_cb + *cb =SCTP_INPUT_CB (chunk->skb), + *head_cb =SCTP_INPUT_CB (chunk->head_skb); + + cb->chunk = head_cb->chunk; + } } chunk->chunk_hdr = ch;
net/sctp/ipv6.c +4 -5
@@ -420,6 +420,7 @@ static void sctp_v6_from_skb(union sctp_addr *addr, struct sk_buff *skb, addr->v6.sin6_flowinfo =0 ;/* FIXME */ addr->v6.sin6_scope_id = ((struct inet6_skb_parm *)skb->cb)->iif; +/* Always called on head skb, so this is safe */ sh =sctp_hdr (skb);if (is_saddr) { *port = sh->source;
@@ -710,8 +711,7 @@ static int sctp_v6_addr_to_user(struct sctp_sock *sp, union sctp_addr *addr)/* Where did this skb come from? */ static int sctp_v6_skb_iif (const struct sk_buff *skb) { -struct inet6_skb_parm *opt = (struct inet6_skb_parm *) skb->cb; -return opt->iif; +return IP6CB (skb)->iif; }/* Was this packet marked by Explicit Congestion Notification? */
@@ -780,15 +780,14 @@ static void sctp_inet6_skb_msgname(struct sk_buff *skb, char *msgname,if (ip_hdr (skb)->version =4 ) { addr->v4.sin_family =AF_INET ; addr->v4.sin_port = sh->source; - addr->v4.sin_addr.s_addr =ip_hdr (skb)->saddr; + addr->v4.sin_addr.s_addr =ip_hdr (skb)->saddr; }else { addr->v6.sin6_family =AF_INET6 ; addr->v6.sin6_flowinfo =0 ; addr->v6.sin6_port = sh->source; addr->v6.sin6_addr =ipv6_hdr (skb)->saddr;if (ipv6_addr_type (&addr->v6.sin6_addr) &IPV6_ADDR_LINKLOCAL ) { -struct sctp_ulpevent *ev =sctp_skb2event (skb); - addr->v6.sin6_scope_id = ev->iif; + addr->v6.sin6_scope_id =sctp_v6_skb_iif (skb); } }
net/sctp/protocol.c +1 -0
@@ -240,6 +240,7 @@ static void sctp_v4_from_skb(union sctp_addr *addr, struct sk_buff *skb, port = &addr->v4.sin_port; addr->v4.sin_family =AF_INET ; +/* Always called on head skb, so this is safe */ sh =sctp_hdr (skb);if (is_saddr) { *port = sh->source;
net/sctp/sm_statefuns.c +2 -1
@@ -6125,7 +6125,8 @@ static int sctp_eat_data(const struct sctp_association *asoc, af =sctp_get_af_specific (ipver2af (ip_hdr (chunk->skb)->version)); -if (af && af->is_ce (chunk->skb) && asoc->peer.ecn_capable) { +if (af && af->is_ce (sctp_gso_headskb (chunk->skb)) && + asoc->peer.ecn_capable) {/* Do real work as sideffect. */ sctp_add_cmd_sf (commands,SCTP_CMD_ECN_CE ,SCTP_U32 (tsn));
net/sctp/socket.c +7 -3
@@ -2066,7 +2066,7 @@ static int sctp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, {struct sctp_ulpevent *event =NULL ;struct sctp_sock *sp =sctp_sk (sk); -struct sk_buff *skb; +struct sk_buff *skb, *head_skb;int copied;int err =0 ;int skb_len;
@@ -2102,12 +2102,16 @@ static int sctp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,if (err) goto out_free; -sock_recv_ts_and_drops (msg, sk, skb); +if (event->chunk && event->chunk->head_skb) + head_skb = event->chunk->head_skb; +else + head_skb = skb; +sock_recv_ts_and_drops (msg, sk, head_skb);if (sctp_ulpevent_is_notification (event)) { msg->msg_flags |=MSG_NOTIFICATION ; sp->pf->event_msgname (event, msg->msg_name, addr_len); }else { - sp->pf->skb_msgname (skb, msg->msg_name, addr_len); + sp->pf->skb_msgname (head_skb, msg->msg_name, addr_len); }/* Check if we allow SCTP_NXTINFO. */
net/sctp/ulpevent.c +9 -1
@@ -701,6 +701,12 @@ struct sctp_ulpevent *sctp_ulpevent_make_rcvmsg(struct sctp_association *asoc,sctp_ulpevent_receive_data (event, asoc); +/* And hold the chunk as we need it for getting the IP headers +* later in recvmsg +*/ +sctp_chunk_hold (chunk); + event->chunk = chunk; + event->stream =ntohs (chunk->subh.data_hdr->stream); event->ssn =ntohs (chunk->subh.data_hdr->ssn); event->ppid = chunk->subh.data_hdr->ppid;
@@ -710,11 +716,11 @@ struct sctp_ulpevent *sctp_ulpevent_make_rcvmsg(struct sctp_association *asoc, } event->tsn =ntohl (chunk->subh.data_hdr->tsn); event->msg_flags |= chunk->chunk_hdr->flags; - event->iif =sctp_chunk_iif (chunk);return event; fail_mark: +sctp_chunk_put (chunk);kfree_skb (skb); fail:return NULL ;
@@ -1007,6 +1013,7 @@ static void sctp_ulpevent_release_data(struct sctp_ulpevent *event) done:sctp_assoc_rwnd_increase (event->asoc, len); +sctp_chunk_put (event->chunk);sctp_ulpevent_release_owner (event); }
@@ -1029,6 +1036,7 @@ static void sctp_ulpevent_release_frag_data(struct sctp_ulpevent *event) } done: +sctp_chunk_put (event->chunk);sctp_ulpevent_release_owner (event); }
Signature · 1 line
2.7.4
Marcelo Ricardo Leitner
maintainerPATCH 4/6 sctp: avoid identifying address family many times for a chunk
In reply to Marcelo Ricardo LeitnerIdentifying address family operations during rx path is not something expensive but it's ugly to the eye to have it done multiple times, specially when we already validated it during initial rx processing.
This patch takes advantage of the now shared sctp_input_cb and make the pointer to the operations readily available.
--- include/net/sctp/structs.h | 1 + net/sctp/input.c | 1 + net/sctp/inqueue.c | 1 + net/sctp/sm_make_chunk.c | 20 ++++---------------- net/sctp/sm_statefuns.c | 7 ++----- 5 files changed, 9 insertions(+), 21 deletions(-)
include/net/sctp/structs.h +1 -0
@@ -1104,6 +1104,7 @@ struct sctp_input_cb {#endif } header;struct sctp_chunk *chunk; +struct sctp_af *af; };#define SCTP_INPUT_CB (__skb) ((struct sctp_input_cb *)&((__skb)->cb[0]))
net/sctp/input.c +1 -0
@@ -140,6 +140,7 @@ int sctp_rcv(struct sk_buff *skb) af =sctp_get_af_specific (family);if (unlikely (!af)) goto discard_it; +SCTP_INPUT_CB (skb)->af = af;/* Initialize local addresses for lookups. */ af->from_skb (&src, skb,1 );
net/sctp/inqueue.c +1 -0
@@ -224,6 +224,7 @@ new_skb: *head_cb =SCTP_INPUT_CB (chunk->head_skb); cb->chunk = head_cb->chunk; + cb->af = head_cb->af; } }
net/sctp/sm_make_chunk.c +4 -16
@@ -108,14 +108,9 @@ static void sctp_control_set_owner_w(struct sctp_chunk *chunk)/* What was the inbound interface for this chunk? */ int sctp_chunk_iif (const struct sctp_chunk *chunk) { -struct sctp_af *af; -int iif =0 ; - - af =sctp_get_af_specific (ipver2af (ip_hdr (chunk->skb)->version)); -if (af) - iif = af->skb_iif (chunk->skb); +struct sk_buff *skb = chunk->skb; -return iif; +return SCTP_INPUT_CB (skb)->af->skb_iif (skb); }/* RFC 2960 3.3.2 Initiation (INIT) (1)
@@ -1600,7 +1595,6 @@ struct sctp_association *sctp_make_temp_asoc(const struct sctp_endpoint *ep,struct sctp_association *asoc;struct sk_buff *skb;sctp_scope_t scope; -struct sctp_af *af;/* Create the bare association. */ scope =sctp_scope (sctp_source (chunk));
@@ -1610,16 +1604,10 @@ struct sctp_association *sctp_make_temp_asoc(const struct sctp_endpoint *ep, asoc->temp =1 ; skb = chunk->skb;/* Create an entry for the source address of the packet. */ - af =sctp_get_af_specific (ipver2af (ip_hdr (skb)->version)); -if (unlikely (!af)) - goto fail; - af->from_skb (&asoc->c.peer_addr, skb,1 ); +SCTP_INPUT_CB (skb)->af->from_skb (&asoc->c.peer_addr, skb,1 ); + nodata:return asoc; - -fail: -sctp_association_free (asoc); -return NULL ; }/* Build a cookie representing asoc.
net/sctp/sm_statefuns.c +2 -5
@@ -6119,13 +6119,10 @@ static int sctp_eat_data(const struct sctp_association *asoc,*/ if (!chunk->ecn_ce_done) { -struct sctp_af *af; +struct sctp_af *af =SCTP_INPUT_CB (chunk->skb)->af; chunk->ecn_ce_done =1 ; - af =sctp_get_af_specific ( -ipver2af (ip_hdr (chunk->skb)->version)); - -if (af && af->is_ce (sctp_gso_headskb (chunk->skb)) && +if (af->is_ce (sctp_gso_headskb (chunk->skb)) && asoc->peer.ecn_capable) {/* Do real work as sideffect. */ sctp_add_cmd_sf(commands, SCTP_CMD_ECN_CE,
Signature · 1 line
2.7.4
Marcelo Ricardo Leitner
maintainerPATCH 5/6 sctp: do not clear chunk->ecn_ce_done flag
In reply to Marcelo Ricardo LeitnerWe should not clear that flag when switching to a new skb from a GSO skb because it would cause ECN processing to happen multiple times per GSO skb, which is not wanted. Instead, let it be processed once per chunk. That is, in other words, once per IP header available.
--- net/sctp/inqueue.c | 1 - 1 file changed, 1 deletion(-)
net/sctp/inqueue.c +0 -1
@@ -217,7 +217,6 @@ new_skb: chunk->auth =0 ; chunk->has_asconf =0 ; chunk->end_of_packet =0 ; - chunk->ecn_ce_done =0 ;if (chunk->head_skb) {struct sctp_input_cb *cb = SCTP_INPUT_CB(chunk->skb),
Signature · 1 line
2.7.4
Marcelo Ricardo Leitner
maintainerPATCH 6/6 sctp: only check for ECN if peer is using it
In reply to Marcelo Ricardo LeitnerCurrently only read-only checks are performed up to the point on where we check if peer is ECN capable, checks which we can avoid otherwise. The flag ecn_ce_done is only used to perform this check once per incoming packet, and nothing more.
Thus this patch moves the peer check up.
--- net/sctp/sm_statefuns.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
net/sctp/sm_statefuns.c +2 -3
@@ -6118,12 +6118,11 @@ static int sctp_eat_data(const struct sctp_association *asoc,* chunk later. */ -if (!chunk->ecn_ce_done) { +if (asoc->peer.ecn_capable && !chunk->ecn_ce_done) {struct sctp_af *af =SCTP_INPUT_CB (chunk->skb)->af; chunk->ecn_ce_done =1 ; -if (af->is_ce (sctp_gso_headskb (chunk->skb)) && - asoc->peer.ecn_capable) { +if (af->is_ce (sctp_gso_headskb (chunk->skb))) {/* Do real work as sideffect. */ sctp_add_cmd_sf (commands,SCTP_CMD_ECN_CE ,SCTP_U32 (tsn));
Signature · 1 line
2.7.4
David Miller
maintainerFrom: Marcelo Ricardo Leitner <[email protected]>
Date: Wed, 13 Jul 2016 15:08:54 -0300
Quoted 7 lines
Patchset is named after the most important fix in it. First two patches are preparing the grounds for the 3rd patch.
After the 3rd, they are not strictly logically related to the patchset, but I kept them together as they depend on each other.
More details on patch changelogs.
Series applied, thanks.