From 2cfa2271604bb26e75b828d38f357ed084464795 Mon Sep 17 00:00:00 2001 From: David Howells Date: Fri, 5 Oct 2018 14:05:35 +0100 Subject: [PATCH] rxrpc: Fix the data_ready handler Fix the rxrpc_data_ready() function to pick up all packets and to not miss any. There are two problems: (1) The sk_data_ready pointer on the UDP socket is set *after* it is bound. This means that it's open for business before we're ready to dequeue packets and there's a tiny window exists in which a packet can sneak onto the receive queue, but we never know about it. Fix this by setting the pointers on the socket prior to binding it. (2) skb_recv_udp() will return an error (such as ENETUNREACH) if there was an error on the transmission side, even though we set the sk_error_report hook. Because rxrpc_data_ready() returns immediately in such a case, it never actually removes its packet from the receive queue. Fix this by abstracting out the UDP dequeuing and checksumming into a separate function that keeps hammering on skb_recv_udp() until it returns -EAGAIN, passing the packets extracted to the remainder of the function. and two potential problems: (3) It might be possible in some circumstances or in the future for packets to be being added to the UDP receive queue whilst rxrpc is running consuming them, so the data_ready() handler might get called less often than once per packet. Allow for this by fully draining the queue on each call as (2). (4) If a packet fails the checksum check, the code currently returns after discarding the packet without checking for more. Allow for this by fully draining the queue on each call as (2). Fixes: 17926a79320a ("[AF_RXRPC]: Provide secure RxRPC sockets for use by userspace and kernel both") Signed-off-by: David Howells Acked-by: Paolo Abeni --- net/rxrpc/input.c | 68 +++++++++++++++++++++++++++--------------------- net/rxrpc/local_object.c | 11 ++++---- 2 files changed, 44 insertions(+), 35 deletions(-) diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c index c5af995..c3114fa 100644 --- a/net/rxrpc/input.c +++ b/net/rxrpc/input.c @@ -1121,7 +1121,7 @@ int rxrpc_extract_header(struct rxrpc_skb_priv *sp, struct sk_buff *skb) * shut down and the local endpoint from going away, thus sk_user_data will not * be cleared until this function returns. */ -void rxrpc_data_ready(struct sock *udp_sk) +void rxrpc_input_packet(struct sock *udp_sk, struct sk_buff *skb) { struct rxrpc_connection *conn; struct rxrpc_channel *chan; @@ -1130,39 +1130,11 @@ void rxrpc_data_ready(struct sock *udp_sk) struct rxrpc_local *local = udp_sk->sk_user_data; struct rxrpc_peer *peer = NULL; struct rxrpc_sock *rx = NULL; - struct sk_buff *skb; unsigned int channel; - int ret, skew = 0; + int skew = 0; _enter("%p", udp_sk); - ASSERT(!irqs_disabled()); - - skb = skb_recv_udp(udp_sk, 0, 1, &ret); - if (!skb) { - if (ret == -EAGAIN) - return; - _debug("UDP socket error %d", ret); - return; - } - - if (skb->tstamp == 0) - skb->tstamp = ktime_get_real(); - - rxrpc_new_skb(skb, rxrpc_skb_rx_received); - - _net("recv skb %p", skb); - - /* we'll probably need to checksum it (didn't call sock_recvmsg) */ - if (skb_checksum_complete(skb)) { - rxrpc_free_skb(skb, rxrpc_skb_rx_freed); - __UDP_INC_STATS(sock_net(udp_sk), UDP_MIB_INERRORS, 0); - _leave(" [CSUM failed]"); - return; - } - - __UDP_INC_STATS(sock_net(udp_sk), UDP_MIB_INDATAGRAMS, 0); - /* The UDP protocol already released all skb resources; * we are free to add our own data there. */ @@ -1181,6 +1153,8 @@ void rxrpc_data_ready(struct sock *udp_sk) } } + if (skb->tstamp == 0) + skb->tstamp = ktime_get_real(); trace_rxrpc_rx_packet(sp); switch (sp->hdr.type) { @@ -1398,3 +1372,37 @@ reject_packet: rxrpc_reject_packet(local, skb); _leave(" [badmsg]"); } + +void rxrpc_data_ready(struct sock *udp_sk) +{ + struct sk_buff *skb; + int ret; + + for (;;) { + skb = skb_recv_udp(udp_sk, 0, 1, &ret); + if (!skb) { + if (ret == -EAGAIN) + return; + + /* If there was a transmission failure, we get an error + * here that we need to ignore. + */ + _debug("UDP socket error %d", ret); + continue; + } + + rxrpc_new_skb(skb, rxrpc_skb_rx_received); + + /* we'll probably need to checksum it (didn't call sock_recvmsg) */ + if (skb_checksum_complete(skb)) { + rxrpc_free_skb(skb, rxrpc_skb_rx_freed); + __UDP_INC_STATS(sock_net(udp_sk), UDP_MIB_INERRORS, 0); + _debug("csum failed"); + continue; + } + + __UDP_INC_STATS(sock_net(udp_sk), UDP_MIB_INDATAGRAMS, 0); + + rxrpc_input_packet(udp_sk, skb); + } +} diff --git a/net/rxrpc/local_object.c b/net/rxrpc/local_object.c index 94d234e..30862f4 100644 --- a/net/rxrpc/local_object.c +++ b/net/rxrpc/local_object.c @@ -122,6 +122,12 @@ static int rxrpc_open_socket(struct rxrpc_local *local, struct net *net) return ret; } + /* set the socket up */ + sock = local->socket->sk; + sock->sk_user_data = local; + sock->sk_data_ready = rxrpc_data_ready; + sock->sk_error_report = rxrpc_error_report; + /* if a local address was supplied then bind it */ if (local->srx.transport_len > sizeof(sa_family_t)) { _debug("bind"); @@ -191,11 +197,6 @@ static int rxrpc_open_socket(struct rxrpc_local *local, struct net *net) BUG(); } - /* set the socket up */ - sock = local->socket->sk; - sock->sk_user_data = local; - sock->sk_data_ready = rxrpc_data_ready; - sock->sk_error_report = rxrpc_error_report; _leave(" = 0"); return 0; -- 2.7.4