netdev - Re: [patch net-next-2.6] net: allow multiple rx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 12 Jul 2011 15:20:08 +0200
From:	Jiri Pirko <jpirko@...hat.com>
To:	David Lamparter <equinox@...c24.net>
Cc:	netdev@...r.kernel.org, davem@...emloft.net,
	shemminger@...ux-foundation.org, kaber@...sh.net, fubar@...ibm.com,
	eric.dumazet@...il.com, nicolas.2p.debian@...il.com,
	andy@...yhouse.net, greearb@...delatech.com, mirqus@...il.com
Subject: Re: [patch net-next-2.6] net: allow multiple rx_handler registration

Tue, Jul 12, 2011 at 01:54:22PM CEST, equinox@...c24.net wrote:
>On Tue, Jul 12, 2011 at 01:06:01PM +0200, Jiri Pirko wrote:
>> For some net topos it is necessary to have multiple "soft-net-devices"
>> hooked on one netdev. For example very common is to have
>> eth<->(br+vlan). Vlan is not using rh_handler (yet) but it might be useful
>> for other setups.
>
>I disagree strongly, especially with the use cases you're enabling in
>this patch.
>
>> +	res = netdev_rx_handler_register(slave_dev, &new_slave->rx_handler,
>> +					 bond_handle_frame,
>> +					 RX_HANDLER_PRIO_BOND);
>
>> +	err = netdev_rx_handler_register(dev, &port->rx_handler,
>> +					 macvlan_handle_frame,
>> +					 RX_HANDLER_PRIO_MACVLAN);
>
>> +	err = netdev_rx_handler_register(dev, &p->rx_handler, br_handle_frame,
>> +					 RX_HANDLER_PRIO_BRIDGE);
>
>> +enum rx_handler_prio {
>> +	RX_HANDLER_PRIO_BRIDGE,
>> +	RX_HANDLER_PRIO_BOND,
>> +	RX_HANDLER_PRIO_MACVLAN,
>> +};
>
>These are all incompatible with each other to a varying degree and/or
>don't make much sense. Let's look at them:
>
>a) a device simultaneously being a bridge member and a bond slave
> -> Fully incompatible. Your bonding peer switch will start sending
>    the bridge's packets on other bond member devices.

Not possible. See netdev_set_master(). Anyway, before rx_handler was
introduced, this was possible and no one cared.

>
>b) a device having macvlans and being a bond slave
> -> Fully incompatible. Same as above, packets to the macvlan will end
>    up on other bond member devices.
>
>c) bridge + macvlan
> -> Mostly useless. Add veth/tap devices to your bridge... as a bonus
>    you get a proper MAC table.
>
>This at least needs bonding support removed since bonding is essentially
>incompatible with anything else w/ the same reasoning as above. Bonds
>are as low-level as Pause frames. Never ever touch individual bond
>slaves.
>
>What does make sense is a device being member of multiple bridges, with
>ebtables as solicitor for which bridge gets the packet. But that's not
>possible with your patch...
>+       if (netdev_rx_handler_get_by_prio(dev, prio))
>                return -EBUSY;
>
>I think your idea is good, but it needs WAY more proper consideration.

This patch doen't introduce anything new which wasn't possible before
rx_handler times. Anyway removing bond from using rx_handler as you
suggested pushes us back.

The rationale of this patch is to have all in one place, clean
architecture. The rest of problems, like what can be
used with what in one time etc can be easily sorted out by follow-up
patches.

And to your idea about multi-bridge support, br co needs to be
adjusted as well. And in relation with PRIO, my idea (inspired from RFC
of this patch comments) is to allow users to change priorities
dynamically from userspace. Also then it could be a range of prios for
bridge for example.

Hope that I cleared that out for you.

Jirka

>
>
>-David
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html