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]

Message-ID: <20110712142937.GA909183@jupiter.n2.diac24.net>
Date:	Tue, 12 Jul 2011 16:29:38 +0200
From:	David Lamparter <equinox@...c24.net>
To:	Jiri Pirko <jpirko@...hat.com>
Cc:	David Lamparter <equinox@...c24.net>, 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

On Tue, Jul 12, 2011 at 03:20:08PM +0200, Jiri Pirko wrote:
> 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.

I don't see how this is related. I'm talking about the other end of your
bond. Like for example the 802.3ad capable switch you're bonding to.

> >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.

I would actually consider this a regression, if the clashing rx_handler
is the only thing that gets bonding an 'exclusive' hold of the device.

> 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.

Yes, I see what you're trying to do. But if your patch goes back to
allowing broken combinations, I think we need to have those follow-up
patches right here with this patch.

> 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.

Hoping I can convey my point,


-David


P.S.: Could you please provide some sample usage cases for this feature?
--
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