On Tue, Nov 10, 2020 at 08:47:40 -0800, Jakub Kicinski wrote: > On Tue, 10 Nov 2020 10:28:34 +0100 Guillaume Nault wrote: > > On Mon, Nov 09, 2020 at 03:52:37PM -0800, Jakub Kicinski wrote: > > > On Fri, 6 Nov 2020 18:16:45 +0000 Tom Parkin wrote: > > > > This small RFC series implements a suggestion from Guillaume Nault in > > > > response to my previous submission to add an ac/pppoe driver to the l2tp > > > > subsystem[1]. > > > > > > > > Following Guillaume's advice, this series adds an ioctl to the ppp code > > > > to allow a ppp channel to be bridged to another. Quoting Guillaume: > > > > > > > > "It's just a matter of extending struct channel (in ppp_generic.c) with > > > > a pointer to another channel, then testing this pointer in ppp_input(). > > > > If the pointer is NULL, use the classical path, if not, forward the PPP > > > > frame using the ->start_xmit function of the peer channel." > > > > > > > > This allows userspace to easily take PPP frames from e.g. a PPPoE > > > > session, and forward them over a PPPoL2TP session; accomplishing the > > > > same thing my earlier ac/pppoe driver in l2tp did but in much less > > > > code! > > > > > > I have little understanding of the ppp code, but I can't help but > > > wonder why this special channel connection is needed? We have great > > > many ways to redirect traffic between interfaces - bpf, tc, netfilter, > > > is there anything ppp specific that is required here? > > > > I can see two viable ways to implement this feature. The one described > > in this patch series is the simplest. The reason why it doesn't reuse > > existing infrastructure is because it has to work at the link layer > > (no netfilter) and also has to work on PPP channels (no network > > device). > > > > The alternative, is to implement a virtual network device for the > > protocols we want to support (at least PPPoE and L2TP, maybe PPTP) > > and teach tunnel_key about them. Then we could use iproute2 commands > > like: > > # ip link add name pppoe0 up type pppoe external > > # ip link add name l2tp0 up type l2tp external > > # tc qdisc add dev pppoe0 ingress > > # tc qdisc add dev l2tp0 ingress > > # tc filter add dev pppoe0 ingress matchall \ > > action tunnel_key set l2tp_version 2 l2tp_tid XXX l2tp_sid YYY \ > > action mirred egress redirect dev pppoe0 > > # tc filter add dev l2tp0 ingress matchall \ > > action tunnel_key set pppoe_sid ZZZ \ > > action mirred egress redirect dev l2tp0 > > > > Note: I've used matchall for simplicity, but a real uses case would > > have to filter on the L2TP session and tunnel IDs and on the PPPoE > > session ID. > > > > As I said in my reply to the original thread, I like this idea, but > > haven't thought much about the details. So there might be some road > > blocks. Beyond modernising PPP and making it better integrated into the > > stack, that should also bring the possibility of hardware offload (but > > would any NIC vendor be interested?). > > Integrating with the stack gives you access to all its features, other > types of encap, firewalling, bpf, etc. > > > I think the question is more about long term maintainance. Do we want > > to keep PPP related module self contained, with low maintainance code > > (the current proposal)? Or are we willing to modernise the > > infrastructure, add support and maintain PPP features in other modules > > like flower, tunnel_key, etc.? > > Right, it's really not great to see new IOCTLs being added to drivers, > but the alternative would require easily 50 times more code.
Jakub, could I quickly poll you on your current gut-feel level of opposition to the ioctl-based approach? Guillaume has given good feedback on the RFC code which I can work into an actual patch submission, but I don't really want to if you're totally opposed to the whole idea :-) I appreciate you may want to reserve judgement pending a recap of the ppp subsystem as it stands. Thanks!
signature.asc
Description: PGP signature
