Skip to contentSkip to conversation

knfsd: Resolve IPv6 related link error

PATCH New NeilBrown 2 participants 8 messages Mar 2, 2007 – Mar 5, 2007

Subsystems (5)
lore
Share & tools
lore

NeilBrown

author
#0 lore raw

PATCH 1/1 knfsd: Resolve IPv6 related link error

Current mainline has a compile linkage problem if both
CONFIG_IPV6=m
CONFIG_SUNRPC=y

because net/sunrpc/svcsock.c conditionally used a function defined in the IPv6 module.

These three patches resolve the issue.

The problem is caused because svcsock needs to get the source and destination address for a udp packet, but doesn't want to just use sock_recvmsg like userspace would as it wants to be able to use the data directly out of the skbuff rather than copying it (when practical).

Currently it copies code from udp.c (both ipv4/ and ipv6/) and this causes the problem.

This patch changes it to use kernel_recvmsg with a length of 0 and flags of MSG_PEEK to get the addresses but leave the data untouched.

A small problem here is that kernel_recvmsg always checks the checksum, so in the case of a large packet we will check the checksum at a different time to when we copy it out into a buffer, which is not ideal.

So the second patch of this series avoids the check when recv_msg is called with size==0 and flags==MSG_PEEK. This change should be acked by someone on netdev before going upsteam!!! The rest of the series is still appropriate without the patch, it is just a small optimisation.

Finally the last patch removes all the
#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
from sunrpc as it really isn't needed and just hides this sort of problem.

Patches 1 and 3 are suitable for 2.6.21. Patch 2 needs confirmation.

Thanks,
NeilBrown

[PATCH 001 of 3] knfsd: Use recv_msg to get peer address for NFSD instead of code-copying [PATCH 002 of 3] knfsd: Avoid checksum checks when collecting metadata for a UDP packet. [PATCH 003 of 3] knfsd: Remove CONFIG_IPV6 ifdefs from sunrpc server code.

author
#1 lore raw

PATCH 1/1 knfsd: Use recv_msg to get peer address for NFSD instead of code-copying

The sunrpc server code needs to know the source and destination address for UDP packets so it can reply properly.
It currently copies code out of the network stack to pick the pieces out of the skb.
This is ugly and causes compile problems with the IPv6 stuff.

So, rip that out and use recv_msg instead. This is a much cleaner interface, but has a slight cost in that the checksum is now checked before the copy, so we don't benefit from doing both at the same time. This can probably be fixed.

### Diffstat output

 ./net/sunrpc/svcsock.c |   63 ++++++++++++++++++++++++-------------------------
 1 file changed, 31 insertions(+), 32 deletions(-)
net/sunrpc/svcsock.c +31 -32
diff .prev/net/sunrpc/svcsock.c ./net/sunrpc/svcsock.c
@@ -721,45 +721,23 @@ svc_write_space(struct sock *sk)
 	}
 }
 
-static void svc_udp_get_sender_address(struct svc_rqst *rqstp,
-					struct sk_buff *skb)
+static inline void svc_udp_get_dest_address(struct svc_rqst *rqstp,
+					    struct cmsghdr *cmh)
 {
 	switch (rqstp->rq_sock->sk_sk->sk_family) {
 	case AF_INET: {
-		/* this seems to come from net/ipv4/udp.c:udp_recvmsg */
-			struct sockaddr_in *sin = svc_addr_in(rqstp);
-
-			sin->sin_family = AF_INET;
-			sin->sin_port = skb->h.uh->source;
-			sin->sin_addr.s_addr = skb->nh.iph->saddr;
-			rqstp->rq_addrlen = sizeof(struct sockaddr_in);
-			/* Remember which interface received this request */
-			rqstp->rq_daddr.addr.s_addr = skb->nh.iph->daddr;
-		}
+		struct in_pktinfo *pki = CMSG_DATA(cmh);
+		rqstp->rq_daddr.addr.s_addr = pki->ipi_spec_dst.s_addr;
 		break;
+		}
 #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 	case AF_INET6: {
-		/* this is derived from net/ipv6/udp.c:udpv6_recvmesg */
-			struct sockaddr_in6 *sin6 = svc_addr_in6(rqstp);
-
-			sin6->sin6_family = AF_INET6;
-			sin6->sin6_port = skb->h.uh->source;
-			sin6->sin6_flowinfo = 0;
-			sin6->sin6_scope_id = 0;
-			if (ipv6_addr_type(&sin6->sin6_addr) &
-							IPV6_ADDR_LINKLOCAL)
-				sin6->sin6_scope_id = IP6CB(skb)->iif;
-			ipv6_addr_copy(&sin6->sin6_addr,
-							&skb->nh.ipv6h->saddr);
-			rqstp->rq_addrlen = sizeof(struct sockaddr_in);
-			/* Remember which interface received this request */
-			ipv6_addr_copy(&rqstp->rq_daddr.addr6,
-							&skb->nh.ipv6h->saddr);
-		}
+		struct in6_pktinfo *pki = CMSG_DATA(cmh);
+		ipv6_addr_copy(&rqstp->rq_daddr.addr6, &pki->ipi6_addr);
 		break;
