| Message ID | 20100225084915.GA3171@psychotron.redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Jiri Pirko <jpirko@redhat.com> Date: Thu, 25 Feb 2010 09:49:15 +0100 > Subject: [net-next-2.6 PATCH] net: add addr len check to dev_mc_add > > Signed-off-by: Jiri Pirko <jpirko@redhat.com> Applied. -- 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, Feb 25, 2010 at 09:49:15AM +0100, Jiri Pirko wrote: > Thu, Feb 25, 2010 at 09:00:07AM CET, ogerlitz@voltaire.com wrote: > >Jiri Pirko wrote: > >> +++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c > >> @@ -767,11 +767,8 @@ void ipoib_mcast_dev_flush(struct net_device *dev) > >> -static int ipoib_mcast_addr_is_valid(const u8 *addr, unsigned int addrlen, > >> - const u8 *broadcast) > >> +static int ipoib_mcast_addr_is_valid(const u8 *addr, const u8 *broadcast) > >> { > >> - if (addrlen != INFINIBAND_ALEN) > >> - return 0; > > > >This check was added by commit 5e47596b "IPoIB: Check multicast address format", may I ask what is the reason for removing it now? > > Yes, at this very moment the check is not needless but it will be in a brief > future. dev_mc_add will look very similar like dev_unicast_add. But ok. Here's > patch adding the check in dev_mc_add right now to correct this state. Thanks Or. Is this enough? The problem this statement is trying to solve had to do with bonding creating multicast addresess for ethernet rather than infiniband in some cases. This happens because bonding makes a device that switches from ethernet to infiniband during its lifetime. I'm not quite sure what kind of life cycle those addresses go through, but if they somehow stay on the mc_list then ipoib_mcast_addr_is_valid will still need to have the check. Jason -- 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
Sat, Feb 27, 2010 at 07:01:46AM CET, jgunthorpe@obsidianresearch.com wrote: >On Thu, Feb 25, 2010 at 09:49:15AM +0100, Jiri Pirko wrote: >> Thu, Feb 25, 2010 at 09:00:07AM CET, ogerlitz@voltaire.com wrote: >> >Jiri Pirko wrote: >> >> +++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c >> >> @@ -767,11 +767,8 @@ void ipoib_mcast_dev_flush(struct net_device *dev) >> >> -static int ipoib_mcast_addr_is_valid(const u8 *addr, unsigned int addrlen, >> >> - const u8 *broadcast) >> >> +static int ipoib_mcast_addr_is_valid(const u8 *addr, const u8 *broadcast) >> >> { >> >> - if (addrlen != INFINIBAND_ALEN) >> >> - return 0; >> > >> >This check was added by commit 5e47596b "IPoIB: Check multicast address format", may I ask what is the reason for removing it now? >> >> Yes, at this very moment the check is not needless but it will be in a brief >> future. dev_mc_add will look very similar like dev_unicast_add. But ok. Here's >> patch adding the check in dev_mc_add right now to correct this state. Thanks Or. > >Is this enough? > >The problem this statement is trying to solve had to do with bonding >creating multicast addresess for ethernet rather than infiniband in >some cases. This happens because bonding makes a device that switches >from ethernet to infiniband during its lifetime. I'm not quite >sure what kind of life cycle those addresses go through, but if they >somehow stay on the mc_list then ipoib_mcast_addr_is_valid will still >need to have the check. > Ok I see your point now. But in this case, the length check should be in all drivers because if for example bonding device changes from infiniband back to eth, the addresses stored would be "cut" to 6 bytes and would make no sense. I see two solutions: 1) check the length in all drivers to ensure correctness. 2) when bonding changes it's type, flush mc addresses and start over. Second option looks nicer to me, but I might be missing something, thouths? Thanks Jirka >Jason -- 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
From: Jiri Pirko <jpirko@redhat.com> Date: Sat, 27 Feb 2010 11:38:37 +0100 > I see two solutions: > 1) check the length in all drivers to ensure correctness. > 2) when bonding changes it's type, flush mc addresses and start over. > > Second option looks nicer to me, but I might be missing something, thouths? If bonding is causing this problem, perhaps we should make bonding go through a programmatic interface to do it's work so that the check can be done generically and in a place that will get caught during code changes and audits. -- 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 Sat, Feb 27, 2010 at 11:38:37AM +0100, Jiri Pirko wrote: > >The problem this statement is trying to solve had to do with bonding > >creating multicast addresess for ethernet rather than infiniband in > >some cases. This happens because bonding makes a device that switches > >from ethernet to infiniband during its lifetime. I'm not quite > >sure what kind of life cycle those addresses go through, but if they > >somehow stay on the mc_list then ipoib_mcast_addr_is_valid will still > >need to have the check. > Ok I see your point now. But in this case, the length check should be in all > drivers because if for example bonding device changes from infiniband back to > eth, the addresses stored would be "cut" to 6 bytes and would make no sense. Ideally, but the other wrinkle is due to other problems in the ipoib driver one bad multicast address blocks further addresses from being processed - effectively rendering the interface useless for multicast. Ethernet drivers will just subscribe to a bogus address, no real harm done. I would prefer to see the check remain in ipoib until we can see through testing that it is no longer triggering. > 2) when bonding changes it's type, flush mc addresses and start over. > > Second option looks nicer to me, but I might be missing something, thouths? There was a patch posted that tried to do something like what you are describing: From: Moni Shoua <monis@Voltaire.COM> Subject: Re: [PATCH RESEND] bonding: remap muticast addresses without using dev_close() and dev_open() Thanks, Jason -- 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
Jason Gunthorpe wrote: > Jiri Pirko wrote: >> when bonding changes it's type, flush mc addresses and start over. > There was a patch posted that tried to do something like what you are describing Indeed, Jason, commit 75c785 "bonding: remap muticast addresses without using dev_close() and dev_open()" from Moni Shoua <monis@voltaire.com> does exactly that, isn't it? Or. -- 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
Mon, Mar 01, 2010 at 08:48:09AM CET, ogerlitz@voltaire.com wrote: >Jason Gunthorpe wrote: >> Jiri Pirko wrote: >>> when bonding changes it's type, flush mc addresses and start over. > >> There was a patch posted that tried to do something like what you are describing > >Indeed, Jason, commit 75c785 "bonding: remap muticast addresses without using dev_close() and dev_open()" from Moni Shoua <monis@voltaire.com> does exactly that, isn't it? This affects different mc_list. > >Or. -- 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/net/core/dev_mcast.c b/net/core/dev_mcast.c index 9e2fa39..fd91569 100644 --- a/net/core/dev_mcast.c +++ b/net/core/dev_mcast.c @@ -96,6 +96,8 @@ int dev_mc_add(struct net_device *dev, void *addr, int alen, int glbl) int err; netif_addr_lock_bh(dev); + if (alen != dev->addr_len) + return -EINVAL; err = __dev_addr_add(&dev->mc_list, &dev->mc_count, addr, alen, glbl); if (!err) __dev_set_rx_mode(dev);