klibc - Mar 2011 - ipconfig problem if multiple devices are up

Hi,
it seems that ipconfig has a problem if multiple devices are up and
connected to the same network. It seems that it uses the wrong
socket/device index to compare it to incoming packet. To be more
precise, the packet gets discarded in do_pkt_recv as the ifindex from
state differs always from the incoming packet

To reproduce create two tap devices:
$ sudo tunctl -u uli -t tap0                                          
Set 'tap0' persistent and owned by uid 1000
$ sudo tunctl -u uli -t tap1                                              
Set 'tap1' persistent and owned by uid 1000
$ 

I am using for the following step the vde2 software as it runs as a
normal user, but you can do it manually (create a bridge, add tap{0,1}
to that bridge, give the bridge a ip address, start a dhcp server)

,----
| # start a switch 
| vde_switch -s /tmp/switch -d
| # connect the tap devices to the switch
| vde_plug2tap -s /tmp/switch tap0 -d
| vde_plug2tap -s /tmp/switch tap1 -d
| # create a dhcp server
| slirpvde -s /tmp/switch -dhcp
| # start the devices
| sudo ifconfig tap0 up
| sudo ifconfig tap1 up
`----

Now you can test everything:

sudo ipconfig -t 10 tap0
IP-Config: no response after 5 secs - giving up
sudo ipconfig -t 10 tap1
IP-Config: no response after 5 secs - giving up

I added additional debug output:
,----
| diff --git a/usr/kinit/ipconfig/main.c b/usr/kinit/ipconfig/main.c
| index d501bec..8e1b62a 100644
| --- a/usr/kinit/ipconfig/main.c
| +++ b/usr/kinit/ipconfig/main.c
| @@ -312,7 +313,8 @@ static int do_pkt_recv(int pkt_fd, time_t now)
|                 return ret;
|  
|         for (s = slist; s; s = s->next) {
| -               if (s->dev->ifindex == ifindex) {
| +               dprintf("ifindex: %d - s->dev>ifindex: %d -
%s\n",ifindex, s->de
| +               if (s->dev->ifindex == (ifindex)) {
|                         ret = process_receive_event(s, now);
|                         break;
|                 }
|
`----

And the output is (for ipconfig tap1):
ifindex: 1105 - s->dev>ifindex: 1106 - tap1

(1105 is tap0)

If you delete on of the two devices (tunctl -d tap0 / don't forget to
kill the corresponding vde_plug2tap process) everything works like
expected.

Ulrich
-- 
twitter: @mr_ud  | identica: @mru
IRCNet:  mru     | freenode: mrud

On 03/27/2011 06:09 PM, Ulrich Dangel wrote:
> Hi,
> it seems that ipconfig has a problem if multiple devices are up and
> connected to the same network. It seems that it uses the wrong
> socket/device index to compare it to incoming packet. To be more
> precise, the packet gets discarded in do_pkt_recv as the ifindex from
> state differs always from the incoming packet
I wonder if Linux's default ARP behaviour of being somewhat promiscuous 
is involved?  You could try /proc/sys/net/ipv4/conf/all/arp_announce or 
arp_accept?

IOW, maybe the reply packet is coming in a different interface than it 
was sent from?

-- 
Coplanar Networks
http://www.coplanar.net
(519)489-4903

This patch fixes a bug on systems with multiple connected network devices.
As packet_peek uses all devices to receive data instead of a specific
device. As the return value was never reset it was possible that packets
from other devices were returned by packet_peek. As the the ifindex did
not match any ifindex of the specified devices the packet was never
removed and packets for the correct device were never processed.

This patch enhance packet_peek and packet_discard to only work on packages
for the specified device instead of all packets.
---
 usr/kinit/ipconfig/bootp_proto.c |    2 +-
 usr/kinit/ipconfig/dhcp_proto.c  |    2 +-
 usr/kinit/ipconfig/main.c        |   16 ++++++----------
 usr/kinit/ipconfig/packet.c      |   16 +++++++++-------
 usr/kinit/ipconfig/packet.h      |    6 +++---
 5 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/usr/kinit/ipconfig/bootp_proto.c b/usr/kinit/ipconfig/bootp_proto.c
index baf9d3e..f2cc90c 100644
--- a/usr/kinit/ipconfig/bootp_proto.c
+++ b/usr/kinit/ipconfig/bootp_proto.c
@@ -169,7 +169,7 @@ int bootp_recv_reply(struct netdev *dev)
 	};
 	int ret;
 
-	ret = packet_recv(iov, 3);
+	ret = packet_recv(dev, iov, 3);
 	if (ret <= 0)
 		return ret;
 
diff --git a/usr/kinit/ipconfig/dhcp_proto.c b/usr/kinit/ipconfig/dhcp_proto.c
index fc0494d..993db52 100644
--- a/usr/kinit/ipconfig/dhcp_proto.c
+++ b/usr/kinit/ipconfig/dhcp_proto.c
@@ -147,7 +147,7 @@ static int dhcp_recv(struct netdev *dev)
 	};
 	int ret;
 
-	ret = packet_recv(iov, 3);
+	ret = packet_recv(dev, iov, 3);
 	if (ret <= 0)
 		return ret;
 