+		}
 #endif
 	}
-	return;
 }
 
 /*
@@ -771,7 +749,15 @@ svc_udp_recvfrom(struct svc_rqst *rqstp)
 	struct svc_sock	*svsk = rqstp->rq_sock;
 	struct svc_serv	*serv = svsk->sk_server;
 	struct sk_buff	*skb;
+	char		buffer[CMSG_SPACE(sizeof(union svc_pktinfo_u))];
+	struct cmsghdr *cmh = (struct cmsghdr *)buffer;
 	int		err, len;
+	struct msghdr msg = {
+		.msg_name = svc_addr(rqstp),
+		.msg_control = cmh,
+		.msg_controllen = sizeof(buffer),
+		.msg_flags = MSG_DONTWAIT,
+	};
 
 	if (test_and_clear_bit(SK_CHNGBUF, &svsk->sk_flags))
 	    /* udp sockets need large rcvbuf as all pending
@@ -797,7 +783,9 @@ svc_udp_recvfrom(struct svc_rqst *rqstp)
 	}
 
 	clear_bit(SK_DATA, &svsk->sk_flags);
-	while ((skb = skb_recv_datagram(svsk->sk_sk, 0, 1, &err)) == NULL) {
+	while ((err == kernel_recvmsg(svsk->sk_sock, &msg, NULL,
+				      0, 0, MSG_PEEK)) < 0 ||
+	       (skb = skb_recv_datagram(svsk->sk_sk, 0, 1, &err)) == NULL) {
 		if (err == -EAGAIN) {
 			svc_sock_received(svsk);
 			return err;
@@ -805,6 +793,7 @@ svc_udp_recvfrom(struct svc_rqst *rqstp)
 		/* possibly an icmp error */
 		dprintk("svc: recvfrom returned error %d\n", -err);
 	}
