Re: [net-next PATCH v1 7/7] macvlan: add FDB bridge ops and new macvlan mode

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Apr 10, 2012 at 08:29:45AM -0700, John Fastabend wrote:
> 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

OK so with the new flag, you will also disable
the forwarding of ALLMULTI from macvlan to lowerdev?
Fair enough.

-- 
MST