[PATCH net] udp: fix behavior of wrong checksums

[PATCH net] udp: fix behavior of wrong checksums

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

We have two problems in UDP stack related to bogus checksums :

1) We return -EAGAIN to application even if receive queue is not empty.
   This breaks applications using edge trigger epoll()

2) Under UDP flood, we can loop forever without yielding to other
   processes, potentially hanging the host, especially on non SMP.


This patch is an attempt to make things better.

We might in the future add extra support for rt applications
wanting to better control time spent doing a recv() in a hostile
environment. For example we could validate checksums before queuing
packets in socket receive queue.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Willem de Bruijn <willemb@google.com>
---
 net/ipv4/udp.c |    6 ++----
 net/ipv6/udp.c |    6 ++----
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index d10b7e0112eb..1c92ea67baef 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1345,10 +1345,8 @@ csum_copy_err:
 	}
 	unlock_sock_fast(sk, slow);
 
-	if (noblock)
-		return -EAGAIN;
-
-	/* starting over for a new packet */
+	/* starting over for a new packet, but check if we need to yield */
+	cond_resched();
 	msg->msg_flags &= ~MSG_TRUNC;
 	goto try_again;
 }
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index c2ec41617a35..e51fc3eee6db 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -525,10 +525,8 @@ csum_copy_err:
 	}
 	unlock_sock_fast(sk, slow);
 
-	if (noblock)
-		return -EAGAIN;
-
-	/* starting over for a new packet */
+	/* starting over for a new packet, but check if we need to yield */
+	cond_resched();
 	msg->msg_flags &= ~MSG_TRUNC;
 	goto try_again;
 }

Re: [PATCH net] udp: fix behavior of wrong checksums

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sat, 30 May 2015 09:16:53 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> We have two problems in UDP stack related to bogus checksums :
> 
> 1) We return -EAGAIN to application even if receive queue is not empty.
>    This breaks applications using edge trigger epoll()
> 
> 2) Under UDP flood, we can loop forever without yielding to other
>    processes, potentially hanging the host, especially on non SMP.
> 
> 
> This patch is an attempt to make things better.
> 
> We might in the future add extra support for rt applications
> wanting to better control time spent doing a recv() in a hostile
> environment. For example we could validate checksums before queuing
> packets in socket receive queue.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Yeah this looks correct, applied and queued up for -stable.

Thanks!

Re: [PATCH net] udp: fix behavior of wrong checksums

[Eric Dumazet]

On Sun, 2015-05-31 at 21:42 -0700, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Sat, 30 May 2015 09:16:53 -0700
> 
> > From: Eric Dumazet <edumazet@google.com>
> > 
> > We have two problems in UDP stack related to bogus checksums :
> > 
> > 1) We return -EAGAIN to application even if receive queue is not empty.
> >    This breaks applications using edge trigger epoll()
> > 
> > 2) Under UDP flood, we can loop forever without yielding to other
> >    processes, potentially hanging the host, especially on non SMP.
> > 
> > 
> > This patch is an attempt to make things better.
> > 
> > We might in the future add extra support for rt applications
> > wanting to better control time spent doing a recv() in a hostile
> > environment. For example we could validate checksums before queuing
> > packets in socket receive queue.
> > 
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> 
> Yeah this looks correct, applied and queued up for -stable.
> 
> Thanks!

Thanks David

It seems MSG_PEEK support is broken with checksums and truncated data:

Multiple threads could operate on same skb and
__udp_lib_checksum_complete() / __skb_checksum_complete() can do bad
things.

Tom, any idea how this could be fixed ? It looks you added this bug in
3.16


        /*
         * If checksum is needed at all, try to do it while copying the
         * data.  If the data is truncated, or if we only want a partial
         * coverage checksum (UDP-Lite), do it before the copy.
         */

        if (copied < ulen || UDP_SKB_CB(skb)->partial_cov) {
                if (udp_lib_checksum_complete(skb))
                        goto csum_copy_err;
        }


...

__sum16 __skb_checksum_complete(struct sk_buff *skb)
{
        __wsum csum;
        __sum16 sum;

        csum = skb_checksum(skb, 0, skb->len, 0);

        /* skb->csum holds pseudo checksum */
        sum = csum_fold(csum_add(skb->csum, csum));
        if (likely(!sum)) {
                if (unlikely(skb->ip_summed == CHECKSUM_COMPLETE) &&
                    !skb->csum_complete_sw)
                        netdev_rx_csum_fault(skb->dev);
        }

        /* Save full packet checksum */
        skb->csum = csum;
        skb->ip_summed = CHECKSUM_COMPLETE;
        skb->csum_complete_sw = 1;
        skb->csum_valid = !sum;

        return sum;
}


Maybe adding a 'readonly' bool param to __skb_checksum_complete() ?