From: John Fastabend <john.r.fastabend@intel•com>
To: "Michael S. Tsirkin" <mst@redhat•com>
Cc: roprabhu@cisco•com, stephen.hemminger@vyatta•com,
davem@davemloft•net, hadi@cyberus•ca, bhutchings@solarflare•com,
jeffrey.t.kirsher@intel•com, netdev@vger•kernel.org,
gregory.v.rose@intel•com, krkumar2@in•ibm.com, sri@us•ibm.com
Subject: Re: [net-next PATCH v1 7/7] macvlan: add FDB bridge ops and new macvlan mode
Date: Tue, 10 Apr 2012 08:29:45 -0700 [thread overview]
Message-ID: <4F8451E9.5060805@intel.com> (raw)
In-Reply-To: <20120410143306.GC19556@redhat.com>
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
next prev parent reply other threads:[~2012-04-10 15:29 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-04-09 22:00 [net-next PATCH v1 0/7] Managing the forwarding database(FDB) John Fastabend
2012-04-09 22:00 ` [net-next PATCH v1 1/7] net: add generic PF_BRIDGE:RTM_ FDB hooks John Fastabend
2012-04-11 3:23 ` Ben Hutchings
2012-04-11 14:45 ` John Fastabend
2012-04-11 16:05 ` Ben Hutchings
2012-04-11 17:22 ` John Fastabend
2012-04-09 22:00 ` [net-next PATCH v1 2/7] net: addr_list: add exclusive dev_uc_add and dev_mc_add John Fastabend
2012-04-10 8:03 ` Michael S. Tsirkin
2012-04-11 3:33 ` Ben Hutchings
2012-04-11 14:46 ` John Fastabend
2012-04-09 22:00 ` [net-next PATCH v1 3/7] net: add fdb generic dump routine John Fastabend
2012-04-11 3:45 ` Ben Hutchings
2012-04-11 14:46 ` John Fastabend
2012-04-09 22:00 ` [net-next PATCH v1 4/7] ixgbe: enable FDB netdevice ops John Fastabend
2012-04-09 22:00 ` [net-next PATCH v1 5/7] ixgbe: allow RAR table to be updated in promisc mode John Fastabend
2012-04-09 22:00 ` [net-next PATCH v1 6/7] ixgbe: UTA table incorrectly programmed John Fastabend
2012-04-09 22:00 ` [net-next PATCH v1 7/7] macvlan: add FDB bridge ops and new macvlan mode John Fastabend
2012-04-10 8:09 ` Michael S. Tsirkin
2012-04-10 8:14 ` Michael S. Tsirkin
2012-04-10 13:50 ` John Fastabend
2012-04-10 14:33 ` Michael S. Tsirkin
2012-04-10 15:29 ` John Fastabend [this message]
2012-04-10 15:32 ` Michael S. Tsirkin
2012-04-10 13:27 ` John Fastabend
2012-04-10 13:43 ` Michael S. Tsirkin
2012-04-10 14:25 ` John Fastabend
2012-04-10 14:35 ` Michael S. Tsirkin
2012-04-10 15:26 ` John Fastabend
2012-04-10 15:30 ` Michael S. Tsirkin
2012-04-10 15:35 ` John Fastabend
2012-04-11 0:46 ` Sridhar Samudrala
2012-04-11 1:42 ` John Fastabend
2012-04-11 8:02 ` Michael S. Tsirkin
2012-04-11 14:32 ` John Fastabend
2012-04-09 22:15 ` [net-next PATCH v1 0/7] Managing the forwarding database(FDB) Stephen Hemminger
2012-04-09 22:32 ` John Fastabend
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4F8451E9.5060805@intel.com \
--to=john.r.fastabend@intel$(echo .)com \
--cc=bhutchings@solarflare$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=gregory.v.rose@intel$(echo .)com \
--cc=hadi@cyberus$(echo .)ca \
--cc=jeffrey.t.kirsher@intel$(echo .)com \
--cc=krkumar2@in$(echo .)ibm.com \
--cc=mst@redhat$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=roprabhu@cisco$(echo .)com \
--cc=sri@us$(echo .)ibm.com \
--cc=stephen.hemminger@vyatta$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox