* [PATCH] Fix infinite loop on dev_mc_unsync() @ 2007-11-09 15:11 ` Luis R. Rodriguez 0 siblings, 0 replies; 15+ messages in thread From: Luis R. Rodriguez @ 2007-11-09 15:11 UTC (permalink / raw) To: netdev; +Cc: linux-wireless, Jeff Garzik While reviewing net/core/dev_mcast.c I found what I think is an infinite loop on dev_mc_unsync(). This fixes it. We make use of this guy on mac80211 in ieee80211_stop(). This is untested. Signed-off-by: Luis R. Rodriguez <mcgrof@winlab.rutgers.edu> diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c index 15241cf..5373c03 100644 --- a/net/core/dev_mcast.c +++ b/net/core/dev_mcast.c @@ -168,8 +168,10 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from) da = from->mc_list; while (da != NULL) { next = da->next; - if (!da->da_synced) + if (!da->da_synced) { + da = next; continue; + } __dev_addr_delete(&to->mc_list, &to->mc_count, da->da_addr, da->da_addrlen, 0); da->da_synced = 0; ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH] Fix infinite loop on dev_mc_unsync() @ 2007-11-09 15:11 ` Luis R. Rodriguez 0 siblings, 0 replies; 15+ messages in thread From: Luis R. Rodriguez @ 2007-11-09 15:11 UTC (permalink / raw) To: netdev-u79uwXL29TY76Z2rM5mHXA Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA, Jeff Garzik While reviewing net/core/dev_mcast.c I found what I think is an infinite loop on dev_mc_unsync(). This fixes it. We make use of this guy on mac80211 in ieee80211_stop(). This is untested. Signed-off-by: Luis R. Rodriguez <mcgrof-NvJAj8134tRxEa0u/P/EHDe48wsgrGvP@public.gmane.org> diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c index 15241cf..5373c03 100644 --- a/net/core/dev_mcast.c +++ b/net/core/dev_mcast.c @@ -168,8 +168,10 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from) da = from->mc_list; while (da != NULL) { next = da->next; - if (!da->da_synced) + if (!da->da_synced) { + da = next; continue; + } __dev_addr_delete(&to->mc_list, &to->mc_count, da->da_addr, da->da_addrlen, 0); da->da_synced = 0; ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix infinite loop on dev_mc_unsync() 2007-11-09 15:11 ` Luis R. Rodriguez @ 2007-11-09 18:51 ` Luis R. Rodriguez -1 siblings, 0 replies; 15+ messages in thread From: Luis R. Rodriguez @ 2007-11-09 18:51 UTC (permalink / raw) To: netdev; +Cc: linux-wireless, Jeff Garzik, David Miller Sorry, forgot to CC David. On Fri, Nov 09, 2007 at 10:11:35AM -0500, Luis R. Rodriguez wrote: While reviewing net/core/dev_mcast.c I found what I think is an infinite loop on dev_mc_unsync(). This fixes it. We make use of this guy on mac80211 in ieee80211_stop(). This is untested. Signed-off-by: Luis R. Rodriguez <mcgrof@winlab.rutgers.edu> diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c index 15241cf..5373c03 100644 --- a/net/core/dev_mcast.c +++ b/net/core/dev_mcast.c @@ -168,8 +168,10 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from) da = from->mc_list; while (da != NULL) { next = da->next; - if (!da->da_synced) + if (!da->da_synced) { + da = next; continue; + } __dev_addr_delete(&to->mc_list, &to->mc_count, da->da_addr, da->da_addrlen, 0); da->da_synced = 0; ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix infinite loop on dev_mc_unsync() @ 2007-11-09 18:51 ` Luis R. Rodriguez 0 siblings, 0 replies; 15+ messages in thread From: Luis R. Rodriguez @ 2007-11-09 18:51 UTC (permalink / raw) To: netdev-u79uwXL29TY76Z2rM5mHXA Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA, Jeff Garzik, David Miller Sorry, forgot to CC David. On Fri, Nov 09, 2007 at 10:11:35AM -0500, Luis R. Rodriguez wrote: While reviewing net/core/dev_mcast.c I found what I think is an infinite loop on dev_mc_unsync(). This fixes it. We make use of this guy on mac80211 in ieee80211_stop(). This is untested. Signed-off-by: Luis R. Rodriguez <mcgrof-NvJAj8134tRxEa0u/P/EHDe48wsgrGvP@public.gmane.org> diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c index 15241cf..5373c03 100644 --- a/net/core/dev_mcast.c +++ b/net/core/dev_mcast.c @@ -168,8 +168,10 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from) da = from->mc_list; while (da != NULL) { next = da->next; - if (!da->da_synced) + if (!da->da_synced) { + da = next; continue; + } __dev_addr_delete(&to->mc_list, &to->mc_count, da->da_addr, da->da_addrlen, 0); da->da_synced = 0; ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix infinite loop on dev_mc_unsync() 2007-11-09 18:51 ` Luis R. Rodriguez @ 2007-11-09 19:07 ` Joe Perches -1 siblings, 0 replies; 15+ messages in thread From: Joe Perches @ 2007-11-09 19:07 UTC (permalink / raw) To: Luis R. Rodriguez; +Cc: netdev, linux-wireless, Jeff Garzik, David Miller On Fri, 2007-11-09 at 13:51 -0500, Luis R. Rodriguez wrote: > While reviewing net/core/dev_mcast.c I found what I think is an > infinite loop on dev_mc_unsync(). This fixes it. We make use of > this guy on mac80211 in ieee80211_stop(). This is untested. > > Signed-off-by: Luis R. Rodriguez <mcgrof@winlab.rutgers.edu> > > diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c > index 15241cf..5373c03 100644 > --- a/net/core/dev_mcast.c > +++ b/net/core/dev_mcast.c > @@ -168,8 +168,10 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from) > da = from->mc_list; > while (da != NULL) { > next = da->next; > - if (!da->da_synced) > + if (!da->da_synced) { > + da = next; > continue; > + } > __dev_addr_delete(&to->mc_list, &to->mc_count, > da->da_addr, da->da_addrlen, 0); > da->da_synced = 0; > - > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Perhaps this is clearer as: void dev_mc_unsync(struct net_device *to, struct net_device *from) { struct dev_addr_list *da; netif_tx_lock_bh(from); netif_tx_lock_bh(to); da = from->mc_list; while (da) { if (da->da_synced) { __dev_addr_delete(&to->mc_list, &to->mc_count, da->da_addr, da->da_addrlen, 0); __dev_addr_delete(&from->mc_list, &from->mc_count, da->da_addr, da->da_addrlen, 0); da->da_synced = 0; } da = da->next; } __dev_set_rx_mode(to); netif_tx_unlock_bh(to); netif_tx_unlock_bh(from); } EXPORT_SYMBOL(dev_mc_unsync); ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix infinite loop on dev_mc_unsync() @ 2007-11-09 19:07 ` Joe Perches 0 siblings, 0 replies; 15+ messages in thread From: Joe Perches @ 2007-11-09 19:07 UTC (permalink / raw) To: Luis R. Rodriguez Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-wireless-u79uwXL29TY76Z2rM5mHXA, Jeff Garzik, David Miller On Fri, 2007-11-09 at 13:51 -0500, Luis R. Rodriguez wrote: > While reviewing net/core/dev_mcast.c I found what I think is an > infinite loop on dev_mc_unsync(). This fixes it. We make use of > this guy on mac80211 in ieee80211_stop(). This is untested. > > Signed-off-by: Luis R. Rodriguez <mcgrof-NvJAj8134tRxEa0u/P/EHDe48wsgrGvP@public.gmane.org> > > diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c > index 15241cf..5373c03 100644 > --- a/net/core/dev_mcast.c > +++ b/net/core/dev_mcast.c > @@ -168,8 +168,10 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from) > da = from->mc_list; > while (da != NULL) { > next = da->next; > - if (!da->da_synced) > + if (!da->da_synced) { > + da = next; > continue; > + } > __dev_addr_delete(&to->mc_list, &to->mc_count, > da->da_addr, da->da_addrlen, 0); > da->da_synced = 0; > - > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Perhaps this is clearer as: void dev_mc_unsync(struct net_device *to, struct net_device *from) { struct dev_addr_list *da; netif_tx_lock_bh(from); netif_tx_lock_bh(to); da = from->mc_list; while (da) { if (da->da_synced) { __dev_addr_delete(&to->mc_list, &to->mc_count, da->da_addr, da->da_addrlen, 0); __dev_addr_delete(&from->mc_list, &from->mc_count, da->da_addr, da->da_addrlen, 0); da->da_synced = 0; } da = da->next; } __dev_set_rx_mode(to); netif_tx_unlock_bh(to); netif_tx_unlock_bh(from); } EXPORT_SYMBOL(dev_mc_unsync); ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix infinite loop on dev_mc_unsync() 2007-11-09 19:07 ` Joe Perches @ 2007-11-09 19:21 ` Luis R. Rodriguez -1 siblings, 0 replies; 15+ messages in thread From: Luis R. Rodriguez @ 2007-11-09 19:21 UTC (permalink / raw) To: Joe Perches; +Cc: netdev, linux-wireless, Jeff Garzik, David Miller On Fri, Nov 09, 2007 at 11:07:16AM -0800, Joe Perches wrote: > On Fri, 2007-11-09 at 13:51 -0500, Luis R. Rodriguez wrote: > > While reviewing net/core/dev_mcast.c I found what I think is an > > infinite loop on dev_mc_unsync(). This fixes it. We make use of > > this guy on mac80211 in ieee80211_stop(). This is untested. > > > > Signed-off-by: Luis R. Rodriguez <mcgrof@winlab.rutgers.edu> > > > > diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c > > index 15241cf..5373c03 100644 > > --- a/net/core/dev_mcast.c > > +++ b/net/core/dev_mcast.c > > @@ -168,8 +168,10 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from) > > da = from->mc_list; > > while (da != NULL) { > > next = da->next; > > - if (!da->da_synced) > > + if (!da->da_synced) { > > + da = next; > > continue; > > + } > > __dev_addr_delete(&to->mc_list, &to->mc_count, > > da->da_addr, da->da_addrlen, 0); > > da->da_synced = 0; > > - > > To unsubscribe from this list: send the line "unsubscribe netdev" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Perhaps this is clearer as: > > void dev_mc_unsync(struct net_device *to, struct net_device *from) > { > struct dev_addr_list *da; > > netif_tx_lock_bh(from); > netif_tx_lock_bh(to); > > da = from->mc_list; > while (da) { > if (da->da_synced) { > __dev_addr_delete(&to->mc_list, &to->mc_count, > da->da_addr, da->da_addrlen, 0); > __dev_addr_delete(&from->mc_list, &from->mc_count, > da->da_addr, da->da_addrlen, 0); > da->da_synced = 0; > } > da = da->next; > } > > __dev_set_rx_mode(to); > > netif_tx_unlock_bh(to); > netif_tx_unlock_bh(from); > } > EXPORT_SYMBOL(dev_mc_unsync); Sure, or better with a for loop and do away with next pointer then: void dev_mc_unsync(struct net_device *to, struct net_device *from) { struct dev_addr_list *da; netif_tx_lock_bh(from); netif_tx_lock_bh(to); for (da = from->mc_list; da; da = da->next) { if (!da->da_synced) continue; __dev_addr_delete(&to->mc_list, &to->mc_count, da->da_addr, da->da_addrlen, 0); da->da_synced = 0; __dev_addr_delete(&from->mc_list, &from->mc_count, da->da_addr, da->da_addrlen, 0); } __dev_set_rx_mode(to); netif_tx_unlock_bh(to); netif_tx_unlock_bh(from); } EXPORT_SYMBOL(dev_mc_unsync); Patch below. Signed-off-by: Luis R. Rodriguez <mcgrof@winlab.rutgers.edu> diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c index 15241cf..2aea8e1 100644 --- a/net/core/dev_mcast.c +++ b/net/core/dev_mcast.c @@ -160,14 +160,12 @@ EXPORT_SYMBOL(dev_mc_sync); */ void dev_mc_unsync(struct net_device *to, struct net_device *from) { - struct dev_addr_list *da, *next; + struct dev_addr_list *da; netif_tx_lock_bh(from); netif_tx_lock_bh(to); - da = from->mc_list; - while (da != NULL) { - next = da->next; + for (da = from->mc_list; da; da = da->next) { if (!da->da_synced) continue; __dev_addr_delete(&to->mc_list, &to->mc_count, @@ -175,7 +173,6 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from) da->da_synced = 0; __dev_addr_delete(&from->mc_list, &from->mc_count, da->da_addr, da->da_addrlen, 0); - da = next; } __dev_set_rx_mode(to); ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix infinite loop on dev_mc_unsync() @ 2007-11-09 19:21 ` Luis R. Rodriguez 0 siblings, 0 replies; 15+ messages in thread From: Luis R. Rodriguez @ 2007-11-09 19:21 UTC (permalink / raw) To: Joe Perches Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-wireless-u79uwXL29TY76Z2rM5mHXA, Jeff Garzik, David Miller On Fri, Nov 09, 2007 at 11:07:16AM -0800, Joe Perches wrote: > On Fri, 2007-11-09 at 13:51 -0500, Luis R. Rodriguez wrote: > > While reviewing net/core/dev_mcast.c I found what I think is an > > infinite loop on dev_mc_unsync(). This fixes it. We make use of > > this guy on mac80211 in ieee80211_stop(). This is untested. > > > > Signed-off-by: Luis R. Rodriguez <mcgrof-NvJAj8134tRxEa0u/P/EHDe48wsgrGvP@public.gmane.org> > > > > diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c > > index 15241cf..5373c03 100644 > > --- a/net/core/dev_mcast.c > > +++ b/net/core/dev_mcast.c > > @@ -168,8 +168,10 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from) > > da = from->mc_list; > > while (da != NULL) { > > next = da->next; > > - if (!da->da_synced) > > + if (!da->da_synced) { > > + da = next; > > continue; > > + } > > __dev_addr_delete(&to->mc_list, &to->mc_count, > > da->da_addr, da->da_addrlen, 0); > > da->da_synced = 0; > > - > > To unsubscribe from this list: send the line "unsubscribe netdev" in > > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Perhaps this is clearer as: > > void dev_mc_unsync(struct net_device *to, struct net_device *from) > { > struct dev_addr_list *da; > > netif_tx_lock_bh(from); > netif_tx_lock_bh(to); > > da = from->mc_list; > while (da) { > if (da->da_synced) { > __dev_addr_delete(&to->mc_list, &to->mc_count, > da->da_addr, da->da_addrlen, 0); > __dev_addr_delete(&from->mc_list, &from->mc_count, > da->da_addr, da->da_addrlen, 0); > da->da_synced = 0; > } > da = da->next; > } > > __dev_set_rx_mode(to); > > netif_tx_unlock_bh(to); > netif_tx_unlock_bh(from); > } > EXPORT_SYMBOL(dev_mc_unsync); Sure, or better with a for loop and do away with next pointer then: void dev_mc_unsync(struct net_device *to, struct net_device *from) { struct dev_addr_list *da; netif_tx_lock_bh(from); netif_tx_lock_bh(to); for (da = from->mc_list; da; da = da->next) { if (!da->da_synced) continue; __dev_addr_delete(&to->mc_list, &to->mc_count, da->da_addr, da->da_addrlen, 0); da->da_synced = 0; __dev_addr_delete(&from->mc_list, &from->mc_count, da->da_addr, da->da_addrlen, 0); } __dev_set_rx_mode(to); netif_tx_unlock_bh(to); netif_tx_unlock_bh(from); } EXPORT_SYMBOL(dev_mc_unsync); Patch below. Signed-off-by: Luis R. Rodriguez <mcgrof-NvJAj8134tRxEa0u/P/EHDe48wsgrGvP@public.gmane.org> diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c index 15241cf..2aea8e1 100644 --- a/net/core/dev_mcast.c +++ b/net/core/dev_mcast.c @@ -160,14 +160,12 @@ EXPORT_SYMBOL(dev_mc_sync); */ void dev_mc_unsync(struct net_device *to, struct net_device *from) { - struct dev_addr_list *da, *next; + struct dev_addr_list *da; netif_tx_lock_bh(from); netif_tx_lock_bh(to); - da = from->mc_list; - while (da != NULL) { - next = da->next; + for (da = from->mc_list; da; da = da->next) { if (!da->da_synced) continue; __dev_addr_delete(&to->mc_list, &to->mc_count, @@ -175,7 +173,6 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from) da->da_synced = 0; __dev_addr_delete(&from->mc_list, &from->mc_count, da->da_addr, da->da_addrlen, 0); - da = next; } __dev_set_rx_mode(to); ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix infinite loop on dev_mc_unsync() 2007-11-09 19:21 ` Luis R. Rodriguez (?) @ 2007-11-09 23:12 ` Patrick McHardy 2007-11-10 0:08 ` Joe Perches -1 siblings, 1 reply; 15+ messages in thread From: Patrick McHardy @ 2007-11-09 23:12 UTC (permalink / raw) To: Luis R. Rodriguez Cc: Joe Perches, netdev, linux-wireless, Jeff Garzik, David Miller Luis R. Rodriguez wrote: > diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c > index 15241cf..2aea8e1 100644 > --- a/net/core/dev_mcast.c > +++ b/net/core/dev_mcast.c > @@ -160,14 +160,12 @@ EXPORT_SYMBOL(dev_mc_sync); > */ > void dev_mc_unsync(struct net_device *to, struct net_device *from) > { > - struct dev_addr_list *da, *next; > + struct dev_addr_list *da; > > netif_tx_lock_bh(from); > netif_tx_lock_bh(to); > > - da = from->mc_list; > - while (da != NULL) { > - next = da->next; > + for (da = from->mc_list; da; da = da->next) { > This may cause a use-after-free since __dev_addr_delete frees the address when all references are gone. > if (!da->da_synced) > continue; > __dev_addr_delete(&to->mc_list, &to->mc_count, > @@ -175,7 +173,6 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from) > da->da_synced = 0; > __dev_addr_delete(&from->mc_list, &from->mc_count, > da->da_addr, da->da_addrlen, 0); > - da = next; > } > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix infinite loop on dev_mc_unsync() @ 2007-11-10 0:08 ` Joe Perches 0 siblings, 0 replies; 15+ messages in thread From: Joe Perches @ 2007-11-10 0:08 UTC (permalink / raw) To: Patrick McHardy Cc: Luis R. Rodriguez, netdev, linux-wireless, Jeff Garzik, David Miller On Sat, 2007-11-10 at 00:12 +0100, Patrick McHardy wrote: > This may cause a use-after-free since __dev_addr_delete frees the address > when all references are gone. How about a comment then? Perhaps: diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c index ae35405..63576aa 100644 --- a/net/core/dev_mcast.c +++ b/net/core/dev_mcast.c @@ -165,16 +165,23 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from) netif_tx_lock_bh(from); netif_tx_lock_bh(to); + /* + This while loop can't be written as + for (da = from->mc_list; da; da = da->next) + da = from->mc_list and __dev_addr_delete can kfree(from->mc_list) + which could cause a use-after-free of da->next + */ + da = from->mc_list; while (da != NULL) { next = da->next; - if (!da->da_synced) - continue; - __dev_addr_delete(&to->mc_list, &to->mc_count, - da->da_addr, da->da_addrlen, 0); - da->da_synced = 0; - __dev_addr_delete(&from->mc_list, &from->mc_count, - da->da_addr, da->da_addrlen, 0); + if (da->da_synced) { + __dev_addr_delete(&to->mc_list, &to->mc_count, + da->da_addr, da->da_addrlen, 0); + da->da_synced = 0; + __dev_addr_delete(&from->mc_list, &from->mc_count, + da->da_addr, da->da_addrlen, 0); + } da = next; } __dev_set_rx_mode(to); ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix infinite loop on dev_mc_unsync() @ 2007-11-10 0:08 ` Joe Perches 0 siblings, 0 replies; 15+ messages in thread From: Joe Perches @ 2007-11-10 0:08 UTC (permalink / raw) To: Patrick McHardy Cc: Luis R. Rodriguez, netdev-u79uwXL29TY76Z2rM5mHXA, linux-wireless-u79uwXL29TY76Z2rM5mHXA, Jeff Garzik, David Miller On Sat, 2007-11-10 at 00:12 +0100, Patrick McHardy wrote: > This may cause a use-after-free since __dev_addr_delete frees the address > when all references are gone. How about a comment then? Perhaps: diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c index ae35405..63576aa 100644 --- a/net/core/dev_mcast.c +++ b/net/core/dev_mcast.c @@ -165,16 +165,23 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from) netif_tx_lock_bh(from); netif_tx_lock_bh(to); + /* + This while loop can't be written as + for (da = from->mc_list; da; da = da->next) + da = from->mc_list and __dev_addr_delete can kfree(from->mc_list) + which could cause a use-after-free of da->next + */ + da = from->mc_list; while (da != NULL) { next = da->next; - if (!da->da_synced) - continue; - __dev_addr_delete(&to->mc_list, &to->mc_count, - da->da_addr, da->da_addrlen, 0); - da->da_synced = 0; - __dev_addr_delete(&from->mc_list, &from->mc_count, - da->da_addr, da->da_addrlen, 0); + if (da->da_synced) { + __dev_addr_delete(&to->mc_list, &to->mc_count, + da->da_addr, da->da_addrlen, 0); + da->da_synced = 0; + __dev_addr_delete(&from->mc_list, &from->mc_count, + da->da_addr, da->da_addrlen, 0); + } da = next; } __dev_set_rx_mode(to); ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix infinite loop on dev_mc_unsync() 2007-11-10 0:08 ` Joe Perches @ 2007-11-10 0:13 ` Patrick McHardy -1 siblings, 0 replies; 15+ messages in thread From: Patrick McHardy @ 2007-11-10 0:13 UTC (permalink / raw) To: Joe Perches Cc: Luis R. Rodriguez, netdev, linux-wireless, Jeff Garzik, David Miller Joe Perches wrote: > On Sat, 2007-11-10 at 00:12 +0100, Patrick McHardy wrote: > >> This may cause a use-after-free since __dev_addr_delete frees the address >> when all references are gone. >> > > How about a comment then? Perhaps: > > diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c > index ae35405..63576aa 100644 > --- a/net/core/dev_mcast.c > +++ b/net/core/dev_mcast.c > @@ -165,16 +165,23 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from) > netif_tx_lock_bh(from); > netif_tx_lock_bh(to); > > + /* > + This while loop can't be written as > + for (da = from->mc_list; da; da = da->next) > + da = from->mc_list and __dev_addr_delete can kfree(from->mc_list) > + which could cause a use-after-free of da->next > + */ > Seems unnecessary to me, we also don't comment each list_for_each_entry_safe iteration. I consider the use of a seperate next variable self-explanatory. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix infinite loop on dev_mc_unsync() @ 2007-11-10 0:13 ` Patrick McHardy 0 siblings, 0 replies; 15+ messages in thread From: Patrick McHardy @ 2007-11-10 0:13 UTC (permalink / raw) To: Joe Perches Cc: Luis R. Rodriguez, netdev-u79uwXL29TY76Z2rM5mHXA, linux-wireless-u79uwXL29TY76Z2rM5mHXA, Jeff Garzik, David Miller Joe Perches wrote: > On Sat, 2007-11-10 at 00:12 +0100, Patrick McHardy wrote: > >> This may cause a use-after-free since __dev_addr_delete frees the address >> when all references are gone. >> > > How about a comment then? Perhaps: > > diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c > index ae35405..63576aa 100644 > --- a/net/core/dev_mcast.c > +++ b/net/core/dev_mcast.c > @@ -165,16 +165,23 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from) > netif_tx_lock_bh(from); > netif_tx_lock_bh(to); > > + /* > + This while loop can't be written as > + for (da = from->mc_list; da; da = da->next) > + da = from->mc_list and __dev_addr_delete can kfree(from->mc_list) > + which could cause a use-after-free of da->next > + */ > Seems unnecessary to me, we also don't comment each list_for_each_entry_safe iteration. I consider the use of a seperate next variable self-explanatory. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix infinite loop on dev_mc_unsync() @ 2007-11-11 5:34 ` David Miller 0 siblings, 0 replies; 15+ messages in thread From: David Miller @ 2007-11-11 5:34 UTC (permalink / raw) To: kaber; +Cc: joe, mcgrof, netdev, linux-wireless, jeff From: Patrick McHardy <kaber@trash.net> Date: Sat, 10 Nov 2007 01:13:42 +0100 > Joe Perches wrote: > > On Sat, 2007-11-10 at 00:12 +0100, Patrick McHardy wrote: > > > >> This may cause a use-after-free since __dev_addr_delete frees the address > >> when all references are gone. > >> > > > > How about a comment then? Perhaps: > > > > diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c > > index ae35405..63576aa 100644 > > --- a/net/core/dev_mcast.c > > +++ b/net/core/dev_mcast.c > > @@ -165,16 +165,23 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from) > > netif_tx_lock_bh(from); > > netif_tx_lock_bh(to); > > > > + /* > > + This while loop can't be written as > > + for (da = from->mc_list; da; da = da->next) > > + da = from->mc_list and __dev_addr_delete can kfree(from->mc_list) > > + which could cause a use-after-free of da->next > > + */ > > > > Seems unnecessary to me, we also don't comment each list_for_each_entry_safe > iteration. I consider the use of a seperate next variable self-explanatory. Agreed, this comment is pointless. I'll apply Joe's patch without the comment. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix infinite loop on dev_mc_unsync() @ 2007-11-11 5:34 ` David Miller 0 siblings, 0 replies; 15+ messages in thread From: David Miller @ 2007-11-11 5:34 UTC (permalink / raw) To: kaber-dcUjhNyLwpNeoWH0uzbU5w Cc: joe-6d6DIl74uiNBDgjK7y7TUQ, mcgrof-Re5JQEeQqe8AvxtiuMwx3w, netdev-u79uwXL29TY76Z2rM5mHXA, linux-wireless-u79uwXL29TY76Z2rM5mHXA, jeff-o2qLIJkoznsdnm+yROfE0A From: Patrick McHardy <kaber-dcUjhNyLwpNeoWH0uzbU5w@public.gmane.org> Date: Sat, 10 Nov 2007 01:13:42 +0100 > Joe Perches wrote: > > On Sat, 2007-11-10 at 00:12 +0100, Patrick McHardy wrote: > > > >> This may cause a use-after-free since __dev_addr_delete frees the address > >> when all references are gone. > >> > > > > How about a comment then? Perhaps: > > > > diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c > > index ae35405..63576aa 100644 > > --- a/net/core/dev_mcast.c > > +++ b/net/core/dev_mcast.c > > @@ -165,16 +165,23 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from) > > netif_tx_lock_bh(from); > > netif_tx_lock_bh(to); > > > > + /* > > + This while loop can't be written as > > + for (da = from->mc_list; da; da = da->next) > > + da = from->mc_list and __dev_addr_delete can kfree(from->mc_list) > > + which could cause a use-after-free of da->next > > + */ > > > > Seems unnecessary to me, we also don't comment each list_for_each_entry_safe > iteration. I consider the use of a seperate next variable self-explanatory. Agreed, this comment is pointless. I'll apply Joe's patch without the comment. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2007-11-11 5:34 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2007-11-09 15:11 [PATCH] Fix infinite loop on dev_mc_unsync() Luis R. Rodriguez 2007-11-09 15:11 ` Luis R. Rodriguez 2007-11-09 18:51 ` Luis R. Rodriguez 2007-11-09 18:51 ` Luis R. Rodriguez 2007-11-09 19:07 ` Joe Perches 2007-11-09 19:07 ` Joe Perches 2007-11-09 19:21 ` Luis R. Rodriguez 2007-11-09 19:21 ` Luis R. Rodriguez 2007-11-09 23:12 ` Patrick McHardy 2007-11-10 0:08 ` Joe Perches 2007-11-10 0:08 ` Joe Perches 2007-11-10 0:13 ` Patrick McHardy 2007-11-10 0:13 ` Patrick McHardy 2007-11-11 5:34 ` David Miller 2007-11-11 5:34 ` David Miller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.