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

[Guillaume Nault <gnault@redhat.com>]

On Tue, Nov 10, 2020 at 12:42:24PM +0000, Tom Parkin wrote:
> On  Tue, Nov 10, 2020 at 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.
> > > 
> > > 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.
> 
> One potential downside of this approach is the addition of two virtual
> interfaces for each pppoe->pppol2tp mapping: the concern here
> primarily being the cost of doing so.

No, this is fixed cost. There'd be only one PPPoE interface for
handling all the PPPoE sessions and one for L2TP. These virtual
interfaces wouldn't be specific to a particular session ID. Instead,
the encapsulation information would be attached to the skb and the
virtual PPPoE or L2TP device would build the header based on these
metadata.

> I'm not saying the cost is necessarily prohibitive, but the "bridge the
> channels" approach in the RFC is certainly cheaper.
> 
> Another concern would be the possibility of the virtual devices being
> misconfigured in such a way as to e.g. allow locally generated
> broadcast packets to go out on one of the interfaces.  Possibly this
> would be easy to avoid, I'm not sure.

I'm not too woried about that. A PPPoE or L2TP interface in external
mode couldn't build its header and forward the packet if the skb it
received doesn't have the proper metadat attached to it. So packets
couldn't be inadvertently sent through these interfaces, something
would have to attach the tunnel metadata to the skb beforehand.

But I agree that the setup becomes visible to the administrator, while
the ioctl() approach kept the whole kernel configuration in the hands
of the control-plane implementation. I think that's a good thing
(easier testing and troubleshooting), but yes, that also opens the
possibility for fat finger mistakes.

> > 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.?
> 
> FWIW I would tend to agree.

Yes, it's really about how much we're ready to invest into PPP-related
features.