Recent select() handling change breaks Poptop

Recent select() handling change breaks Poptop

[Phil Oester]

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

Re: Recent select() handling change breaks Poptop

[Mitchell Blank Jr]

Phil Oester wrote:
> -       .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

Re: Recent select() handling change breaks Poptop

[Phil Oester]

On Mon, Dec 06, 2004 at 06:52:18PM -0800, Mitchell Blank Jr wrote:
> 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

>   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

Re: Recent select() handling change breaks Poptop

[Mitchell Blank Jr]

(adding netdev to cc:)

Phil Oester wrote:
> >   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

[PATCH] fix select() for SOCK_RAW sockets

[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.

Patch is versus 2.6.10-rc3.

Signed-off-by: Mitchell Blank Jr <mitch@sfgoth.com>

-Mitch
--- linux-2.6.10-rc3-VIRGIN/net/ipv4/af_inet.c	2004-12-07 06:37:52.480082706 -0800
+++ linux-2.6.10-rc3/net/ipv4/af_inet.c	2004-12-07 06:57:47.799013216 -0800
@@ -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,

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

[Phil Oester]

On Tue, Dec 07, 2004 at 07:08:34AM -0800, Mitchell Blank Jr 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.

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

Phil

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

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20041207150834.GA75700@gaz.sfgoth.com> (at Tue, 7 Dec 2004 07:08:34 -0800), Mitchell Blank Jr <mitch@sfgoth.com> says:

> 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

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

[Stephen Hemminger]

> 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
--- a/net/ipv6/af_inet6.c	2004-12-07 10:02:50 -08:00
+++ b/net/ipv6/af_inet6.c	2004-12-07 10:02:50 -08:00
@@ -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,

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

[David S. Miller]

On Tue, 7 Dec 2004 10:01:40 -0800
Stephen Hemminger <shemminger@osdl.org> wrote:

> > 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
> --- a/net/ipv6/af_inet6.c	2004-12-07 10:02:50 -08:00
> +++ b/net/ipv6/af_inet6.c	2004-12-07 10:02:50 -08:00
> @@ -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?

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

[Stephen Hemminger]

On Tue, 7 Dec 2004 10:48:15 -0800
"David S. Miller" <davem@davemloft.net> wrote:

> On Tue, 7 Dec 2004 10:01:40 -0800
> Stephen Hemminger <shemminger@osdl.org> wrote:
> 
> > > 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
> > --- a/net/ipv6/af_inet6.c	2004-12-07 10:02:50 -08:00
> > +++ b/net/ipv6/af_inet6.c	2004-12-07 10:02:50 -08:00
> > @@ -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?

yeah, didn't look that deeply.

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

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20041207104815.3f7a4684.davem@davemloft.net> (at Tue, 7 Dec 2004 10:48:15 -0800), "David S. Miller" <davem@davemloft.net> says:

> On Tue, 7 Dec 2004 10:01:40 -0800
> Stephen Hemminger <shemminger@osdl.org> wrote:
> 
> > > 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
> > --- a/net/ipv6/af_inet6.c	2004-12-07 10:02:50 -08:00
> > +++ b/net/ipv6/af_inet6.c	2004-12-07 10:02:50 -08:00
> > @@ -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

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

[Stephen Hemminger]

On Tue, 7 Dec 2004 07:08:34 -0800
Mitchell Blank Jr <mitch@sfgoth.com> 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.

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

[David S. Miller]

On Tue, 7 Dec 2004 07:08:34 -0800
Mitchell Blank Jr <mitch@sfgoth.com> 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.