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

[John Fastabend <john.r.fastabend@intel.com>]

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