Skip to contentSkip to conversation

2.5.46-bk3: BUG in skbuff.c:178

DISCUSSION Petr Vandrovec 2 participants 5 messages Nov 8, 2002 – Nov 11, 2002

Subsystems (2)
Networking IPv4/IPv6
lore
Share & tools
lore

Petr Vandrovec

author
#0 lore raw
Quoted 6 lines · Andrew Morton
Quoted 3 lines

Single-CPU system, running 2.5.46-bk3. Whiling compiling bk4, and running a script that was pinging every host on my subnet (I was running arp -a to see what was in the arp table at the time), I hit this BUG.

I'd be suspecting the seq_file conversion in arp.c. The read_lock_bh() stuff in there looks, umm, unclear ;)

Yes, see my emails from 23th Oct, 25th Oct (2.5.44: Strange oopses from userspace), from Nov 6th + Nov 7th: Preempt count check when leaving IRQ.

But while yesterday I had no idea, today I have one (it looks like that nobody else is going to fix it for me :-( ) :
seq subsystem can call arp_seq_start/next/stop several times, but state->is_pneigh is set to 0 only once, by memset in arp_seq_open :-(

I think that arp_seq_start should do

{

+   struct arp_iter_state* state = seq->private;
+   seq->is_pneigh = 0;
+   seq->bucket = 0;
    read_lock_bh(&arp_tbl.lock);
    return *pos ? arp_get_bucket(seq, pos) : (void *)1;

}

and we can drop memset from arp_seq_open. I'll try it, and if it will survive my tests, I'll send real patch.

Best regards,
Petr Vandrovec [email protected]

author
#1 lore raw
Quoted 27 lines · Petr Vandrovec
Quoted 6 lines · Andrew Morton
Quoted 3 lines

Single-CPU system, running 2.5.46-bk3. Whiling compiling bk4, and running a script that was pinging every host on my subnet (I was running arp -a to see what was in the arp table at the time), I hit this BUG.

I'd be suspecting the seq_file conversion in arp.c. The read_lock_bh() stuff in there looks, umm, unclear ;)

Yes, see my emails from 23th Oct, 25th Oct (2.5.44: Strange oopses from userspace), from Nov 6th + Nov 7th: Preempt count check when leaving IRQ.

But while yesterday I had no idea, today I have one (it looks like that nobody else is going to fix it for me :-( ) :
seq subsystem can call arp_seq_start/next/stop several times, but state->is_pneigh is set to 0 only once, by memset in arp_seq_open :-(

I think that arp_seq_start should do

  {
(quoted diff) +3 -0
+   struct arp_iter_state* state = seq->private;
+   seq->is_pneigh = 0;
+   seq->bucket = 0;
    read_lock_bh(&arp_tbl.lock);
    return *pos ? arp_get_bucket(seq, pos) : (void *)1;
  }
Quoted 2 lines

and we can drop memset from arp_seq_open. I'll try it, and if it will survive my tests, I'll send real patch.

It was not that trivial: arp was storing current position in three fields:
pos, bucket and is_pneigh - so any code seeking in /proc/net/arp just returned random data, and eventually locked up box...

Patch below removes 'bucket' from arp_iter_state, and merges it to the pos. It is based on assumption that there is no more than 16M of entries in each bucket, and that NEIGH_HASHMASK + 1 + PNEIGH_HASHMASK + 1 < 127 (currently it is 48). As loff_t is 64bit even on i386, there is plenty of space to grow, but it could require apps compiled with O_LARGEFILE, so I decided to use only 31bit space.

I also removed __inline__ from neigh_get_bucket. This way all functions were compiled by gcc-2.95.4 on i386 without local variables on stack...

Because of there is now only one entry in arp_iter_state, it is possible to use seq->private directly instead of allocating memory for arp_iter_state. Also whole lock obtaining in arp_seq_start could be greatly simplified, but I'd like to hear your opinions whether merging pos + bucket together in the way I did is way to go or not, before I'll dig into this more.

I tested code below here: box no more crashes, and I believe that whole arp table is visible in /proc/net/arp.

Best regards,
Petr Vandrovec [email protected]

net/ipv4/arp.c +49 -22
@@ -1139,23 +1139,39 @@
 #endif /* CONFIG_AX25 */
 
 struct arp_iter_state {
-	int is_pneigh, bucket;
+	int is_pneigh;
 };
 
-static __inline__ struct neighbour *neigh_get_bucket(struct seq_file *seq,
+#define ARP_FIRST_NEIGH		(1)
+#define ARP_FIRST_PNEIGH	(ARP_FIRST_NEIGH + NEIGH_HASHMASK + 1)
+
+static inline unsigned int get_arp_pos(loff_t pos, unsigned int* idx) {
+	*idx = pos & 0x00FFFFFF;
+	return pos >> 24;
+}
+
+static inline unsigned int make_arp_pos(unsigned int bucket, unsigned int idx) {
+	return (bucket << 24) | idx;
+}
+
+static inline loff_t next_bucket(loff_t pos) {
+	return (pos + 0x00FFFFFF) & ~0x00FFFFFF;
+}
+
+static struct neighbour *neigh_get_bucket(struct seq_file *seq,
 						     loff_t *pos)
 {
 	struct neighbour *n = NULL;
-	struct arp_iter_state* state = seq->private;
-	loff_t l = *pos;
+	unsigned int l;
+	unsigned int bucket = get_arp_pos(*pos, &l) - ARP_FIRST_NEIGH;
 	int i;
 
-	for (; state->bucket <= NEIGH_HASHMASK; ++state->bucket)
-		for (i = 0, n = arp_tbl.hash_buckets[state->bucket]; n;
+	for (; bucket <= NEIGH_HASHMASK; ++bucket)
+		for (i = 0, n = arp_tbl.hash_buckets[bucket]; n;
 		     ++i, n = n->next)
 			/* Do not confuse users "arp -a" with magic entries */
 			if ((n->nud_state & ~NUD_NOARP) && !l--) {
-				*pos = i;
+				*pos = make_arp_pos(bucket + ARP_FIRST_NEIGH, i);
 				goto out;
 			}
 out:

@@ -1166,15 +1182,15 @@
 							 loff_t *pos)
 {
 	struct pneigh_entry *n = NULL;
-	struct arp_iter_state* state = seq->private;
-	loff_t l = *pos;
+	unsigned int l;
+	unsigned int bucket = get_arp_pos(*pos, &l) - ARP_FIRST_PNEIGH;
 	int i;
 
-	for (; state->bucket <= PNEIGH_HASHMASK; ++state->bucket)
-		for (i = 0, n = arp_tbl.phash_buckets[state->bucket]; n;
+	for (; bucket <= PNEIGH_HASHMASK; ++bucket)
+		for (i = 0, n = arp_tbl.phash_buckets[bucket]; n;
 		     ++i, n = n->next)
 			if (!l--) {
-				*pos = i;
+				*pos = make_arp_pos(bucket + ARP_FIRST_PNEIGH, i);
 				goto out;
 			}
 out:

@@ -1190,8 +1206,7 @@
 
 		read_unlock_bh(&arp_tbl.lock);
 		state->is_pneigh = 1;
-		state->bucket	 = 0;
-		*pos		 = 0;
+		*pos		 = make_arp_pos(ARP_FIRST_PNEIGH, 0);
 		rc = pneigh_get_bucket(seq, pos);
 	}
 	return rc;

@@ -1199,8 +1214,21 @@
 
 static void *arp_seq_start(struct seq_file *seq, loff_t *pos)
 {
+	struct arp_iter_state* state = seq->private;
+	unsigned int idx;
+	unsigned int bucket;
+	
+	state->is_pneigh = 0;
 	read_lock_bh(&arp_tbl.lock);
-	return *pos ? arp_get_bucket(seq, pos) : (void *)1;
+	bucket = get_arp_pos(*pos, &idx);
+	if (bucket < ARP_FIRST_NEIGH)
+		return (void *)1;
+	if (bucket < ARP_FIRST_PNEIGH) {
+		return arp_get_bucket(seq, pos);
+	}
+	read_unlock_bh(&arp_tbl.lock);
+	state->is_pneigh = 1;
+	return pneigh_get_bucket(seq, pos);
 }
 
 static void *arp_seq_next(struct seq_file *seq, void *v, loff_t *pos)

@@ -1209,34 +1237,33 @@
 	struct arp_iter_state* state;
 
 	if (v == (void *)1) {
+		*pos = make_arp_pos(1, 0);
 		rc = arp_get_bucket(seq, pos);
 		goto out;
 	}
-
 	state = seq->private;
 	if (!state->is_pneigh) {
 		struct neighbour *n = v;
 
+		BUG_ON((*pos < make_arp_pos(ARP_FIRST_NEIGH, 0)) || (*pos >= make_arp_pos(ARP_FIRST_PNEIGH, 0)));
 		rc = n = n->next;
 		if (n)
 			goto out;
-		*pos = 0;
-		++state->bucket;
+		*pos = next_bucket(*pos);
 		rc = neigh_get_bucket(seq, pos);
 		if (rc)
 			goto out;
 		read_unlock_bh(&arp_tbl.lock);
 		state->is_pneigh = 1;
-		state->bucket	 = 0;
-		*pos		 = 0;
+		*pos		 = make_arp_pos(ARP_FIRST_PNEIGH, 0);
 		rc = pneigh_get_bucket(seq, pos);
 	} else {
 		struct pneigh_entry *pn = v;
 
+		BUG_ON(*pos < make_arp_pos(ARP_FIRST_PNEIGH, 0));
 		pn = pn->next;
 		if (!pn) {
-			++state->bucket;
-			*pos = 0;
+			*pos = next_bucket(*pos);
 			pn   = pneigh_get_bucket(seq, pos);
 		}
 		rc = pn;
#2 lore raw

Em Fri, Nov 08, 2002 at 11:02:15PM +0100, Petr Vandrovec escreveu:

Quoted 6 lines · Petr Vandrovec
Quoted 1 line · Andrew Morton

Patch below removes 'bucket' from arp_iter_state, and merges it to the pos. It is based on assumption that there is no more than 16M of entries in each bucket, and that NEIGH_HASHMASK + 1 + PNEIGH_HASHMASK + 1 < 127

I did that in the past, but it gets too ugly, see previous changeset in bk tree, lemme see... 1.781.1.52, but anyway, I was aware of this bug but I was on the run, going to Japan and back in 5 days :-\ Well, I have already sent this one to several people, so if you could review/test it...

===== net/ipv4/arp.c 1.13 vs edited =====

net/ipv4/arp.c +99 -64
@@ -1140,67 +1140,122 @@
 
 struct arp_iter_state {
 	int is_pneigh, bucket;
+	union {
+		struct neighbour    *n;
+		struct pneigh_entry *pn;
+	} u;
 };
 
-static __inline__ struct neighbour *neigh_get_bucket(struct seq_file *seq,
-						     loff_t *pos)
+static struct neighbour *neigh_get_first(struct seq_file *seq)
 {
-	struct neighbour *n = NULL;
 	struct arp_iter_state* state = seq->private;
-	loff_t l = *pos;
-	int i;
 
-	for (; state->bucket <= NEIGH_HASHMASK; ++state->bucket)
-		for (i = 0, n = arp_tbl.hash_buckets[state->bucket]; n;
-		     ++i, n = n->next)
-			/* Do not confuse users "arp -a" with magic entries */
-			if ((n->nud_state & ~NUD_NOARP) && !l--) {
-				*pos = i;
-				goto out;
-			}
-out:
-	return n;
+	state->is_pneigh = 0;
+
+	for (state->bucket = 0;
+	     state->bucket <= NEIGH_HASHMASK;
+	     ++state->bucket) {
+		state->u.n = arp_tbl.hash_buckets[state->bucket];
+		while (state->u.n && !(state->u.n->nud_state & ~NUD_NOARP))
+			state->u.n = state->u.n->next;
+		if (state->u.n)
+			break;
+	}
+
+	return state->u.n;
 }
 
-static __inline__ struct pneigh_entry *pneigh_get_bucket(struct seq_file *seq,
-							 loff_t *pos)
+static struct neighbour *neigh_get_next(struct seq_file *seq)
 {
-	struct pneigh_entry *n = NULL;
 	struct arp_iter_state* state = seq->private;
-	loff_t l = *pos;
-	int i;
 
-	for (; state->bucket <= PNEIGH_HASHMASK; ++state->bucket)
-		for (i = 0, n = arp_tbl.phash_buckets[state->bucket]; n;
-		     ++i, n = n->next)
-			if (!l--) {
-				*pos = i;
-				goto out;
-			}
-out:
-	return n;
+	for (; state->bucket <= NEIGH_HASHMASK;
+	     ++state->bucket,
+	     state->u.n = arp_tbl.hash_buckets[state->bucket]) {
+		if (state->u.n)
+			do {
+				state->u.n = state->u.n->next;
+				/* Don't confuse "arp -a" w/ magic entries */
+			} while (state->u.n &&
+				 !(state->u.n->nud_state & ~NUD_NOARP));
+		if (state->u.n)
+			break;
+	}
+
+	return state->u.n;
+}
+
+static loff_t neigh_get_idx(struct seq_file *seq, loff_t pos)
+{
+	neigh_get_first(seq);
+	while (pos && neigh_get_next(seq))
+		--pos;
+	return pos;
+}
+
+static struct pneigh_entry *pneigh_get_first(struct seq_file *seq)
+{
+	struct arp_iter_state* state = seq->private;
+
+	state->is_pneigh = 1;
+
+	for (state->bucket = 0;
+	     state->bucket <= PNEIGH_HASHMASK;
+	     ++state->bucket) {
+		state->u.pn = arp_tbl.phash_buckets[state->bucket];
+		if (state->u.pn)
+			break;
+	}
+	return state->u.pn;
+}
+
+static struct pneigh_entry *pneigh_get_next(struct seq_file *seq)
+{
+	struct arp_iter_state* state = seq->private;
+
+	for (; state->bucket <= PNEIGH_HASHMASK;
+	     ++state->bucket,
+	     state->u.pn = arp_tbl.phash_buckets[state->bucket]) {
+		if (state->u.pn)
+			state->u.pn = state->u.pn->next;
+		
+		if (state->u.pn)
+			break;
+	}
+	return state->u.pn;
 }
 
-static __inline__ void *arp_get_bucket(struct seq_file *seq, loff_t *pos)
+static loff_t pneigh_get_idx(struct seq_file *seq, loff_t pos)
 {
-	void *rc = neigh_get_bucket(seq, pos);
+	pneigh_get_first(seq);
+	while (pos && pneigh_get_next(seq))
+		--pos;
+	return pos;
+}
+
+static void *arp_get_idx(struct seq_file *seq, loff_t pos)
+{
+	struct arp_iter_state* state = seq->private;
+	void *rc;
+	loff_t p;
+
+	read_lock_bh(&arp_tbl.lock);
+	p = neigh_get_idx(seq, pos);
 
-	if (!rc) {
+	if (p || !state->u.n) {
 		struct arp_iter_state* state = seq->private;
 
 		read_unlock_bh(&arp_tbl.lock);
-		state->is_pneigh = 1;
-		state->bucket	 = 0;
-		*pos		 = 0;
-		rc = pneigh_get_bucket(seq, pos);
-	}
+		pneigh_get_idx(seq, p);
+		rc = state->u.pn;
+	} else
+		rc = state->u.n;
 	return rc;
 }
 
 static void *arp_seq_start(struct seq_file *seq, loff_t *pos)
 {
-	read_lock_bh(&arp_tbl.lock);
-	return *pos ? arp_get_bucket(seq, pos) : (void *)1;
+	return *pos ? arp_get_idx(seq, *pos - 1) : (void *)1;
 }
 
 static void *arp_seq_next(struct seq_file *seq, void *v, loff_t *pos)

@@ -1209,38 +1264,19 @@
 	struct arp_iter_state* state;
 
 	if (v == (void *)1) {
-		rc = arp_get_bucket(seq, pos);
+		rc = arp_get_idx(seq, 0);
 		goto out;
 	}
 
 	state = seq->private;
 	if (!state->is_pneigh) {
-		struct neighbour *n = v;
-
-		rc = n = n->next;
-		if (n)
-			goto out;
-		*pos = 0;
-		++state->bucket;
-		rc = neigh_get_bucket(seq, pos);
+		rc = neigh_get_next(seq);
 		if (rc)
 			goto out;
 		read_unlock_bh(&arp_tbl.lock);
-		state->is_pneigh = 1;
-		state->bucket	 = 0;
-		*pos		 = 0;
-		rc = pneigh_get_bucket(seq, pos);
-	} else {
-		struct pneigh_entry *pn = v;
-
-		pn = pn->next;
-		if (!pn) {
-			++state->bucket;
-			*pos = 0;
-			pn   = pneigh_get_bucket(seq, pos);
-		}
-		rc = pn;
-	}
+		rc = pneigh_get_first(seq);
+	} else
+		rc = pneigh_get_next(seq);
 out:
 	++*pos;
 	return rc;

@@ -1291,7 +1327,6 @@
 static __inline__ void arp_format_pneigh_entry(struct seq_file *seq,
 					       struct pneigh_entry *n)
 {
-
 	struct net_device *dev = n->dev;
 	int hatype = dev ? dev->type : 0;
 	char tbuf[16];
author
#3 lore raw
Quoted 4 lines · Arnaldo Carvalho de Melo

Em Fri, Nov 08, 2002 at 11:02:15PM +0100, Petr Vandrovec escreveu:

Quoted 2 lines · Petr Vandrovec
Quoted 1 line · Andrew Morton
Quoted 8 lines
Quoted 3 lines

Patch below removes 'bucket' from arp_iter_state, and merges it to the pos. It is based on assumption that there is no more than 16M of entries in each bucket, and that NEIGH_HASHMASK + 1 + PNEIGH_HASHMASK + 1 < 127

I did that in the past, but it gets too ugly, see previous changeset in bk tree, lemme see... 1.781.1.52, but anyway, I was aware of this bug but I was on the run, going to Japan and back in 5 days :-\ Well, I have already sent this one to several people, so if you could review/test it...

I tried to find how it is supposed to work, and after I tried to boot kernel (at home) with it, I can say that it does not work...

I tried it only at home (where arp table is empty by default), so I did not test whether lock is released properly (if there will be arp_seq_start and arp_seq_stop, with pos == 0 and without intervening arp_seq_next, you'll unlock unlocked arp_tbl.lock in arp_seq_stop (and from what I see in seq_file.c, it can happen), but when I just tried to ping various addresses at vmware's vmnet8, I got very short output of /proc/net/arp, although it should contain couple of entries:

ppc:~# cat /proc/net/arp
IP address       HW type     Flags       HW address            Mask     Device
ppc:~# ping 192.168.27.2
PING 192.168.27.2 (192.168.27.2) 56(84) bytes of data.

--- 192.168.27.2 ping statistics ---
1 packets transmitted, 0 received, 100% packet loss, time 0ms

ppc:~# ping 192.168.27.3
PING 192.168.27.3 (192.168.27.3) 56(84) bytes of data.

--- 192.168.27.3 ping statistics ---
1 packets transmitted, 0 received, 100% packet loss, time 0ms

ppc:~# ping 192.168.27.4
PING 192.168.27.4 (192.168.27.4) 56(84) bytes of data.

--- 192.168.27.4 ping statistics ---
1 packets transmitted, 0 received, 100% packet loss, time 0ms

ppc:~# ping 192.168.27.5
PING 192.168.27.5 (192.168.27.5) 56(84) bytes of data.

--- 192.168.27.5 ping statistics ---
1 packets transmitted, 0 received, 100% packet loss, time 0ms

ppc:~# ping 192.168.27.6
PING 192.168.27.6 (192.168.27.6) 56(84) bytes of data.

--- 192.168.27.6 ping statistics ---
1 packets transmitted, 0 received, 100% packet loss, time 0ms

ppc:~# cat /proc/net/arp
IP address       HW type     Flags       HW address            Mask     Device
192.168.27.2     0x1         0x0         00:00:00:00:00:00     *        vmnet8
ppc:~#

Not something I expect. Before reboot it was listing all 6 addresses, not only first.
Best regards,
Petr Vandrovec [email protected]

#4 lore raw

Em Mon, Nov 11, 2002 at 03:26:02AM +0100, Petr Vandrovec escreveu:

Quoted 9 lines · Arnaldo Carvalho de Melo
Quoted 8 lines

Em Fri, Nov 08, 2002 at 11:02:15PM +0100, Petr Vandrovec escreveu:

Quoted 6 lines · Petr Vandrovec
Quoted 1 line · Andrew Morton

Patch below removes 'bucket' from arp_iter_state, and merges it to the pos. It is based on assumption that there is no more than 16M of entries in each bucket, and that NEIGH_HASHMASK + 1 + PNEIGH_HASHMASK + 1 < 127

Quoted 7 lines
Quoted 4 lines

I did that in the past, but it gets too ugly, see previous changeset in bk tree, lemme see... 1.781.1.52, but anyway, I was aware of this bug but I was on the run, going to Japan and back in 5 days :-\ Well, I have already sent this one to several people, so if you could review/test it...

I tried to find how it is supposed to work, and after I tried to boot kernel (at home) with it, I can say that it does not work...

I'm working on this now... :-\