Recent select() handling change breaks Poptop

PATCH In review Phil Oester 5 participants 13 messages Dec 7, 2004 – Dec 8, 2004

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Phil Oester

author

2004-12-07 00:35 UTC #0 lore raw

The recent changeset [1] dealing with blocking usage of select() seems to break the Poptop PPTP server.

After upgrading from -rc2 to -rc3, I could no longer connect to a server running PopTop. Ended up reverting various -bk snapshots, and the breakage was between -bk13 and -bk14. Reverting this change to af_inet:

-       .poll =         datagram_poll,
+       .poll =         udp_poll,

makes the Poptop server work again.

Any ideas?

Phil

[1] http://linux.bkbits.net:8080/linux-2.5/cset@41ad55f4lM2IigkTUmtz82At8P3duA?nav=index.html|ChangeSet@-2w

Mitchell Blank Jr

2004-12-07 02:49 UTC #1 lore raw

Quoted 6 lines · Phil Oester

(quoted diff) +1 -1
-       .poll =         datagram_poll,
+       .poll =         udp_poll,
makes the Poptop server work again.
Any ideas?

That's very strange. It would be helpful if you could gather:
1. strace of the server, both working and broken
2. a "tcpdump -nvv" of its udp traffic (ideally captured from a seperate server, but from the server would probably be OK too)

Most of the discussion of this change was on the netdev mailing list, it would probably be best to send this information there.

-Mitch

Phil Oester

author

2004-12-07 04:53 UTC #2 lore raw

Quoted 2 lines · Mitchell Blank Jr

That's very strange. It would be helpful if you could gather:
1. strace of the server, both working and broken

Will work on it

Quoted 2 lines

2. a "tcpdump -nvv" of its udp traffic (ideally captured from a seperate server, but from the server would probably be OK too)

PPTP uses TCP 1723 and GRE (proto 47), so there is no udp traffic involved. I suspect the change was made to all datagram traffic with the assumption that UDP was the only protocol impacted. Perhaps GRE was not considered?

Phil

Mitchell Blank Jr

2004-12-07 05:45 UTC #3 lore raw

(adding netdev to cc:)

Quoted 6 lines · Phil Oester

Quoted 2 lines
2. a "tcpdump -nvv" of its udp traffic (ideally captured from a seperate server, but from the server would probably be OK too)
PPTP uses TCP 1723 and GRE (proto 47), so there is no udp traffic involved. I suspect the change was made to all datagram traffic with the assumption that UDP was the only protocol impacted. Perhaps GRE was not considered?

Yeah, it looks like the problem for sure. The patch modifies the structure "inet_dgram_ops" to use udp_poll(), but looking farther down:

static struct inet_protosw inetsw_array[] =
[...]

                .type =       SOCK_DGRAM,
                .protocol =   IPPROTO_UDP,
                .prot =       &udp_prot,
                .ops =        &inet_dgram_ops,
[...]
               .type =       SOCK_RAW,
               .protocol =   IPPROTO_IP,        /* wild card */
               .prot =       &raw_prot,
               .ops =        &inet_dgram_ops,

[...]

so it looks like udp_poll() will end up getting used for both SOCK_DGRAM and SOCK_RAW inet sockets; obviously Poptop is using the latter and failing as a result. No need for the strace/tcpdump data I guess.

The fix is to just make a copy of the inet_dgram_ops called inet_udp_ops and make the udp_poll() change only in that one (and obviously change the SOCK_DGRAM case there to use &inet_udp_ops). I don't have time right this second to spin a patch, but could you try that out and see if it fixes your problem.

-Mitch

Mitchell Blank Jr

2004-12-07 15:07 UTC #4 lore raw

PATCH 1/1 fix select() for SOCK_RAW sockets

Phil: Here's a real patch for you to test. I actually left inet_dgram_ops alone since it's an exported symbol (two of the users just want the .do_ioctl value which is the same between SOCK_DGRAM and SOCK_RAW; the other is ipv6 where it's clearly dealing with a UDP socket -- therefore I think its safest to leave inet_dgram_ops to have the UDP behavior)

Davem: I only tested that this doesn't break UDP; if it works for Phil and Stephen can verify that it doesn't break his bad-checksum UDP tests then please push it for 2.6.10.

Patch is versus 2.6.10-rc3.

Signed-off-by Mitchell Blank Jr <[email protected]>

-Mitch

net/ipv4/af_inet.c +26 -1

@@ -821,6 +821,31 @@
 	.sendpage =	inet_sendpage,
 };
 
