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

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: <20201110084740.3e3418c0@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Tue, 10 Nov 2020 08:47:40 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Guillaume Nault <gnault@...hat.com>
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 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.
 
> 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.

Some pointers would be appreciated if you don't mind :)