diff --git a/usr/kinit/ipconfig/main.c b/usr/kinit/ipconfig/main.c
index d501bec..1e48083 100644
--- a/usr/kinit/ipconfig/main.c
+++ b/usr/kinit/ipconfig/main.c
@@ -304,23 +304,19 @@ struct netdev *ifaces;
  */
 static int do_pkt_recv(int pkt_fd, time_t now)
 {
-	int ifindex, ret;
+	int ret = 0;
 	struct state *s;
 
-	ret = packet_peek(&ifindex);
-	if (ret == 0)
-		return ret;
-
 	for (s = slist; s; s = s->next) {
-		if (s->dev->ifindex == ifindex) {
+		ret = packet_peek(s->dev);
+		if (ret) {
 			ret = process_receive_event(s, now);
+			if (ret == 0) {
+				packet_discard(s->dev);
+			}
 			break;
 		}
 	}
-
-	if (ret == 0)
-		packet_discard();
-
 	return ret;
 }
 
diff --git a/usr/kinit/ipconfig/packet.c b/usr/kinit/ipconfig/packet.c
index 84267b7..993a2fa 100644
--- a/usr/kinit/ipconfig/packet.c
+++ b/usr/kinit/ipconfig/packet.c
@@ -167,17 +167,18 @@ int packet_send(struct netdev *dev, struct iovec *iov, int
iov_len)
 }
 
 /*
- * Fetches a bootp packet, but doesn't remove it.
+ * Fetches a bootp packet from specified device, but doesn't remove it.
  * Returns:
  *  0 = Error
  * >0 = A packet of size "ret" is available for interface ifindex
  */
-int packet_peek(int *ifindex)
+int packet_peek(struct netdev *dev)
 {
 	struct sockaddr_ll sll;
 	struct iphdr iph;
 	int ret, sllen = sizeof(struct sockaddr_ll);
 
+	sll.sll_ifindex = dev->ifindex;
 	/*
 	 * Peek at the IP header.
 	 */
@@ -192,21 +193,22 @@ int packet_peek(int *ifindex)
 	if (iph.ihl < 5 || iph.version != IPVERSION)
 		goto discard_pkt;
 
-	*ifindex = sll.sll_ifindex;
 
 	return ret;
 
 discard_pkt:
-	packet_discard();
+	packet_discard(dev);
 	return 0;
 }
 
-void packet_discard(void)
+void packet_discard(struct netdev *dev)
 {
 	struct iphdr iph;
 	struct sockaddr_ll sll;
 	socklen_t sllen = sizeof(sll);
 
+	sll.sll_ifindex = dev->ifindex;
+
 	recvfrom(pkt_fd, &iph, sizeof(iph), 0,
 		 (struct sockaddr *)&sll, &sllen);
 }
@@ -219,7 +221,7 @@ void packet_discard(void)
 *   0 = Discarded packet (non-DHCP/BOOTP traffic)
  * >0 = Size of packet
  */
-int packet_recv(struct iovec *iov, int iov_len)
+int packet_recv(struct netdev* dev, struct iovec *iov, int iov_len)
 {
 	struct iphdr *ip, iph;
 	struct udphdr *udp;
@@ -293,6 +295,6 @@ free_pkt:
 
 discard_pkt:
 	dprintf("discarded\n");
-	packet_discard();
+	packet_discard(dev);
 	return 0;
 }
diff --git a/usr/kinit/ipconfig/packet.h b/usr/kinit/ipconfig/packet.h
index 627d282..524f393 100644
--- a/usr/kinit/ipconfig/packet.h
+++ b/usr/kinit/ipconfig/packet.h
@@ -6,8 +6,8 @@ struct iovec;
 int packet_open(void);
 void packet_close(void);
 int packet_send(struct netdev *dev, struct iovec *iov, int iov_len);
-int packet_peek(int *ifindex);
-void packet_discard(void);
-int packet_recv(struct iovec *iov, int iov_len);
+int packet_peek(struct netdev *dev);
+void packet_discard(struct netdev *dev);
+int packet_recv(struct netdev *dev, struct iovec *iov, int iov_len);
 
 #endif /* IPCONFIG_PACKET_H */
-- 
1.7.1

This patch fixes a bug on systems with multiple connected network devices.
As packet_peek uses all devices to receive data instead of a specific
device. As the return value was never reset it was possible that packets
from other devices were returned by packet_peek. As the the ifindex did
not match any ifindex of the specified devices the packet was never
removed and packets for the correct device were never processed.

This patch reset ret before iterating over the available network device to
discard packages from irrelevant network devices. It maybe better to change
packet_peek and packet_discard to work on specific interfaces.
---
 usr/kinit/ipconfig/main.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/usr/kinit/ipconfig/main.c b/usr/kinit/ipconfig/main.c
index d501bec..ea2abc8 100644
--- a/usr/kinit/ipconfig/main.c
+++ b/usr/kinit/ipconfig/main.c
@@ -311,6 +311,9 @@ static int do_pkt_recv(int pkt_fd, time_t now)
 	if (ret == 0)
 		return ret;
 
+	// reset ret
+	ret = 0;
+
 	for (s = slist; s; s = s->next) {
 		if (s->dev->ifindex == ifindex) {
 			ret = process_receive_event(s, now);
-- 
1.7.1

* Ulrich Dangel wrote [28.03.11 02:30]:
Hi,
>  static int do_pkt_recv(int pkt_fd, time_t now)
>  {
> -	int ifindex, ret;
> +	int ret = 0;
>  	struct state *s;
>  
> -	ret = packet_peek(&ifindex);
> -	if (ret == 0)
> -		return ret;
> -
>  	for (s = slist; s; s = s->next) {
> -		if (s->dev->ifindex == ifindex) {
> +		ret = packet_peek(s->dev);
> +		if (ret) {
>  			ret = process_receive_event(s, now);
> +			if (ret == 0) {
> +				packet_discard(s->dev);
> +			}
>  			break;
>  		}
>  	}
If you use this patch then packet_peek could get removed as well as it does
basically nothing. I did not integrate this as i wanted to be as less as
invasive as possible. But removing packet_peek would make do_pkt_recv
simpler and easier.

Ulrich
-- 
twitter: @mr_ud  | identica: @mru
IRCNet:  mru     | freenode: mrud