Message-ID: <20201110092834.GA30007@linux.home>
Date:   Tue, 10 Nov 2020 10:28:34 +0100
From:   Guillaume Nault <gnault@...hat.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Tom Parkin <tparkin@...alix.com>, netdev@...r.kernel.org,
        jchapman@...alix.com
Subject: Re: [RFC PATCH 0/2] add ppp_generic ioctl to bridge channels

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

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

Of course, I might have missed other ways to implement this feature.
But that's all I could think of for now.

And if anyone wants a quick recap about PPP (what are these PPP channel
and unit things? what's the relationship between PPPoE, L2TP and PPP?
etc.), just let me know.

Hope this clarifies the situation.

Powered by blists - more mailing lists