+	rqstp->rq_addrlen = sizeof(rqstp->rq_addr);
 	if (skb->tstamp.off_sec == 0) {
 		struct timeval tv;
 
@@ -827,7 +816,7 @@ svc_udp_recvfrom(struct svc_rqst *rqstp)
 
 	rqstp->rq_prot = IPPROTO_UDP;
 
-	svc_udp_get_sender_address(rqstp, skb);
+	svc_udp_get_dest_address(rqstp, cmh);
 
 	if (skb_is_nonlinear(skb)) {
 		/* we have to copy */
@@ -884,6 +873,9 @@ svc_udp_sendto(struct svc_rqst *rqstp)
 static void
 svc_udp_init(struct svc_sock *svsk)
 {
+	int one = 1;
+	mm_segment_t oldfs;
+
 	svsk->sk_sk->sk_data_ready = svc_udp_data_ready;
 	svsk->sk_sk->sk_write_space = svc_write_space;
 	svsk->sk_recvfrom = svc_udp_recvfrom;
@@ -899,6 +891,13 @@ svc_udp_init(struct svc_sock *svsk)
 
 	set_bit(SK_DATA, &svsk->sk_flags); /* might have come in before data_ready set up */
 	set_bit(SK_CHNGBUF, &svsk->sk_flags);
+
+	oldfs = get_fs();
+	set_fs(KERNEL_DS);
+	/* make sure we get destination address info */
+	svsk->sk_sock->ops->setsockopt(svsk->sk_sock, IPPROTO_IP, IP_PKTINFO,
+				       (char __user *)&one, sizeof(one));
+	set_fs(oldfs);
 }
 
 /*

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

#2 lore raw

Re: [NFS] [PATCH 001 of 3] knfsd: Use recv_msg to get peer address for NFSD instead of code-copying

Quoted 5 lines · NeilBrown

The sunrpc server code needs to know the source and destination address for UDP packets so it can reply properly.
It currently copies code out of the network stack to pick the pieces out of the skb.
This is ugly and causes compile problems with the IPv6 stuff.

... and this IPv6 code could never have worked anyway:

Quoted 1 line

case AF_INET6: {

...

Review

Quoted 1 line
(quoted diff) +0 -1
-			rqstp->rq_addrlen = sizeof(struct sockaddr_in);

... this should have been sizeof(sockaddr_in6)...

net/sunrpc/svcsock.c svc_write_space(struct sock *sk) · line 757

Quoted 3 lines
net/sunrpc/svcsock.c +0 -3
-			/* Remember which interface received this request */
-			ipv6_addr_copy(&rqstp->rq_daddr.addr6,
-							&skb->nh.ipv6h->saddr);

.... and this should have copied from daddr, not saddr.

But I find using recvmsg just for getting at the addresses a little awkward too. And I think to be on the safe side, you should check that you're really looking at a PKTINFO cmsg rather than something else.

Olaf

Signature · 11 lines
Olaf Kirch  |  --- o --- Nous sommes du soleil we love when we play
[email protected] |    / | \   sol.dhoop.naytheet.ah kin.ir.samse.qurax

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
author
#3 lore raw

Re: [NFS] [PATCH 001 of 3] knfsd: Use recv_msg to get peer address for NFSD instead of code-copying

Quoted 8 lines · On Monday March 5, [email protected] wrote:
Quoted 5 lines · NeilBrown

The sunrpc server code needs to know the source and destination address for UDP packets so it can reply properly.
It currently copies code out of the network stack to pick the pieces out of the skb.
This is ugly and causes compile problems with the IPv6 stuff.

... and this IPv6 code could never have worked anyway:

:-(
It's hard to test the IPv6 server until we have an IPv6 client I guess, so thanks for the code review, even though we aren't going to end up using that code...

Quoted 3 lines

But I find using recvmsg just for getting at the addresses a little awkward too.

Do you? It's surely a lot better than code duplication, and it is exactly how you would get the information from user-space.

Quoted 3 lines

And I think to be on the safe side, you
should check that you're really looking at a PKTINFO cmsg rather than something else.

Maybe.....
But is there really a chance that it might not be PKTINFO? And what do you do if it isn't?
Log an error and drop the packet I guess.

I'll see what I can do.

NeilBrown

#4 lore raw

Re: [NFS] [PATCH 001 of 3] knfsd: Use recv_msg to get peer address for NFSD instead of code-copying

In reply to NeilBrown

Hi Neil,

here's another minor comment:

net/sunrpc/svcsock.c svc_write_space(struct sock *sk) · line 732

Quoted 9 lines · NeilBrown
net/sunrpc/svcsock.c +5 -0
+static inline void svc_udp_get_dest_address(struct svc_rqst *rqstp,
+					    struct cmsghdr *cmh)
 {
 	switch (rqstp->rq_sock->sk_sk->sk_family) {
 	case AF_INET: {
+		struct in_pktinfo *pki = CMSG_DATA(cmh);
+		rqstp->rq_daddr.addr.s_addr = pki->ipi_spec_dst.s_addr;
 		break;
+		}

...

The daddr that is extracted here will only ever be used to build another PKTINFO cmsg when sending the reply. So it would be much easier to just store the raw control message in the svc_rqst, without looking at its contents, and send it out along with the reply, unchanged.

Olaf

Signature · 2 lines
Olaf Kirch  |  --- o --- Nous sommes du soleil we love when we play
[email protected] |    / | \   sol.dhoop.naytheet.ah kin.ir.samse.qurax
author
#5 lore raw

Re: [NFS] [PATCH 001 of 3] knfsd: Use recv_msg to get peer address for NFSD instead of code-copying

Quoted 17 lines · On Monday March 5, [email protected] wrote:

Hi Neil,

here's another minor comment:

Quoted 9 lines · NeilBrown
net/sunrpc/svcsock.c +5 -0
+static inline void svc_udp_get_dest_address(struct svc_rqst *rqstp,
+					    struct cmsghdr *cmh)
 {
 	switch (rqstp->rq_sock->sk_sk->sk_family) {
 	case AF_INET: {
+		struct in_pktinfo *pki = CMSG_DATA(cmh);
+		rqstp->rq_daddr.addr.s_addr = pki->ipi_spec_dst.s_addr;
 		break;
+		}

...

Quoted 5 lines

The daddr that is extracted here will only ever be used to build another PKTINFO cmsg when sending the reply. So it would be much easier to just store the raw control message in the svc_rqst, without looking at its contents, and send it out along with the reply, unchanged.

Yes, sounds tempting, doesn't it?
Unfortunately it isn't that simple as I found out when the sunrpc code in glibc did exactly that.

You see sendmsg will use the interface-number as well as the source address from the PKTINFO structure.

Suppose my server has two interfaces (A and B) on two subnets that both are connected to some router which is connected to a third subnet that my client is on. Further, suppose my server has only one default route, out interface A.
The client chooses the IP address of interface B and sends a request. It arrives on interface B and is processed.
If the PKTINFO received is passed unchanged to sendmsg, the pack will be sent out interface B. But interfacve B doesn't have a route to that client, so the packet is dropped.

This exactly what was happening for me with mountd a few years ago.

So yes, we could just zero the interface field, but I think it is clearer to extract that wanted data, then re-insert it. They really are different structures with different meanings (send verse receive) which happen to have the same layout.

Thanks,
NeilBrown

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

author
#6 lore raw

PATCH 1/1 knfsd: Avoid checksum checks when collecting metadata for a UDP packet.

In reply to NeilBrown

When recv_msg is called with a size of 0 and MSG_PEEK (and sunrpc/svcsock.c does), it is clear that we only interested in metadata (from/to addresses) and not the data, so don't do any checksum checking at this point. Leave that until the data is requested.

### Diffstat output

 ./net/ipv4/udp.c |    3 +++
 ./net/ipv6/udp.c |    4 ++++
 2 files changed, 7 insertions(+)
net/ipv4/udp.c +3 -0
diff .prev/net/ipv4/udp.c ./net/ipv4/udp.c
@@ -846,6 +846,9 @@ try_again:
 			goto csum_copy_err;
 		copy_only = 1;
 	}
+	if (len == 0 &&  (flags & MSG_PEEK))
+		/* avoid checksum concerns when just getting metadata */
+		copy_only = 1;
 
 	if (copy_only)
 		err = skb_copy_datagram_iovec(skb, sizeof(struct udphdr),
net/ipv6/udp.c +4 -0
diff .prev/net/ipv6/udp.c ./net/ipv6/udp.c
@@ -151,6 +151,10 @@ try_again:
 		copy_only = 1;
 	}
 
+	if (len == 0 &&  (flags & MSG_PEEK))
+		/* avoid checksum concerns when just getting metadata */
+		copy_only = 1;
+
 	if (copy_only)
 		err = skb_copy_datagram_iovec(skb, sizeof(struct udphdr),
 					      msg->msg_iov, copied       );

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

author
#7 lore raw

PATCH 1/1 knfsd: Remove CONFIG_IPV6 ifdefs from sunrpc server code.

In reply to NeilBrown

They don't really save that much, and aren't worth the hassle.

### Diffstat output

 ./include/linux/sunrpc/svc.h |    2 --
 ./net/sunrpc/svcsock.c       |   13 +++----------
 2 files changed, 3 insertions(+), 12 deletions(-)
include/linux/sunrpc/svc.h +0 -2
diff .prev/include/linux/sunrpc/svc.h ./include/linux/sunrpc/svc.h
@@ -194,9 +194,7 @@ static inline void svc_putu32(struct kve
 
 union svc_addr_u {
     struct in_addr	addr;
-#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
     struct in6_addr	addr6;
-#endif
 };
 
 /*
net/sunrpc/svcsock.c +3 -10
diff .prev/net/sunrpc/svcsock.c ./net/sunrpc/svcsock.c
@@ -131,13 +131,13 @@ static char *__svc_print_addr(struct soc
 			NIPQUAD(((struct sockaddr_in *) addr)->sin_addr),
 			htons(((struct sockaddr_in *) addr)->sin_port));
 		break;
-#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+
 	case AF_INET6:
 		snprintf(buf, len, "%x:%x:%x:%x:%x:%x:%x:%x, port=%u",
 			NIP6(((struct sockaddr_in6 *) addr)->sin6_addr),
 			htons(((struct sockaddr_in6 *) addr)->sin6_port));
 		break;
-#endif
+
 	default:
 		snprintf(buf, len, "unknown address type: %d", addr->sa_family);
 		break;
@@ -449,9 +449,7 @@ svc_wake_up(struct svc_serv *serv)
 
 union svc_pktinfo_u {
 	struct in_pktinfo pkti;
-#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 	struct in6_pktinfo pkti6;
-#endif
 };
 
 static void svc_set_cmsg_data(struct svc_rqst *rqstp, struct cmsghdr *cmh)
@@ -467,7 +465,7 @@ static void svc_set_cmsg_data(struct svc
 			cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
 		}
 		break;
-#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+
 	case AF_INET6: {
 			struct in6_pktinfo *pki = CMSG_DATA(cmh);
 
@@ -479,7 +477,6 @@ static void svc_set_cmsg_data(struct svc
 			cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
 		}
 		break;
-#endif
 	}
 	return;
 }
@@ -730,13 +727,11 @@ static inline void svc_udp_get_dest_addr
 		rqstp->rq_daddr.addr.s_addr = pki->ipi_spec_dst.s_addr;
 		break;
 		}
-#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 	case AF_INET6: {
 		struct in6_pktinfo *pki = CMSG_DATA(cmh);
 		ipv6_addr_copy(&rqstp->rq_daddr.addr6, &pki->ipi6_addr);
 		break;
 		}
-#endif
 	}
 }
 
@@ -976,11 +971,9 @@ static inline int svc_port_is_privileged
 	case AF_INET:
 		return ntohs(((struct sockaddr_in *)sin)->sin_port)
 			< PROT_SOCK;
-#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 	case AF_INET6:
 		return ntohs(((struct sockaddr_in6 *)sin)->sin6_port)
 			< PROT_SOCK;
-#endif
 	default:
 		return 0;
 	}

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs