* [PATCH net-next 8/8] 8021q: Check return of dev_set_promiscuity/allmulti @ 2008-06-20 0:55 Wang Chen 2008-06-20 9:47 ` Patrick McHardy 0 siblings, 1 reply; 6+ messages in thread From: Wang Chen @ 2008-06-20 0:55 UTC (permalink / raw) To: David S. Miller; +Cc: Patrick McHardy, NETDEV dev_set_promiscuity/allmulti might overflow. Commit: "netdevice: Fix promiscuity and allmulti overflow" in net-next makes dev_set_promiscuity/allmulti return error number if overflow happened. Here, we check all positive increment for promiscuity and allmulti to get error return. Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com> --- net/8021q/vlan_dev.c | 19 +++++++++++++++---- 1 files changed, 15 insertions(+), 4 deletions(-) diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c index 5d055c2..1f2669b 100644 --- a/net/8021q/vlan_dev.c +++ b/net/8021q/vlan_dev.c @@ -547,12 +547,23 @@ static int vlan_dev_open(struct net_device *dev) } memcpy(vlan->real_dev_addr, real_dev->dev_addr, ETH_ALEN); - if (dev->flags & IFF_ALLMULTI) - dev_set_allmulti(real_dev, 1); - if (dev->flags & IFF_PROMISC) - dev_set_promiscuity(real_dev, 1); + if (dev->flags & IFF_ALLMULTI) { + err = dev_set_allmulti(real_dev, 1); + if (err < 0) + goto out; + } + if (dev->flags & IFF_PROMISC) { + err = dev_set_promiscuity(real_dev, 1); + if (err < 0) + goto out; + } return 0; + +out: + if (compare_ether_addr(dev->dev_addr, real_dev->dev_addr)) + dev_unicast_delete(real_dev, dev->dev_addr, dev->addr_len); + return err; } static int vlan_dev_stop(struct net_device *dev) -- 1.5.3.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net-next 8/8] 8021q: Check return of dev_set_promiscuity/allmulti 2008-06-20 0:55 [PATCH net-next 8/8] 8021q: Check return of dev_set_promiscuity/allmulti Wang Chen @ 2008-06-20 9:47 ` Patrick McHardy 2008-06-20 10:02 ` Wang Chen 0 siblings, 1 reply; 6+ messages in thread From: Patrick McHardy @ 2008-06-20 9:47 UTC (permalink / raw) To: Wang Chen; +Cc: David S. Miller, NETDEV Wang Chen wrote: > diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c > index 5d055c2..1f2669b 100644 > --- a/net/8021q/vlan_dev.c > +++ b/net/8021q/vlan_dev.c > @@ -547,12 +547,23 @@ static int vlan_dev_open(struct net_device *dev) > } > memcpy(vlan->real_dev_addr, real_dev->dev_addr, ETH_ALEN); > > - if (dev->flags & IFF_ALLMULTI) > - dev_set_allmulti(real_dev, 1); > - if (dev->flags & IFF_PROMISC) > - dev_set_promiscuity(real_dev, 1); > + if (dev->flags & IFF_ALLMULTI) { > + err = dev_set_allmulti(real_dev, 1); > + if (err < 0) > + goto out; > + } > + if (dev->flags & IFF_PROMISC) { > + err = dev_set_promiscuity(real_dev, 1); > + if (err < 0) > + goto out; > + } > > return 0; > + > +out: > + if (compare_ether_addr(dev->dev_addr, real_dev->dev_addr)) > + dev_unicast_delete(real_dev, dev->dev_addr, dev->addr_len); > + return err; This doesn't undo the operations that already succeeded. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next 8/8] 8021q: Check return of dev_set_promiscuity/allmulti 2008-06-20 9:47 ` Patrick McHardy @ 2008-06-20 10:02 ` Wang Chen 2008-06-20 10:08 ` Patrick McHardy 0 siblings, 1 reply; 6+ messages in thread From: Wang Chen @ 2008-06-20 10:02 UTC (permalink / raw) To: Patrick McHardy; +Cc: David S. Miller, NETDEV Patrick McHardy said the following on 2008-6-20 17:47: > Wang Chen wrote: >> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c >> index 5d055c2..1f2669b 100644 >> --- a/net/8021q/vlan_dev.c >> +++ b/net/8021q/vlan_dev.c >> @@ -547,12 +547,23 @@ static int vlan_dev_open(struct net_device *dev) >> } >> memcpy(vlan->real_dev_addr, real_dev->dev_addr, ETH_ALEN); >> >> - if (dev->flags & IFF_ALLMULTI) >> - dev_set_allmulti(real_dev, 1); >> - if (dev->flags & IFF_PROMISC) >> - dev_set_promiscuity(real_dev, 1); >> + if (dev->flags & IFF_ALLMULTI) { >> + err = dev_set_allmulti(real_dev, 1); >> + if (err < 0) >> + goto out; >> + } >> + if (dev->flags & IFF_PROMISC) { >> + err = dev_set_promiscuity(real_dev, 1); >> + if (err < 0) >> + goto out; >> + } >> >> return 0; >> + >> +out: >> + if (compare_ether_addr(dev->dev_addr, real_dev->dev_addr)) >> + dev_unicast_delete(real_dev, dev->dev_addr, dev->addr_len); >> + return err; > > This doesn't undo the operations that already succeeded. > Why? Should I do dev_mc_unsync(real_dev, dev); dev_unicast_unsync(real_dev, dev); before dev_unicast_delete()? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next 8/8] 8021q: Check return of dev_set_promiscuity/allmulti 2008-06-20 10:02 ` Wang Chen @ 2008-06-20 10:08 ` Patrick McHardy 2008-06-20 15:08 ` Wang Chen 0 siblings, 1 reply; 6+ messages in thread From: Patrick McHardy @ 2008-06-20 10:08 UTC (permalink / raw) To: Wang Chen; +Cc: David S. Miller, NETDEV Wang Chen wrote: > Patrick McHardy said the following on 2008-6-20 17:47: >> Wang Chen wrote: >>> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c >>> index 5d055c2..1f2669b 100644 >>> --- a/net/8021q/vlan_dev.c >>> +++ b/net/8021q/vlan_dev.c >>> @@ -547,12 +547,23 @@ static int vlan_dev_open(struct net_device *dev) >>> } >>> memcpy(vlan->real_dev_addr, real_dev->dev_addr, ETH_ALEN); >>> >>> - if (dev->flags & IFF_ALLMULTI) >>> - dev_set_allmulti(real_dev, 1); >>> - if (dev->flags & IFF_PROMISC) >>> - dev_set_promiscuity(real_dev, 1); >>> + if (dev->flags & IFF_ALLMULTI) { >>> + err = dev_set_allmulti(real_dev, 1); >>> + if (err < 0) >>> + goto out; >>> + } >>> + if (dev->flags & IFF_PROMISC) { >>> + err = dev_set_promiscuity(real_dev, 1); >>> + if (err < 0) >>> + goto out; >>> + } >>> >>> return 0; >>> + >>> +out: >>> + if (compare_ether_addr(dev->dev_addr, real_dev->dev_addr)) >>> + dev_unicast_delete(real_dev, dev->dev_addr, dev->addr_len); >>> + return err; >> This doesn't undo the operations that already succeeded. >> > > Why? > Should I do > dev_mc_unsync(real_dev, dev); > dev_unicast_unsync(real_dev, dev); > before dev_unicast_delete()? When returning an error, the state shouldn't change at all, so you need to: - remove unicast address if changed - restore old address in vlan->real_dev_addr - undo dev_set_allmulti ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next 8/8] 8021q: Check return of dev_set_promiscuity/allmulti 2008-06-20 10:08 ` Patrick McHardy @ 2008-06-20 15:08 ` Wang Chen 2008-06-24 11:41 ` Patrick McHardy 0 siblings, 1 reply; 6+ messages in thread From: Wang Chen @ 2008-06-20 15:08 UTC (permalink / raw) To: Patrick McHardy; +Cc: David S. Miller, NETDEV Patrick McHardy said the following on 2008-6-20 18:08: > Wang Chen wrote: >> Patrick McHardy said the following on 2008-6-20 17:47: >>> Wang Chen wrote: >>>> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c >>>> index 5d055c2..1f2669b 100644 >>>> --- a/net/8021q/vlan_dev.c >>>> +++ b/net/8021q/vlan_dev.c >>>> @@ -547,12 +547,23 @@ static int vlan_dev_open(struct net_device *dev) >>>> } >>>> memcpy(vlan->real_dev_addr, real_dev->dev_addr, ETH_ALEN); >>>> >>>> - if (dev->flags & IFF_ALLMULTI) >>>> - dev_set_allmulti(real_dev, 1); >>>> - if (dev->flags & IFF_PROMISC) >>>> - dev_set_promiscuity(real_dev, 1); >>>> + if (dev->flags & IFF_ALLMULTI) { >>>> + err = dev_set_allmulti(real_dev, 1); >>>> + if (err < 0) >>>> + goto out; >>>> + } >>>> + if (dev->flags & IFF_PROMISC) { >>>> + err = dev_set_promiscuity(real_dev, 1); >>>> + if (err < 0) >>>> + goto out; >>>> + } >>>> >>>> return 0; >>>> + >>>> +out: >>>> + if (compare_ether_addr(dev->dev_addr, real_dev->dev_addr)) >>>> + dev_unicast_delete(real_dev, dev->dev_addr, dev->addr_len); >>>> + return err; >>> This doesn't undo the operations that already succeeded. >>> >> >> Why? >> Should I do >> dev_mc_unsync(real_dev, dev); >> dev_unicast_unsync(real_dev, dev); >> before dev_unicast_delete()? > > When returning an error, the state shouldn't change at all, > so you need to: > > - remove unicast address if changed > - restore old address in vlan->real_dev_addr > - undo dev_set_allmulti > Too much cleanup job! Let me think, how about move dev_set_* ahead of memcpy(). Because unwinding dev_set_* is simpler than restoring old real_dev_addr. --- dev_set_promiscuity/allmulti might overflow. Commit: "netdevice: Fix promiscuity and allmulti overflow" in net-next makes dev_set_promiscuity/allmulti return error number if overflow happened. Here, we check all positive increment for promiscuity and allmulti to get error return. Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com> --- net/8021q/vlan_dev.c | 27 +++++++++++++++++++++------ 1 files changed, 21 insertions(+), 6 deletions(-) diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c index 5d055c2..083e0c8 100644 --- a/net/8021q/vlan_dev.c +++ b/net/8021q/vlan_dev.c @@ -543,16 +543,31 @@ static int vlan_dev_open(struct net_device *dev) if (compare_ether_addr(dev->dev_addr, real_dev->dev_addr)) { err = dev_unicast_add(real_dev, dev->dev_addr, ETH_ALEN); if (err < 0) - return err; + goto out; } - memcpy(vlan->real_dev_addr, real_dev->dev_addr, ETH_ALEN); - if (dev->flags & IFF_ALLMULTI) - dev_set_allmulti(real_dev, 1); - if (dev->flags & IFF_PROMISC) - dev_set_promiscuity(real_dev, 1); + if (dev->flags & IFF_ALLMULTI) { + err = dev_set_allmulti(real_dev, 1); + if (err < 0) + goto del_unicast; + } + if (dev->flags & IFF_PROMISC) { + err = dev_set_promiscuity(real_dev, 1); + if (err < 0) + goto clear_allmulti; + } + memcpy(vlan->real_dev_addr, real_dev->dev_addr, ETH_ALEN); return 0; + +clear_allmulti: + if (dev->flags & IFF_ALLMULTI) + dev_set_allmulti(real_dev, -1); +del_unicast: + if (compare_ether_addr(dev->dev_addr, real_dev->dev_addr)) + dev_unicast_delete(real_dev, dev->dev_addr, ETH_ALEN); +out: + return err; } static int vlan_dev_stop(struct net_device *dev) -- 1.5.3.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net-next 8/8] 8021q: Check return of dev_set_promiscuity/allmulti 2008-06-20 15:08 ` Wang Chen @ 2008-06-24 11:41 ` Patrick McHardy 0 siblings, 0 replies; 6+ messages in thread From: Patrick McHardy @ 2008-06-24 11:41 UTC (permalink / raw) To: Wang Chen; +Cc: David S. Miller, NETDEV Wang Chen wrote: > dev_set_promiscuity/allmulti might overflow. > Commit: "netdevice: Fix promiscuity and allmulti overflow" in net-next makes > dev_set_promiscuity/allmulti return error number if overflow happened. > > Here, we check all positive increment for promiscuity and allmulti > to get error return. > > Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com> Acked-by: Patrick McHardy <kaber@trash.net> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2008-06-24 11:41 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-06-20 0:55 [PATCH net-next 8/8] 8021q: Check return of dev_set_promiscuity/allmulti Wang Chen 2008-06-20 9:47 ` Patrick McHardy 2008-06-20 10:02 ` Wang Chen 2008-06-20 10:08 ` Patrick McHardy 2008-06-20 15:08 ` Wang Chen 2008-06-24 11:41 ` Patrick McHardy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).