On 4/10/2012 7:33 AM, Michael S. Tsirkin wrote: > On Tue, Apr 10, 2012 at 06:50:42AM -0700, John Fastabend wrote: >> On 4/10/2012 1:14 AM, Michael S. Tsirkin wrote: >>> On Tue, Apr 10, 2012 at 11:09:16AM +0300, Michael S. Tsirkin wrote: >>>> On Mon, Apr 09, 2012 at 03:00:54PM -0700, John Fastabend wrote: >>>>> This adds a new macvlan mode MACVLAN_PASSTHRU_NOPROMISC >>>>> this mode acts the same as the original passthru mode _except_ >>>>> it does not set promiscuous mode on the lowerdev. Because the >>>>> lowerdev is not put in promiscuous mode any unicast or multicast >>>>> addresses the device should receive must be explicitely added >>>>> with the FDB bridge ops. In many use cases the management stack >>>>> will know the mac addresses needed (maybe negotiated via EVB/VDP) >>>>> or may require only receiving known "good" mac addresses. This >>>>> mode with the FDB ops supports this usage model. >>>> >>>> >>>> Looks good to me. Some questions below: >>>> >>>>> This patch is a result of Roopa Prabhu's work. Follow up >>>>> patches are needed for VEPA and VEB macvlan modes. >>>> >>>> And bridge too? >>>> >>>> Also, my understanding is that other modes won't need a flag >>>> like this since they don't put the device in promisc mode initially, >>>> so no assumptions are broken if we require all addresses >>>> to be declared, right? >>>> >>>> A final question: I think we'll later add a macvlan mode >>>> that does not flood all multicasts. This would change behaviour >>>> in an incompatible way so we'll probably need yet another >>>> flag. Would it make sense to combine this functionality >>>> with nopromisc so we have less modes to support? >>> >>> One other question I forgot: >>> >> >> [...] >> >>>>> >>>>> @@ -344,12 +346,15 @@ static int macvlan_stop(struct net_device *dev) >>>>> struct macvlan_dev *vlan = netdev_priv(dev); >>>>> struct net_device *lowerdev = vlan->lowerdev; >>>>> >>>>> + dev_uc_unsync(lowerdev, dev); >>>>> + dev_mc_unsync(lowerdev, dev); >>>>> + >>>>> if (vlan->port->passthru) { >>>>> - dev_set_promiscuity(lowerdev, -1); >>>>> + if (vlan->mode == MACVLAN_MODE_PASSTHRU) >>>>> + dev_set_promiscuity(lowerdev, 1); >>>>> goto hash_del; >>>>> } >>>>> >>>>> - dev_mc_unsync(lowerdev, dev); >>>>> if (dev->flags & IFF_ALLMULTI) >>>>> dev_set_allmulti(lowerdev, -1); >>>>> >>>>> @@ -399,10 +404,11 @@ static void macvlan_change_rx_flags(struct net_device *dev, int change) >>>>> dev_set_allmulti(lowerdev, dev->flags & IFF_ALLMULTI ? 1 : -1); >>> >>> In the new mode, do we want to have promisc on lowerdev follow whatever >>> is set on the macvlan, like we do for allmulti? >>> I'm not sure at this point - what do others think? >>> >> >> Just to enumerate why you would need this: (1) socket set with >> PACKET_MR_MULTICAST and (2) something like mrouted is running >> on the macvlan (3) maybe some case I missed? >> >> Don't you need CAP_NET_RAW to set these though anyways? So I >> wouldn't think it would be a problem. I assume if a user has >> CAP_NET_RAW or UUID 0 they really should be able to set this >> up. >> >> .John > > I am not sure, really. > But I note that with a security mechanism such as selinux, CAP_NET_RAW > might be insufficient to change the underlying device. > So there might be value in being able to change it in > a controlled manner through macvlan. > > There's also something to be said for being able to let > management deal with macvlan devices (and there are > some very complex tools for that around) while > keeping a simple script around for the physical > one and knowing that they won't disrupt each other. > If people really _need_/_want_ this then I guess we can add another flag. I don't think we should to tie this into the FDB bits creating an interface with strange side effects is probably a poor design. Much better IMHO to have an explicit bit if and when this is needed. .John -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html