aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorDavid S. Miller <davem@davemloft.net>2008-02-12 00:45:44 -0500
committerDavid S. Miller <davem@davemloft.net>2008-02-12 20:54:17 -0500
commit69cc64d8d92bf852f933e90c888dfff083bd4fc9 (patch)
tree33e3b7c690fc1b0658cd64dac3d8c3ef7e7bb71f
parent3611f4d2a5e0f6135805f88bc5ecb63fa9ee5107 (diff)
[NDISC]: Fix race in generic address resolution
Frank Blaschka provided the bug report and the initial suggested fix for this bug. He also validated this version of this fix. The problem is that the access to neigh->arp_queue is inconsistent, we grab references when dropping the lock lock to call neigh->ops->solicit() but this does not prevent other threads of control from trying to send out that packet at the same time causing corruptions because both code paths believe they have exclusive access to the skb. The best option seems to be to hold the write lock on neigh->lock during the ->solicit() call. I looked at all of the ndisc_ops implementations and this seems workable. The only case that needs special care is the IPV4 ARP implementation of arp_solicit(). It wants to take neigh->lock as a reader to protect the header entry in neigh->ha during the emission of the soliciation. We can simply remove the read lock calls to take care of that since holding the lock as a writer at the caller providers a superset of the protection afforded by the existing read locking. The rest of the ->solicit() implementations don't care whether the neigh is locked or not. Signed-off-by: David S. Miller <davem@davemloft.net>
-rw-r--r--net/core/neighbour.c12
-rw-r--r--net/ipv4/arp.c3
2 files changed, 3 insertions, 12 deletions
diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index a16cf1ec5e5e..7bb6a9a1256d 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -834,18 +834,12 @@ static void neigh_timer_handler(unsigned long arg)
834 } 834 }
835 if (neigh->nud_state & (NUD_INCOMPLETE | NUD_PROBE)) { 835 if (neigh->nud_state & (NUD_INCOMPLETE | NUD_PROBE)) {
836 struct sk_buff *skb = skb_peek(&neigh->arp_queue); 836 struct sk_buff *skb = skb_peek(&neigh->arp_queue);
837 /* keep skb alive even if arp_queue overflows */ 837
838 if (skb)
839 skb_get(skb);
840 write_unlock(&neigh->lock);
841 neigh->ops->solicit(neigh, skb); 838 neigh->ops->solicit(neigh, skb);
842 atomic_inc(&neigh->probes); 839 atomic_inc(&neigh->probes);
843 if (skb)
844 kfree_skb(skb);
845 } else {
846out:
847 write_unlock(&neigh->lock);
848 } 840 }
841out:
842 write_unlock(&neigh->lock);
849 843
850 if (notify) 844 if (notify)
851 neigh_update_notify(neigh); 845 neigh_update_notify(neigh);
diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index 8e17f65f4002..c663fa5339ee 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -368,7 +368,6 @@ static void arp_solicit(struct neighbour *neigh, struct sk_buff *skb)
368 if (!(neigh->nud_state&NUD_VALID)) 368 if (!(neigh->nud_state&NUD_VALID))
369 printk(KERN_DEBUG "trying to ucast probe in NUD_INVALID\n"); 369 printk(KERN_DEBUG "trying to ucast probe in NUD_INVALID\n");
370 dst_ha = neigh->ha; 370 dst_ha = neigh->ha;
371 read_lock_bh(&neigh->lock);
372 } else if ((probes -= neigh->parms->app_probes) < 0) { 371 } else if ((probes -= neigh->parms->app_probes) < 0) {
373#ifdef CONFIG_ARPD 372#ifdef CONFIG_ARPD
374 neigh_app_ns(neigh); 373 neigh_app_ns(neigh);
@@ -378,8 +377,6 @@ static void arp_solicit(struct neighbour *neigh, struct sk_buff *skb)
378 377
379 arp_send(ARPOP_REQUEST, ETH_P_ARP, target, dev, saddr, 378 arp_send(ARPOP_REQUEST, ETH_P_ARP, target, dev, saddr,
380 dst_ha, dev->dev_addr, NULL); 379 dst_ha, dev->dev_addr, NULL);
381 if (dst_ha)
382 read_unlock_bh(&neigh->lock);
383} 380}
384 381
385static int arp_ignore(struct in_device *in_dev, __be32 sip, __be32 tip) 382static int arp_ignore(struct in_device *in_dev, __be32 sip, __be32 tip)