2.6 UDP recvmsg vs POSIX

Hi,

I'm currently investigating a problem in udp recvmsg. In short, a test program selects on the socket, gets woken up as a packet arrives, but recvmsg return EAGAIN because the UDP checksum was wrong.

This broke one real-life application, and reportedly this violates POSIX, as operations on a blocking socket must never return EAGAIN.

The quick and dirty fix would be to always do the checksum in udp_queue_rcv_skb, but I assume this "checksum-outside-the-bottom-half" approach is there because it improves performance, so I've tried to come up with a different solution.

Please find attached a patch that tries to pluck bad udp packets from the receive queue as part of poll/select handling, so that we never mark a udp socket as POLLIN when there are only bad packets in the queue. This is admittedly not very elegant, and it may have side effects I'm not seeing yet. But it seems to fix things for me here, without slowing the bottom half handlers down.

The second half of the patch deals with bad checksums in recvmsg. If an application calls recvmsg on a blocking socket, and we find a packet with a bad checksum (this may happen if the application didn't select first), we start over at the top of the function, so that we block in skb_datagram_receive rather than returning EAGAIN.

Any comments welcome.

Olaf

Olaf Kirch     |  Stop wasting entropy - start using predictable
[email protected]   |  tempfile names today!
---------------+ 

@@ -74,6 +74,10 @@
 extern int	udp_ioctl(struct sock *sk, int cmd, unsigned long arg);
 extern int	udp_disconnect(struct sock *sk, int flags);
 
+extern unsigned int udp_poll(struct file *file,
+			    struct socket *sock,
+			    struct poll_table_struct *wait);
+
 DECLARE_SNMP_STAT(struct udp_mib, udp_statistics);
 #define UDP_INC_STATS(field)		SNMP_INC_STATS(udp_statistics, field)
 #define UDP_INC_STATS_BH(field)		SNMP_INC_STATS_BH(udp_statistics, field)

@@ -912,7 +912,7 @@
 	.socketpair =	sock_no_socketpair,
 	.accept =	sock_no_accept,
 	.getname =	inet_getname,
-	.poll =		datagram_poll,
+	.poll =		udp_poll,
 	.ioctl =	inet_ioctl,
 	.listen =	sock_no_listen,
 	.shutdown =	inet_shutdown,

Looks good to me.

One small optimization would be to quickly check the receive queue without taking the receive queue lock. This would avoid the additional lock in many cases.

-Andi

Thanks Olaf, I'm not ignoring your patch I just didn't get to it today.

BTW I think TCP could hit the same problem. But it did already csum-copy in user context in 2.4 and nobody reported it.

-Andi

Well, first things first, using blocking sockets with select/poll is kind of stupid programming.

Nonetheless, udp_recvmsg() can't return -EAGAIN on a blocking socket.

Andi, TCP does not have this issue, it simply goes back to sleep waiting for more data to arrive if the prequeue delayed checksumming fails.

I think we should fix it like the following, which is basically the last part of Olaf's original patch in this thread. Just wait for another packet if a blocking socket and checksum fails, else if non-blocking -EAGAIN is OK.

To be honest, I believe this was Alexey's original intention when he wrote this code.

===== net/ipv4/udp.c 1.56 vs edited =====

@@ -787,6 +787,7 @@
 	if (flags & MSG_ERRQUEUE)
 		return ip_recv_error(sk, msg, len);
 
+try_again:
 	skb = skb_recv_datagram(sk, flags, noblock, &err);
 	if (!skb)
 		goto out;

@@ -852,7 +853,9 @@
 
 	skb_free_datagram(sk, skb);
 
-	return -EAGAIN;	
+	if (noblock)
+		return -EAGAIN;	
+	goto try_again;
 }
 
 int udp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)

But that's the way many real world applications do it, including for instance the glibc sunrpc code.

But that breaks poll/recvmsg on a blocking socket, because poll with assert POLLIN, but the recvmsg call with block.

I agree that plucking packets off the queue inside poll() is ugly. The alternative would be to just walk the queue to see if there is at least one packet with valid checksum, and clear POLLIN if there isn't, but otherwise leave the queue untouched. This would work in all except the most extreme scenarios where your recv queue is filled to the limit with bad packets, and any new packets with potentially good checksums get dropped. This would make it easy to DoS any UDP based service by spraying it with bad udp packets.

Olaf

Olaf Kirch     |  Stop wasting entropy - start using predictable
[email protected]   |  tempfile names today!
---------------+ 

There is nothing wrong with that, I even pinged Linus about this and he agreed with me.

What stops another thread from pulling a packet from the receive queue and thus emptying it? Nothing. POLLIN does not guarentee a read will give you data right now, it never has and it never will.

Therefore, the only bug was returning -EAGAIN unconditionally and that's what is fixed by the correct half of your changes.

TCP does the same thing Olaf, and there is no way I'm adding the gross poll hacks there :-)

Okay, I can live with that. I will push this info to our posix guys and let them chew on it.

UDPv6 needs the exact same fix BTW.

Cheers,
Olaf

Olaf Kirch     |  Stop wasting entropy - start using predictable
[email protected]   |  tempfile names today!
---------------+