Re: [RFC PATCH 0/2] add ppp_generic ioctl to bridge channels

[Tom Parkin <tparkin@katalix.com>]

[-- Attachment #1: Type: text/plain, Size: 4075 bytes --]

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!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]