[PATCH 2.4.20] entered/exited promiscuous mode flip/flop error (2nd submit attempt--white space error in added source lines corrected)

[PATCH 2.4.20] entered/exited promiscuous mode flip/flop error (2nd submit attempt--white space error in added source lines corrected)

[Jim Nisbet]

Re: bug in entering/exiting promiscuous read mode

A number of people have mentioned that they get a weird situation where when
they *start* a program that does promiscuous network reads (with, say,
‘tcpdump –i eth0’). They then get a kernel message “left promiscuous mode”
when the program starts and the message “entered promiscuous mode” when it
exits – the exact opposite of what should happen.  I’ve tracked this problem
down and fixed it in the 2.4.20 kernel sources.  I have also verified that
the problem still exists in the latest dev sources (2.5.73).  The patch file
is at the end of this email.
 
The problem comes when the interface is “downed” the dev->promiscuity count
gets incorrectly decremented by 2.  This happens because the
dev_change_flags routine calls dev_mc_upload which decrements the count and
the count gets decremented a 2nd time when dev_change_flags calls dev_close
which calls
notifier_call_chain->packet_notifier->packet_dev_mclist->packet_dev_mc.  The
logic in dev_set_promiscuity is to test if dev->promiscuity == 0.  What
happens is the count is now set to -1.  When a new caller increments the
count by 1 to enter promisc mode but that means that the count is now set to
0 which means exit promisc; when the caller exits the count is decremented
and is once again set to -1 which means re-enter promisc mode (since the
count is not == 0).

 
There is a similar problem when the interface is enabled when there is
already a promisc reader.  The count is incremented twice.  
 
I’ve fixed the problem and verified the fix by running tcpdump and then
doing various combinations of ifconfig..up/down.  I also added one new
diagnostic message and changed the code so that it will not do the inverse
logic thing if the count should ever be wrong again in the future.
 
 
My solution involves minor changes to net/core/dev.c and
net/packet/af_packet.c.  The patch file is attached to this email.  The
changes are:
 
1.  Don’t increment/decrement the promisc count if the interface is not up
in packet_dev_mc routine.  Note: the request to be in promisc mode is still
stored on the multicast (mc) list so when the interface is enabled the
interface will be put back in promisc mode by dev_mc_upload as it should be.
 
2.  In packet_notifier call packet_dev_mclist to remove multicast/promisc
mode when the interface is GOING_DOWN not DOWN.  Since if it is “DOWN” the
request would now be ignored.
 
3.  Change dev_set_promiscuity routine to check for a count of <= 0 and then
set the count to 0 and unset promisc mode.  What this means is that if the
count is ever wrong the interface will go out of promisc mode “too early”. 
But a new program could still be started and it would enter promisc mode
again.  It will never invert the logic.
 
4.  Write a kernel info message whenever the use count is
incremented/decremented instead of *just* when promisc is turned on/off.  If
a 2nd caller uses setsockopt to put the interface in promisc mode, a message
would be generated:
               Jun 29 16:57:08 tabby kernel: device eth1 still in
promiscuous mode (use count now 2)
 
and when one of the existing callers closes the socket connection then the
message would be:
               Jun 29 16:57:20 tabby kernel: device eth1 still in
promiscuous mode (use count now 1)
 
 
For those who don’t want to patch the kernel, I’d say stay away from
ifconfig up/down if you have a promisc read operation going.


================= diff -u patch follows ===================

diff -ru linux-2.4.20-18.8/net/core/dev.c
linux-2.4.20-18.8-tablus/net/core/dev.c
--- linux-2.4.20-18.8/net/core/dev.c	2003-05-29 03:46:18.000000000 -0700
+++ linux-2.4.20-18.8-tablus/net/core/dev.c	2003-06-30
18:16:59.000000000 -0700
@@ -1949,8 +1949,10 @@
 	unsigned short old_flags = dev->flags;
 
 	dev->flags |= IFF_PROMISC;
