Re: Recent select() handling change breaks Poptop

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.