+/*
+ * For SOCK_RAW sockets; should be the same as inet_dgram_ops but without
+ * udp_poll
+ */
+static struct proto_ops inet_sockraw_ops = {
+	.family =	PF_INET,
+	.owner =	THIS_MODULE,
+	.release =	inet_release,
+	.bind =		inet_bind,
+	.connect =	inet_dgram_connect,
+	.socketpair =	sock_no_socketpair,
+	.accept =	sock_no_accept,
+	.getname =	inet_getname,
+	.poll =		datagram_poll,
+	.ioctl =	inet_ioctl,
+	.listen =	sock_no_listen,
+	.shutdown =	inet_shutdown,
+	.setsockopt =	sock_common_setsockopt,
+	.getsockopt =	sock_common_getsockopt,
+	.sendmsg =	inet_sendmsg,
+	.recvmsg =	sock_common_recvmsg,
+	.mmap =		sock_no_mmap,
+	.sendpage =	inet_sendpage,
+};
+
 static struct net_proto_family inet_family_ops = {
 	.family = PF_INET,
 	.create = inet_create,

@@ -861,7 +886,7 @@
                .type =       SOCK_RAW,
                .protocol =   IPPROTO_IP,	/* wild card */
                .prot =       &raw_prot,
-               .ops =        &inet_dgram_ops,
+               .ops =        &inet_sockraw_ops,
                .capability = CAP_NET_RAW,
                .no_check =   UDP_CSUM_DEFAULT,
                .flags =      INET_PROTOSW_REUSE,

Phil Oester

author

2004-12-07 17:28 UTC #5 lore raw

Re: [PATCH] fix select() for SOCK_RAW sockets

Quoted 9 lines · Mitchell Blank Jr

Phil: Here's a real patch for you to test. I actually left inet_dgram_ops alone since it's an exported symbol (two of the users just want the .do_ioctl value which is the same between SOCK_DGRAM and SOCK_RAW; the other is ipv6 where it's clearly dealing with a UDP socket -- therefore I think its safest to leave inet_dgram_ops to have the UDP behavior)
Davem: I only tested that this doesn't break UDP; if it works for Phil and Stephen can verify that it doesn't break his bad-checksum UDP tests then please push it for 2.6.10.

Yup, that does indeed fix it for me, thanks.

Phil

YOSHIFUJI Hideaki / 吉藤英明

2004-12-07 17:34 UTC #6 lore raw

Re: [PATCH] fix select() for SOCK_RAW sockets

In reply to Mitchell Blank Jr

In article <[email protected]> (at Tue, 7 Dec 2004 07:08:34 -0800), Mitchell Blank Jr <[email protected]> says:

Quoted 5 lines

Phil: Here's a real patch for you to test. I actually left inet_dgram_ops alone since it's an exported symbol (two of the users just want the .do_ioctl value which is the same between SOCK_DGRAM and SOCK_RAW; the other is ipv6 where it's clearly dealing with a UDP socket -- therefore I think its safest to leave inet_dgram_ops to have the UDP behavior)

Probably, we need to do the same for ipv6, don't we?

--yoshfuji

Stephen Hemminger

2004-12-07 18:01 UTC #7 lore raw

PATCH 1/1 fix select() for SOCK_RAW sockets (ipv6)

Quoted 1 line

Probably, we need to do the same for ipv6, don't we?

net/ipv6/af_inet6.c +22 -1

diff -Nru a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
@@ -513,6 +513,27 @@
 	.sendpage =	sock_no_sendpage,
 };
 
+struct proto_ops inet6_raw_ops = {
+	.family =	PF_INET6,
+	.owner =	THIS_MODULE,
+	.release =	inet6_release,
+	.bind =		inet6_bind,
+	.connect =	inet_dgram_connect,		/* ok		*/
+	.socketpair =	sock_no_socketpair,		/* a do nothing	*/
+	.accept =	sock_no_accept,			/* a do nothing	*/
+	.getname =	inet6_getname, 
+	.poll =		datagram_poll,			/* ok		*/
+	.ioctl =	inet6_ioctl,			/* must change  */
+	.listen =	sock_no_listen,			/* ok		*/
+	.shutdown =	inet_shutdown,			/* ok		*/
+	.setsockopt =	sock_common_setsockopt,		/* ok		*/
+	.getsockopt =	sock_common_getsockopt,		/* ok		*/
+	.sendmsg =	inet_sendmsg,			/* ok		*/
+	.recvmsg =	sock_common_recvmsg,		/* ok		*/
+	.mmap =		sock_no_mmap,
+	.sendpage =	sock_no_sendpage,
+};
+
 static struct net_proto_family inet6_family_ops = {
 	.family = PF_INET6,
 	.create = inet6_create,

@@ -528,7 +549,7 @@
 	.type		= SOCK_RAW,
 	.protocol	= IPPROTO_IP,	/* wild card */
 	.prot		= &rawv6_prot,
-	.ops		= &inet6_dgram_ops,
+	.ops		= &inet6_raw_ops,
 	.capability	= CAP_NET_RAW,
 	.no_check	= UDP_CSUM_DEFAULT,
 	.flags		= INET_PROTOSW_REUSE,

David S. Miller

maintainer

2004-12-07 18:48 UTC #8 lore raw

Re: [PATCH] fix select() for SOCK_RAW sockets (ipv6)

net/ipv6/af_inet6.c

Quoted 6 lines · On Tue, 7 Dec 2004 10:01:40 -0800 Stephen Hemminger <[email protected]> wrote:

Quoted 1 line
Probably, we need to do the same for ipv6, don't we?
diff -Nru a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
net/ipv6/af_inet6.c +0 -0
@@ -513,6 +513,27 @@

We didn't do the "UDP select() fix" on ipv6 because we don't have the delayed checksumming optimization there. So the ipv6 side really doesn't need this change.

Or did I miss something?

Stephen Hemminger

2004-12-07 18:56 UTC #9 lore raw

Re: [PATCH] fix select() for SOCK_RAW sockets (ipv6)

Quoted 14 lines · On Tue, 7 Dec 2004 10:48:15 -0800 "David S. Miller" <[email protected]> wrote:

net/ipv6/af_inet6.c
Quoted 6 lines · On Tue, 7 Dec 2004 10:01:40 -0800 Stephen Hemminger <[email protected]> wrote:
Quoted 1 line
Probably, we need to do the same for ipv6, don't we?
diff -Nru a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
net/ipv6/af_inet6.c +0 -0
@@ -513,6 +513,27 @@
We didn't do the "UDP select() fix" on ipv6 because we don't have the delayed checksumming optimization there. So the ipv6 side really doesn't need this change.

Quoted 1 line

Or did I miss something?

yeah, didn't look that deeply.

YOSHIFUJI Hideaki / 吉藤英明

2004-12-08 07:57 UTC #10 lore raw

Re: [PATCH] fix select() for SOCK_RAW sockets (ipv6)

In reply to David S. Miller

In article <[email protected]> (at Tue, 7 Dec 2004 10:48:15 -0800), "David S. Miller" <[email protected]> says:

Quoted 5 lines

Quoted 2 lines · On Tue, 7 Dec 2004 10:01:40 -0800 Stephen Hemminger <[email protected]> wrote:
Quoted 1 line
Probably, we need to do the same for ipv6, don't we?

Quoted 8 lines

net/ipv6/af_inet6.c
Quoted 4 lines
diff -Nru a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
net/ipv6/af_inet6.c +0 -0
@@ -513,6 +513,27 @@
We didn't do the "UDP select() fix" on ipv6 because we don't have the delayed checksumming optimization there. So the ipv6 side really doesn't need this change.

Ok, thanks for the explanation.

--yoshfuji @ Strasbourg

Stephen Hemminger

2004-12-07 17:45 UTC #11 lore raw

Re: [PATCH] fix select() for SOCK_RAW sockets

In reply to Mitchell Blank Jr

Quoted 10 lines · On Tue, 7 Dec 2004 07:08:34 -0800 Mitchell Blank Jr <[email protected]> wrote:

Phil: Here's a real patch for you to test. I actually left inet_dgram_ops alone since it's an exported symbol (two of the users just want the .do_ioctl value which is the same between SOCK_DGRAM and SOCK_RAW; the other is ipv6 where it's clearly dealing with a UDP socket -- therefore I think its safest to leave inet_dgram_ops to have the UDP behavior)
Davem: I only tested that this doesn't break UDP; if it works for Phil and Stephen can verify that it doesn't break his bad-checksum UDP tests then please push it for 2.6.10.

Thanks, I'll retest UDP today, but it looks right.

David S. Miller

maintainer

2004-12-08 05:28 UTC #12 lore raw

Re: [PATCH] fix select() for SOCK_RAW sockets

In reply to Mitchell Blank Jr

Quoted 3 lines · On Tue, 7 Dec 2004 07:08:34 -0800 Mitchell Blank Jr <[email protected]> wrote:

Davem: I only tested that this doesn't break UDP; if it works for Phil and Stephen can verify that it doesn't break his bad-checksum UDP tests then please push it for 2.6.10.

Looks good Mitchell, patch applied.

Thanks.