-	if ((dev->promiscuity += inc) == 0)
+	if ((dev->promiscuity += inc) <= 0) {
+		dev->promiscuity = 0;
 		dev->flags &= ~IFF_PROMISC;
+	}
 	if (dev->flags^old_flags) {
 #ifdef CONFIG_NET_FASTROUTE
 		if (dev->flags&IFF_PROMISC) {
@@ -1963,6 +1965,12 @@
 		printk(KERN_INFO "device %s %s promiscuous mode\n",
 		       dev->name, (dev->flags&IFF_PROMISC) ? "entered" :
"left");
 	}
+	else {
+		if (dev->flags&IFF_PROMISC)
+			printk(KERN_INFO "device %s still in promiscuous
mode"
+				" (use count now %d)\n", 
+				dev->name, dev->promiscuity);
+	}
 }
 
 /**
diff -ru linux-2.4.20-18.8/net/packet/af_packet.c
linux-2.4.20-18.8-tablus/net/packet/af_packet.c
--- linux-2.4.20-18.8/net/packet/af_packet.c	2003-05-29
03:47:00.000000000 -0700
+++ linux-2.4.20-18.8-tablus/net/packet/af_packet.c	2003-06-30
18:19:47.000000000 -0700
@@ -1146,6 +1146,9 @@
 #ifdef CONFIG_PACKET_MULTICAST
 static void packet_dev_mc(struct net_device *dev, struct packet_mclist *i,
int what)
 {
+	if (!(dev->flags & IFF_UP))
+		return;
+
 	switch (i->type) {
 	case PACKET_MR_MULTICAST:
 		if (what > 0)
@@ -1396,6 +1399,8 @@
 				}
 				spin_unlock(&po->bind_lock);
 			}
+			break;
+		case NETDEV_GOING_DOWN:
 #ifdef CONFIG_PACKET_MULTICAST
 			if (po->mclist)
 				packet_dev_mclist(dev, po->mclist, -1);

[PATCH 2.4.22-bk] dev->promiscuity refcounting broken in af_packet.c

[Jason Lunz]

niz@vencraft.com said:
> A number of people have mentioned that they get a weird situation
> where when they *start* a program that does promiscuous network reads
> (with, say, ‘tcpdump –i eth0’). They then get a kernel message
> “left promiscuous mode” when the program starts and the message
> “entered promiscuous mode” when it exits – the exact opposite of
> what should happen.

Thanks for finding this! This has been happening to me for over a year,
but always so rarely that I never bothered to really track it down.

Your patch isn't really correct, though. Aside from the whitespace
damage, it doesn't really address the problem. Clamping the refcount at
zero only stops the bleeding.

The problem is that packet sockets are calling dev_set_promiscuity too
many times. For example, if I take an unconfigured interface and do:

halfoat:~ # ip link show eth1
9: eth1: <BROADCAST,MULTICAST> mtu 1500 qdisc pfifo_fast qlen 100
    link/ether 00:30:48:41:62:12 brd ff:ff:ff:ff:ff:ff
halfoat:~ # ip link set up eth1
halfoat:~ # tcpdump -i eth1 &
[1] 457
tcpdump: WARNING: eth1: no IPv4 address assigned
tcpdump: listening on eth1
halfoat:~ # ip link set down eth1
tcpdump: pcap_loop: recvfrom: Network is down
[1]+  Exit 1                  tcpdump -i eth1
halfoat:~ # ip link show eth1
9: eth1: <BROADCAST,MULTICAST,PROMISC> mtu 1500 qdisc pfifo_fast qlen 100
    link/ether 00:30:48:41:62:12 brd ff:ff:ff:ff:ff:ff

eth1 is now in promiscuous mode because dev->promiscuity is -1 (!= 0).

When the interface goes down, dev_change_flags calls dev_close, which
sends NETDEV_DOWN down the netdev notifier chain. Because tcpdump has a
packet socket open, packet_notifier calls packet_dev_mclist ->
packet_dev_mc -> dev_set_promiscuity.

When tcpdump gets ENETDOWN, it aborts, closing the packet socket.
af_packet.c's proto_ops->release cleanup method is packet_release.  On
close(), packet_release calls packet_flush_mclist, which again
decrements dev->promiscuity, so when tcpdump exits, dev promiscuity is
left at -1.

I can't see any reason to be mucking about with the device promiscuity
on NETDEV_DOWN and NETDEV_UP events in the first place. The attached
patch seems to fix all the cases I can think of. It works properly in
both of the above cases, and has also been verified to do the right
thing with NETDEV_UNREGISTER events.

Jason


Index: linux-2.4/net/packet/af_packet.c
===================================================================
RCS file: /home/cvs/linux-2.4/net/packet/af_packet.c,v
retrieving revision 1.11
diff -u -p -r1.11 af_packet.c
--- linux-2.4/net/packet/af_packet.c	12 Jun 2002 23:10:34 -0000	1.11
+++ linux-2.4/net/packet/af_packet.c	1 Jul 2003 20:17:51 -0000
@@ -1378,8 +1378,13 @@ static int packet_notifier(struct notifi
 		po = sk->protinfo.af_packet;
 
 		switch (msg) {
-		case NETDEV_DOWN:
 		case NETDEV_UNREGISTER:
+#ifdef CONFIG_PACKET_MULTICAST
+			if (po->mclist)
+				packet_dev_mclist(dev, po->mclist, -1);
+			// fallthrough
+#endif
+		case NETDEV_DOWN:
 			if (dev->ifindex == po->ifindex) {
 				spin_lock(&po->bind_lock);
 				if (po->running) {
@@ -1396,10 +1401,6 @@ static int packet_notifier(struct notifi
 				}
 				spin_unlock(&po->bind_lock);
 			}
-#ifdef CONFIG_PACKET_MULTICAST
-			if (po->mclist)
-				packet_dev_mclist(dev, po->mclist, -1);
-#endif
 			break;
 		case NETDEV_UP:
 			spin_lock(&po->bind_lock);
@@ -1409,10 +1410,6 @@ static int packet_notifier(struct notifi
 				po->running = 1;
 			}
 			spin_unlock(&po->bind_lock);
-#ifdef CONFIG_PACKET_MULTICAST
-			if (po->mclist)
-				packet_dev_mclist(dev, po->mclist, +1);
-#endif
 			break;
 		}
 	}