| Message ID | 1344537049-11473-1-git-send-email-fbl@redhat.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
>>> On 8/9/2012 at 03:30 PM, Flavio Leitner <fbl@redhat.com> wrote: > It doesn't make any sense to allow the master to become > its slave. That creates a loop of events causing a crash. > > Reported-by: Leonardo Chiquitto <lchiquitto@suse.com> > Signed-off-by: Flavio Leitner <fbl@redhat.com> > --- > drivers/net/bonding/bond_main.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 6fae5f3..5407b44 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -1505,6 +1505,11 @@ int bond_enslave(struct net_device *bond_dev, struct > net_device *slave_dev) > int link_reporting; > int res = 0; > > + if (bond_dev == slave_dev) { > + pr_err("%s: Error: cannot enslave itself.\n", bond_dev->name); > + return -EINVAL; > + } > + > if (!bond->params.use_carrier && slave_dev->ethtool_ops == NULL && > slave_ops->ndo_do_ioctl == NULL) { > pr_warning("%s: Warning: no link monitoring support for %s\n", I've tested it here and confirm it prevents the crash. Tested-by: Leonardo Chiquitto <lchiquitto@suse.com> Thanks, Leonardo -- 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
On Thu, 2012-08-09 at 15:30 -0300, Flavio Leitner wrote: > It doesn't make any sense to allow the master to become > its slave. That creates a loop of events causing a crash. What if there are other intermediate devices, e.g. the slave is a VLAN sub-device of the bond? And doesn't team also have this problem? I think a more general check for such loops might be required. Ben. > Reported-by: Leonardo Chiquitto <lchiquitto@suse.com> > Signed-off-by: Flavio Leitner <fbl@redhat.com> > --- > drivers/net/bonding/bond_main.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 6fae5f3..5407b44 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -1505,6 +1505,11 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev) > int link_reporting; > int res = 0; > > + if (bond_dev == slave_dev) { > + pr_err("%s: Error: cannot enslave itself.\n", bond_dev->name); > + return -EINVAL; > + } > + > if (!bond->params.use_carrier && slave_dev->ethtool_ops == NULL && > slave_ops->ndo_do_ioctl == NULL) { > pr_warning("%s: Warning: no link monitoring support for %s\n",
On Thu, 9 Aug 2012 20:03:23 +0100 Ben Hutchings <bhutchings@solarflare.com> wrote: > On Thu, 2012-08-09 at 15:30 -0300, Flavio Leitner wrote: > > It doesn't make any sense to allow the master to become > > its slave. That creates a loop of events causing a crash. > > What if there are other intermediate devices, e.g. the slave is a VLAN > sub-device of the bond? And doesn't team also have this problem? > > I think a more general check for such loops might be required. Maybe patching netdev_set_master() to fail in the loop case is the way to go. That would work for bonding, team and bridge. What you think? fbl -- 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
Thu, Aug 09, 2012 at 09:03:23PM CEST, bhutchings@solarflare.com wrote: >On Thu, 2012-08-09 at 15:30 -0300, Flavio Leitner wrote: >> It doesn't make any sense to allow the master to become >> its slave. That creates a loop of events causing a crash. > >What if there are other intermediate devices, e.g. the slave is a VLAN >sub-device of the bond? And doesn't team also have this problem? Yes, it does. > >I think a more general check for such loops might be required. I agree. > >Ben. > >> Reported-by: Leonardo Chiquitto <lchiquitto@suse.com> >> Signed-off-by: Flavio Leitner <fbl@redhat.com> >> --- >> drivers/net/bonding/bond_main.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >> index 6fae5f3..5407b44 100644 >> --- a/drivers/net/bonding/bond_main.c >> +++ b/drivers/net/bonding/bond_main.c >> @@ -1505,6 +1505,11 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev) >> int link_reporting; >> int res = 0; >> >> + if (bond_dev == slave_dev) { >> + pr_err("%s: Error: cannot enslave itself.\n", bond_dev->name); >> + return -EINVAL; >> + } >> + >> if (!bond->params.use_carrier && slave_dev->ethtool_ops == NULL && >> slave_ops->ndo_do_ioctl == NULL) { >> pr_warning("%s: Warning: no link monitoring support for %s\n", > >-- >Ben Hutchings, Staff Engineer, Solarflare >Not speaking for my employer; that's the marketing department's job. >They asked us to note that Solarflare product names are trademarked. > -- 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
Thu, Aug 09, 2012 at 09:39:06PM CEST, fbl@redhat.com wrote: >On Thu, 9 Aug 2012 20:03:23 +0100 >Ben Hutchings <bhutchings@solarflare.com> wrote: > >> On Thu, 2012-08-09 at 15:30 -0300, Flavio Leitner wrote: >> > It doesn't make any sense to allow the master to become >> > its slave. That creates a loop of events causing a crash. >> >> What if there are other intermediate devices, e.g. the slave is a VLAN >> sub-device of the bond? And doesn't team also have this problem? >> >> I think a more general check for such loops might be required. > >Maybe patching netdev_set_master() to fail in the loop case is >the way to go. That would work for bonding, team and bridge. > >What you think? How about other devices who do not use "->master" like vlan, macvlan? > >fbl -- 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
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 6fae5f3..5407b44 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -1505,6 +1505,11 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev) int link_reporting; int res = 0; + if (bond_dev == slave_dev) { + pr_err("%s: Error: cannot enslave itself.\n", bond_dev->name); + return -EINVAL; + } + if (!bond->params.use_carrier && slave_dev->ethtool_ops == NULL && slave_ops->ndo_do_ioctl == NULL) { pr_warning("%s: Warning: no link monitoring support for %s\n",
It doesn't make any sense to allow the master to become its slave. That creates a loop of events causing a crash. Reported-by: Leonardo Chiquitto <lchiquitto@suse.com> Signed-off-by: Flavio Leitner <fbl@redhat.com> --- drivers/net/bonding/bond_main.c | 5 +++++ 1 file changed, 5 insertions(+)