[PATCH] PPPoL2TP: Add more code snippets

[PATCH] PPPoL2TP: Add more code snippets

[Samuel Thibault]

The existing documentation was not telling that one has to create a PPP
channel and a PPP interface to get PPPoL2TP data offloading working.

Also, tunnel switching was not described, so that people were thinking
it was not supported, while it actually is.

Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

---
 Documentation/networking/l2tp.rst |   59 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 56 insertions(+), 3 deletions(-)

--- a/Documentation/networking/l2tp.rst
+++ b/Documentation/networking/l2tp.rst
@@ -387,11 +387,12 @@ Sample userspace code:
   - Create session PPPoX data socket::
 
         struct sockaddr_pppol2tp sax;
-        int fd;
+        int ret;
 
         /* Note, the tunnel socket must be bound already, else it
          * will not be ready
          */
+        int session_fd = socket(AF_PPPOX, SOCK_DGRAM, PX_PROTO_OL2TP);
         sax.sa_family = AF_PPPOX;
         sax.sa_protocol = PX_PROTO_OL2TP;
         sax.pppol2tp.fd = tunnel_fd;
@@ -406,12 +407,64 @@ Sample userspace code:
         /* session_fd is the fd of the session's PPPoL2TP socket.
          * tunnel_fd is the fd of the tunnel UDP / L2TPIP socket.
          */
-        fd = connect(session_fd, (struct sockaddr *)&sax, sizeof(sax));
-        if (fd < 0 ) {
+        ret = connect(session_fd, (struct sockaddr *)&sax, sizeof(sax));
+        if (ret < 0 ) {
                 return -errno;
         }
         return 0;
 
+  - Create PPP channel::
+
+        int chindx;
+        ret = ioctl(session_fd, PPPIOCGCHAN, &chindx);
+        if (ret < 0)
+                return -errno;
+
+        int ppp_chan_fd = open("/dev/ppp", O_RDWR);
+
+        ret = ioctl(ppp_chan_fd, PPPIOCATTCHAN, &chindx);
+        if (ret < 0)
+                return -errno;
+
+Non-data PPP frames will be available for read on `ppp_chan_fd`.
+
+  - Create PPP interface::
+
+        int ppp_if_fd = open("/dev/ppp", O_RDWR);
+
+        int ifunit;
+        ret = ioctl(ppp_if_fd, PPPIOCNEWUNIT, &ifunit);
+        if (ret < 0)
+                return -errno;
+
+        ret = ioctl(ppp_chan_fd, PPPIOCCONNECT, ifunit);
+        if (ret < 0)
+                return -errno;
+
+The ppp<ifunit> interface can then be configured as usual with SIOCSIFMTU,
+SIOCSIFADDR, SIOCSIFDSTADDR, SIOCSIFNETMASK, and activated by setting IFF_UP
+with SIOCSIFFLAGS
+
+  - Tunnel switching is supported by bridging channels::
+
+        int chindx;
+        ret = ioctl(session_fd, PPPIOCGCHAN, &chindx);
+        if (ret < 0)
+                return -errno;
+
+        int chindx2;
+        ret = ioctl(session_fd2, PPPIOCGCHAN, &chind2x);
+        if (ret < 0)
+                return -errno;
+
+        int ppp_chan_fd = open("/dev/ppp", O_RDWR);
+
+        ret = ioctl(ppp_chan_fd, PPPIOCBRIDGECHAN, &chindx2);
+        if (ret < 0)
+                return -errno;
+
+        close(ppp_chan_fd);
+
 Old L2TPv2-only API
 -------------------
 

Re: [PATCH] PPPoL2TP: Add more code snippets

[Dominique Martinet]

Samuel Thibault wrote on Mon, Apr 17, 2023 at 12:07:04AM +0200:
> The existing documentation was not telling that one has to create a PPP
> channel and a PPP interface to get PPPoL2TP data offloading working.
> 
> Also, tunnel switching was not described, so that people were thinking
> it was not supported, while it actually is.
> 
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

Just passing by, thanks!
The update made me ask a couple more questions so I've commented below,
but I think this is already useful in itself so don't hold back for me.

> ---
>  Documentation/networking/l2tp.rst |   59 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 56 insertions(+), 3 deletions(-)
> 
> --- a/Documentation/networking/l2tp.rst
> +++ b/Documentation/networking/l2tp.rst
> @@ -387,11 +387,12 @@ Sample userspace code:
>    - Create session PPPoX data socket::
>  
>          struct sockaddr_pppol2tp sax;
> -        int fd;
> +        int ret;
>  
>          /* Note, the tunnel socket must be bound already, else it
>           * will not be ready
>           */
> +        int session_fd = socket(AF_PPPOX, SOCK_DGRAM, PX_PROTO_OL2TP);
>          sax.sa_family = AF_PPPOX;
>          sax.sa_protocol = PX_PROTO_OL2TP;
>          sax.pppol2tp.fd = tunnel_fd;
> @@ -406,12 +407,64 @@ Sample userspace code:
>          /* session_fd is the fd of the session's PPPoL2TP socket.
>           * tunnel_fd is the fd of the tunnel UDP / L2TPIP socket.
>           */
> -        fd = connect(session_fd, (struct sockaddr *)&sax, sizeof(sax));
> -        if (fd < 0 ) {
> +        ret = connect(session_fd, (struct sockaddr *)&sax, sizeof(sax));
> +        if (ret < 0 ) {
>                  return -errno;
>          }
>          return 0;
>  
> +  - Create PPP channel::
> +
> +        int chindx;
> +        ret = ioctl(session_fd, PPPIOCGCHAN, &chindx);
> +        if (ret < 0)
> +                return -errno;
> +
> +        int ppp_chan_fd = open("/dev/ppp", O_RDWR);
> +
> +        ret = ioctl(ppp_chan_fd, PPPIOCATTCHAN, &chindx);
> +        if (ret < 0)
> +                return -errno;
> +
> +Non-data PPP frames will be available for read on `ppp_chan_fd`.
> +
> +  - Create PPP interface::
> +
> +        int ppp_if_fd = open("/dev/ppp", O_RDWR);
> +
> +        int ifunit;
> +        ret = ioctl(ppp_if_fd, PPPIOCNEWUNIT, &ifunit);
> +        if (ret < 0)
> +                return -errno;
> +
> +        ret = ioctl(ppp_chan_fd, PPPIOCCONNECT, ifunit);
> +        if (ret < 0)
> +                return -errno;
> +
> +The ppp<ifunit> interface can then be configured as usual with SIOCSIFMTU,
> +SIOCSIFADDR, SIOCSIFDSTADDR, SIOCSIFNETMASK, and activated by setting IFF_UP
> +with SIOCSIFFLAGS

(That somewhat makes it sounds like the "new" netlink interface cannot
be used (e.g. ip command); although I guess sommeone implementing this
would be more likely to use the ioctls than not so having the names can
be a timesaver?)

Also, this got me wondering if the 'if' fd can be closed immediately or
if the interface will be removed when the fd is closed (probably not?)
If the fd can immediately be closed, could the chan fd or another fd be
used instead, saving an open?

> +
> +  - Tunnel switching is supported by bridging channels::
> +
> +        int chindx;
> +        ret = ioctl(session_fd, PPPIOCGCHAN, &chindx);
> +        if (ret < 0)
> +                return -errno;
> +
> +        int chindx2;
> +        ret = ioctl(session_fd2, PPPIOCGCHAN, &chind2x);
> +        if (ret < 0)
> +                return -errno;
> +
> +        int ppp_chan_fd = open("/dev/ppp", O_RDWR);
> +
> +        ret = ioctl(ppp_chan_fd, PPPIOCBRIDGECHAN, &chindx2);
> +        if (ret < 0)
> +                return -errno;
> +
> +        close(ppp_chan_fd);
> +
>  Old L2TPv2-only API
>  -------------------
>  

-- 
Dominique Martinet | Asmadeus

Re: [PATCH] PPPoL2TP: Add more code snippets

[Samuel Thibault]

Dominique Martinet, le lun. 17 avril 2023 07:26:41 +0900, a ecrit:
> > +The ppp<ifunit> interface can then be configured as usual with SIOCSIFMTU,
> > +SIOCSIFADDR, SIOCSIFDSTADDR, SIOCSIFNETMASK, and activated by setting IFF_UP
> > +with SIOCSIFFLAGS
> 
> (That somewhat makes it sounds like the "new" netlink interface cannot
> be used (e.g. ip command);

Ah, right...

> although I guess sommeone implementing this would be more likely to
> use the ioctls than not so having the names can be a timesaver?)

It's indeed a timesaver to have the ioctl names, but perhaps we can
replace this part with a pointer to a if-configuration documentation?

> Also, this got me wondering if the 'if' fd can be closed immediately or
> if the interface will be removed when the fd is closed (probably not?)

Closing the fd would close the if, yes. AIUI one really has to keep the
pppox socket (for stats), the ppp chan (for non-data ppp packets), and
the ppp if (for the if).

Samuel

Re: [PATCH] PPPoL2TP: Add more code snippets

[Guillaume Nault]

On Mon, Apr 17, 2023 at 12:43:16AM +0200, Samuel Thibault wrote:
> Dominique Martinet, le lun. 17 avril 2023 07:26:41 +0900, a ecrit:
> > Also, this got me wondering if the 'if' fd can be closed immediately or
> > if the interface will be removed when the fd is closed (probably not?)
> 
> Closing the fd would close the if, yes. AIUI one really has to keep the
> pppox socket (for stats), the ppp chan (for non-data ppp packets), and
> the ppp if (for the if).

L2TP has control and data packets. The L2TP socket is there to handle
L2TP control packets in user space, as the kernel only handles L2TP
data packets. You have to keep the L2TP session socket open, otherwise
you can't handle the session anymore.

Then there are the PPP file descriptors. A PPP channel is used to send
and receive PPP frames. It has to be associated with a lower transport,
for example an L2TP session if you want to encapsulate PPP into L2TP.
But that could be something else (a PPPoE session, a serial link, etc.).
Same as for L2TP session sockets, you need to keep the PPP channel fd
open, otherwise you can't handle the PPP session anymore.

Finally there are PPP units. A PPP unit represents the PPP networking
device (like ppp0). A PPP unit has to be associated with a PPP channel
(or several PPP channels for multilink PPP, but better avoid that). A
PPP unit doesn't know how to send packets, it delegates that to its PPP
channels.

You can avoid creating PPP units if you don't need a PPP network
device. In particular, if you're forwarding a PPP session between a
PPPoE and an L2TP session (or between two different L2TP sesions), you
typically don't need to create any PPP unit. You handle the initial LCP
negociation and PPP authentication using a PPP channel on the incoming
side, then you create another PPP channel on the other side (where you
want to forward the incoming PPP session) and finally bridge them
together with PPPIOCBRIDGECHAN.

> Samuel
> 

Re: [PATCH] PPPoL2TP: Add more code snippets

[Guillaume Nault]

On Mon, Apr 17, 2023 at 07:26:41AM +0900, Dominique Martinet wrote:
> Samuel Thibault wrote on Mon, Apr 17, 2023 at 12:07:04AM +0200:
> (That somewhat makes it sounds like the "new" netlink interface cannot
> be used (e.g. ip command); although I guess sommeone implementing this
> would be more likely to use the ioctls than not so having the names can
> be a timesaver?)

I don't understand what you mean by 'the "new" netlink interface'. You
can create a PPP interface either with the PPPIOCNEWUNIT ioctl or with
netlink. But no matter how you create it, you need a /dev/ppp file
descriptor associated to the PPP network device. Other than that, and
no matter how you create them, PPP network devices can be used and
configured like any other network interface. You absolutely can use
"ip link" to manage you ppp interfaces.

Re: [PATCH] PPPoL2TP: Add more code snippets

[Guillaume Nault]

On Mon, Apr 17, 2023 at 12:07:04AM +0200, Samuel Thibault wrote:
> The existing documentation was not telling that one has to create a PPP
> channel and a PPP interface to get PPPoL2TP data offloading working.
> 
> Also, tunnel switching was not described, so that people were thinking
> it was not supported, while it actually is.
> 
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> 
> ---
>  Documentation/networking/l2tp.rst |   59 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 56 insertions(+), 3 deletions(-)
> 
> --- a/Documentation/networking/l2tp.rst
> +++ b/Documentation/networking/l2tp.rst
> @@ -387,11 +387,12 @@ Sample userspace code:
>    - Create session PPPoX data socket::
>  
>          struct sockaddr_pppol2tp sax;
> -        int fd;
> +        int ret;
>  
>          /* Note, the tunnel socket must be bound already, else it
>           * will not be ready
>           */
> +        int session_fd = socket(AF_PPPOX, SOCK_DGRAM, PX_PROTO_OL2TP);

Please declare session_fd with the other variables.
Also, check the return value of the socket() call.

>          sax.sa_family = AF_PPPOX;
>          sax.sa_protocol = PX_PROTO_OL2TP;
>          sax.pppol2tp.fd = tunnel_fd;
> @@ -406,12 +407,64 @@ Sample userspace code:
>          /* session_fd is the fd of the session's PPPoL2TP socket.
>           * tunnel_fd is the fd of the tunnel UDP / L2TPIP socket.
>           */
> -        fd = connect(session_fd, (struct sockaddr *)&sax, sizeof(sax));
> -        if (fd < 0 ) {
> +        ret = connect(session_fd, (struct sockaddr *)&sax, sizeof(sax));
> +        if (ret < 0 ) {

Now you also need to close session_fd.

>                  return -errno;
>          }
>          return 0;
>  
> +  - Create PPP channel::
> +
> +        int chindx;
> +        ret = ioctl(session_fd, PPPIOCGCHAN, &chindx);
> +        if (ret < 0)
> +                return -errno;
> +
> +        int ppp_chan_fd = open("/dev/ppp", O_RDWR);
> +
> +        ret = ioctl(ppp_chan_fd, PPPIOCATTCHAN, &chindx);
> +        if (ret < 0)
> +                return -errno;
> +
> +Non-data PPP frames will be available for read on `ppp_chan_fd`.
> +
> +  - Create PPP interface::
> +
> +        int ppp_if_fd = open("/dev/ppp", O_RDWR);

Check for errors please.

> +
> +        int ifunit;

Also, keep kernel style formatting:
  * All variable declarations in one block (ordered from longest to
    shortest line).
  * New line between variable declarations and code.

> +        ret = ioctl(ppp_if_fd, PPPIOCNEWUNIT, &ifunit);

You need to initialise ifunit first.
Use -1 to let the kernel pick a free unit index.

> +        if (ret < 0)
> +                return -errno;
> +
> +        ret = ioctl(ppp_chan_fd, PPPIOCCONNECT, ifunit);
> +        if (ret < 0)
> +                return -errno;
> +
> +The ppp<ifunit> interface can then be configured as usual with SIOCSIFMTU,
> +SIOCSIFADDR, SIOCSIFDSTADDR, SIOCSIFNETMASK, and activated by setting IFF_UP
> +with SIOCSIFFLAGS
> +
> +  - Tunnel switching is supported by bridging channels::

This is a PPP feature not an L2TP one. PPPIOCBRIDGECHAN's description
belongs to Documentation/networking/ppp_generic.rst, where it's already
documented. If documentation needs to be improved, that should be done
there.

If necessary, you can link to ppp_generic.rst here.

Also, calling this feature 'tunnel switching' is misleading. Switching
happens between L2TP sessions (or more generally between any PPP
transports), not between L2TP tunnels (which are just L2TP session
multiplexers).

Re: [PATCH] PPPoL2TP: Add more code snippets

[Samuel Thibault]

Guillaume Nault, le mar. 18 avril 2023 10:34:03 +0200, a ecrit:
> On Mon, Apr 17, 2023 at 12:07:04AM +0200, Samuel Thibault wrote:
> >          sax.sa_family = AF_PPPOX;
> >          sax.sa_protocol = PX_PROTO_OL2TP;
> >          sax.pppol2tp.fd = tunnel_fd;
> > @@ -406,12 +407,64 @@ Sample userspace code:
> >          /* session_fd is the fd of the session's PPPoL2TP socket.
> >           * tunnel_fd is the fd of the tunnel UDP / L2TPIP socket.
> >           */
> > -        fd = connect(session_fd, (struct sockaddr *)&sax, sizeof(sax));
> > -        if (fd < 0 ) {
> > +        ret = connect(session_fd, (struct sockaddr *)&sax, sizeof(sax));
> > +        if (ret < 0 ) {
> 
> Now you also need to close session_fd.

? No, we need it for PPPIOCGCHAN, and also PPPIOCGL2TPSTATS.

I'll put return session_fd instead.

> > +The ppp<ifunit> interface can then be configured as usual with SIOCSIFMTU,
> > +SIOCSIFADDR, SIOCSIFDSTADDR, SIOCSIFNETMASK, and activated by setting IFF_UP
> > +with SIOCSIFFLAGS
> > +
> > +  - Tunnel switching is supported by bridging channels::
> 
> This is a PPP feature not an L2TP one.
> 
> PPPIOCBRIDGECHAN's description
> belongs to Documentation/networking/ppp_generic.rst, where it's already
> documented.

Yes but that's hard to find out when you're looking from the L2TP end.

> If necessary, you can link to ppp_generic.rst here.
> 
> Also, calling this feature 'tunnel switching' is misleading.

That's how I have seen it is called in L2TP jargon.

Samuel

Re: [PATCH] PPPoL2TP: Add more code snippets

[Guillaume Nault]

On Tue, Apr 18, 2023 at 10:53:23AM +0200, Samuel Thibault wrote:
> Guillaume Nault, le mar. 18 avril 2023 10:34:03 +0200, a ecrit:
> > On Mon, Apr 17, 2023 at 12:07:04AM +0200, Samuel Thibault wrote:
> > >          sax.sa_family = AF_PPPOX;
> > >          sax.sa_protocol = PX_PROTO_OL2TP;
> > >          sax.pppol2tp.fd = tunnel_fd;
> > > @@ -406,12 +407,64 @@ Sample userspace code:
> > >          /* session_fd is the fd of the session's PPPoL2TP socket.
> > >           * tunnel_fd is the fd of the tunnel UDP / L2TPIP socket.
> > >           */
> > > -        fd = connect(session_fd, (struct sockaddr *)&sax, sizeof(sax));
> > > -        if (fd < 0 ) {
> > > +        ret = connect(session_fd, (struct sockaddr *)&sax, sizeof(sax));
> > > +        if (ret < 0 ) {
> > 
> > Now you also need to close session_fd.
> 
> ? No, we need it for PPPIOCGCHAN, and also PPPIOCGL2TPSTATS.

connect() failed. You can't do anything with this socket.

> I'll put return session_fd instead.

What's the point of returning session_fd if connect() failed?
How will the caller know if session_fd is connected or not?
Why would it even be interested in a half-created session fd?

> > > +The ppp<ifunit> interface can then be configured as usual with SIOCSIFMTU,
> > > +SIOCSIFADDR, SIOCSIFDSTADDR, SIOCSIFNETMASK, and activated by setting IFF_UP
> > > +with SIOCSIFFLAGS
> > > +
> > > +  - Tunnel switching is supported by bridging channels::
> > 
> > This is a PPP feature not an L2TP one.
> > 
> > PPPIOCBRIDGECHAN's description
> > belongs to Documentation/networking/ppp_generic.rst, where it's already
> > documented.
> 
> Yes but that's hard to find out when you're looking from the L2TP end.

That's why I proposed linking to ppp_generic.rst.

> > If necessary, you can link to ppp_generic.rst here.
> > 
> > Also, calling this feature 'tunnel switching' is misleading.
> 
> That's how I have seen it is called in L2TP jargon.

That still doesn't describe the kernel feature. We can add a 'so called
"tunnel switching" in L2TP jargon' into parenthesis to give a hint to
the people using this terminology.

> Samuel
> 

Re: [PATCH] PPPoL2TP: Add more code snippets

[Samuel Thibault]

Guillaume Nault, le mar. 18 avril 2023 11:06:51 +0200, a ecrit:
> On Tue, Apr 18, 2023 at 10:53:23AM +0200, Samuel Thibault wrote:
> > Guillaume Nault, le mar. 18 avril 2023 10:34:03 +0200, a ecrit:
> > > On Mon, Apr 17, 2023 at 12:07:04AM +0200, Samuel Thibault wrote:
> > > >          sax.sa_family = AF_PPPOX;
> > > >          sax.sa_protocol = PX_PROTO_OL2TP;
> > > >          sax.pppol2tp.fd = tunnel_fd;
> > > > @@ -406,12 +407,64 @@ Sample userspace code:
> > > >          /* session_fd is the fd of the session's PPPoL2TP socket.
> > > >           * tunnel_fd is the fd of the tunnel UDP / L2TPIP socket.
> > > >           */
> > > > -        fd = connect(session_fd, (struct sockaddr *)&sax, sizeof(sax));
> > > > -        if (fd < 0 ) {
> > > > +        ret = connect(session_fd, (struct sockaddr *)&sax, sizeof(sax));
> > > > +        if (ret < 0 ) {
> > > 
> > > Now you also need to close session_fd.
> > 
> > ? No, we need it for PPPIOCGCHAN, and also PPPIOCGL2TPSTATS.
> 
> connect() failed. You can't do anything with this socket.

Ah, you were talking about the failure case, ok.

> > > > +The ppp<ifunit> interface can then be configured as usual with SIOCSIFMTU,
> > > > +SIOCSIFADDR, SIOCSIFDSTADDR, SIOCSIFNETMASK, and activated by setting IFF_UP
> > > > +with SIOCSIFFLAGS
> > > > +
> > > > +  - Tunnel switching is supported by bridging channels::
> > > 
> > > This is a PPP feature not an L2TP one.
> > > 
> > > PPPIOCBRIDGECHAN's description
> > > belongs to Documentation/networking/ppp_generic.rst, where it's already
> > > documented.
> > 
> > Yes but that's hard to find out when you're looking from the L2TP end.
> 
> That's why I proposed linking to ppp_generic.rst.

Yes, but it's still not obvious to L2TP people that it's a ppp channel
that you have to bridge. Really, having that 20-line snippet available
would have saved me some head-scratching time.

Samuel

Re: [PATCH] PPPoL2TP: Add more code snippets

[Guillaume Nault]

On Tue, Apr 18, 2023 at 11:11:48AM +0200, Samuel Thibault wrote:
> Guillaume Nault, le mar. 18 avril 2023 11:06:51 +0200, a ecrit:
> > On Tue, Apr 18, 2023 at 10:53:23AM +0200, Samuel Thibault wrote:
> > > Guillaume Nault, le mar. 18 avril 2023 10:34:03 +0200, a ecrit:
> > > > PPPIOCBRIDGECHAN's description
> > > > belongs to Documentation/networking/ppp_generic.rst, where it's already
> > > > documented.
> > > 
> > > Yes but that's hard to find out when you're looking from the L2TP end.
> > 
> > That's why I proposed linking to ppp_generic.rst.
> 
> Yes, but it's still not obvious to L2TP people that it's a ppp channel
> that you have to bridge. Really, having that 20-line snippet available
> would have saved me some head-scratching time.

But the reverse is also true: someone looking at the PPP documentation
is probably not going to realise that PPP sample code have been put in
the L2TP doc.

We can instead add PPP specific code in the PPP documentation. Then add
a line in l2tp.rst saying something like "Handling of the PPP layer is
described in ppp_generic."

If you feel like having sample code for a full example covering the
whole process of creating and setting up the file descriptiors needed
for bridging PPP channels (UDP, L2TP tunnels, L2TP sessions and PPP
channels, and why not PPPoE), then I feel a specific file would be more
indicated, as this would cross several different subsystems.

It'd be okay to to have a few PPP-specific calls in l2tp.rst since
L2TPv2 is tied to PPP and that could give a more complete example of
how to use the L2TP uAPI. But that should be limitted to the most basic
PPP features (creating a channel and a unit).

> Samuel
> 

Re: [PATCH] PPPoL2TP: Add more code snippets

[Samuel Thibault]

Guillaume Nault, le mar. 18 avril 2023 12:17:55 +0200, a ecrit:
> On Tue, Apr 18, 2023 at 11:11:48AM +0200, Samuel Thibault wrote:
> > Guillaume Nault, le mar. 18 avril 2023 11:06:51 +0200, a ecrit:
> > > On Tue, Apr 18, 2023 at 10:53:23AM +0200, Samuel Thibault wrote:
> > > > Guillaume Nault, le mar. 18 avril 2023 10:34:03 +0200, a ecrit:
> > > > > PPPIOCBRIDGECHAN's description
> > > > > belongs to Documentation/networking/ppp_generic.rst, where it's already
> > > > > documented.
> > > > 
> > > > Yes but that's hard to find out when you're looking from the L2TP end.
> > > 
> > > That's why I proposed linking to ppp_generic.rst.
> > 
> > Yes, but it's still not obvious to L2TP people that it's a ppp channel
> > that you have to bridge. Really, having that 20-line snippet available
> > would have saved me some head-scratching time.
> 
> But the reverse is also true: someone looking at the PPP documentation
> is probably not going to realise that PPP sample code have been put in
> the L2TP doc.

Yes, but for PPP people it is obvious that you'll want to bridge two
channels.

The point of the code is not really the bridging ioctl call, but the
fact that you have to use PPPIOCGCHAN over the two sessions, then open
a ppp channel, to be able to make the bridging ioctl call. *That*
is what is really not obvious, and will not actually fit in the PPP
documentation. Of course we could move the few ppp-only lines to the PPP
documentation, but I really don't see the point: that part is obvious in
the PPP context.

Samuel

Re: [PATCH] PPPoL2TP: Add more code snippets

[Guillaume Nault]

On Tue, Apr 18, 2023 at 12:31:40PM +0200, Samuel Thibault wrote:
> Guillaume Nault, le mar. 18 avril 2023 12:17:55 +0200, a ecrit:
> > On Tue, Apr 18, 2023 at 11:11:48AM +0200, Samuel Thibault wrote:
> > > Guillaume Nault, le mar. 18 avril 2023 11:06:51 +0200, a ecrit:
> > > > On Tue, Apr 18, 2023 at 10:53:23AM +0200, Samuel Thibault wrote:
> > > > > Guillaume Nault, le mar. 18 avril 2023 10:34:03 +0200, a ecrit:
> > > > > > PPPIOCBRIDGECHAN's description
> > > > > > belongs to Documentation/networking/ppp_generic.rst, where it's already
> > > > > > documented.
> > > > > 
> > > > > Yes but that's hard to find out when you're looking from the L2TP end.
> > > > 
> > > > That's why I proposed linking to ppp_generic.rst.
> > > 
> > > Yes, but it's still not obvious to L2TP people that it's a ppp channel
> > > that you have to bridge. Really, having that 20-line snippet available
> > > would have saved me some head-scratching time.
> > 
> > But the reverse is also true: someone looking at the PPP documentation
> > is probably not going to realise that PPP sample code have been put in
> > the L2TP doc.
> 
> Yes, but for PPP people it is obvious that you'll want to bridge two
> channels.
> 
> The point of the code is not really the bridging ioctl call, but the
> fact that you have to use PPPIOCGCHAN over the two sessions, then open
> a ppp channel, to be able to make the bridging ioctl call. *That*
> is what is really not obvious, and will not actually fit in the PPP
> documentation. Of course we could move the few ppp-only lines to the PPP
> documentation, but I really don't see the point: that part is obvious in
> the PPP context.

For PPPIOCGCHAN, I agree it should be documented in l2tp.rst. This
ioctl is common to all PPPOX sockets, but it wouldn't make sense to
have a separate document just for it. And L2TP is the only PPPOX user
that is documented as far as I know.

As I said in my previous reply, a simple L2TP example that goes until PPP
channel and unit creation is fine. But any more advanced use of the PPP
API should be documented in the PPP documentation.

I mean, these files document the API of their corresponding modules,
their scope should be limitted to that (the PPP and L2TP layers are
really different).

That shouldn't preclude anyone from describing how to combine L2TP, PPP
and others to cover more advanced use cases. It's just better done in a
different file.

> Samuel
> 

Re: [PATCH] PPPoL2TP: Add more code snippets

[Samuel Thibault]

Guillaume Nault, le mar. 18 avril 2023 13:25:38 +0200, a ecrit:
> As I said in my previous reply, a simple L2TP example that goes until PPP
> channel and unit creation is fine. But any more advanced use of the PPP
> API should be documented in the PPP documentation.

When it's really advanced, yes. But here it's just about tunnel
bridging, which is a very common L2TP thing to do.

> I mean, these files document the API of their corresponding modules,
> their scope should be limitted to that (the PPP and L2TP layers are
> really different).

I wouldn't call

+        ret = ioctl(ppp_chan_fd, PPPIOCBRIDGECHAN, &chindx2);
+        close(ppp_chan_fd);
+        if (ret < 0)
+                return -errno;

documentation...

> That shouldn't preclude anyone from describing how to combine L2TP, PPP
> and others to cover more advanced use cases. It's just better done in a
> different file.

A more complete example, yes. I don't plan on taking time to do it.

Samuel

Re: [PATCH] PPPoL2TP: Add more code snippets

[Guillaume Nault]

On Tue, Apr 18, 2023 at 01:54:09PM +0200, Samuel Thibault wrote:
> Guillaume Nault, le mar. 18 avril 2023 13:25:38 +0200, a ecrit:
> > As I said in my previous reply, a simple L2TP example that goes until PPP
> > channel and unit creation is fine. But any more advanced use of the PPP
> > API should be documented in the PPP documentation.
> 
> When it's really advanced, yes. But here it's just about tunnel
> bridging, which is a very common L2TP thing to do.

I can't undestand why you absolutely want this covered in l2tp.rst.
This feature also works on PPPoE.

Also, it's probably a desirable feature, but certainly not a common
thing on Linux. This interface was added a bit more than 2 years ago,
which is really recent considering the age of the code. Appart from
maybe go-l2tp, I don't know of any user.

> > I mean, these files document the API of their corresponding modules,
> > their scope should be limitted to that (the PPP and L2TP layers are
> > really different).
> 
> I wouldn't call
> 
> +        ret = ioctl(ppp_chan_fd, PPPIOCBRIDGECHAN, &chindx2);
> +        close(ppp_chan_fd);
> +        if (ret < 0)
> +                return -errno;
> 
> documentation...

The documentation is in ppp_generic.rst. Does it really make sense to
you to have the doc there and the sample code in l2tp.rst?

Anyway, I'm not going to argue any longer. You have my opinion.
You're always free to ignore feedbacks.

> > That shouldn't preclude anyone from describing how to combine L2TP, PPP
> > and others to cover more advanced use cases. It's just better done in a
> > different file.
> 
> A more complete example, yes. I don't plan on taking time to do it.
> 
> Samuel
> 

Re: [PATCH] PPPoL2TP: Add more code snippets

[Samuel Thibault]

Guillaume Nault, le mar. 18 avril 2023 15:38:00 +0200, a ecrit:
> On Tue, Apr 18, 2023 at 01:54:09PM +0200, Samuel Thibault wrote:
> > Guillaume Nault, le mar. 18 avril 2023 13:25:38 +0200, a ecrit:
> > > As I said in my previous reply, a simple L2TP example that goes until PPP
> > > channel and unit creation is fine. But any more advanced use of the PPP
> > > API should be documented in the PPP documentation.
> > 
> > When it's really advanced, yes. But here it's just about tunnel
> > bridging, which is a very common L2TP thing to do.
> 
> I can't undestand why you absolutely want this covered in l2tp.rst.

Because that's where people working on L2TP software will look for it.

> This feature also works on PPPoE.

Then PPPoE documentation shouls also contain mentions of it.

Note (and I'll repeat myself again below) I'm not talking about
*documentation* (which belongs to ppp_generic.rst, but *mentions* of it.

Networking is complex. If each protocol only speaks for itself and
doesn't take the effort of showing how they glue with others, it's hard
for people to grasp the whole ideas.

> Also, it's probably a desirable feature, but certainly not a common
> thing on Linux. This interface was added a bit more than 2 years ago,
> which is really recent considering the age of the code.

Yes, and in ISPs we have been in need for it for something like
decades. I can find RFC drafts around 2000.

Or IPs have just baked their own kernel implementation (xl2tpd,
accel-ppp, etc.)

> Appart from maybe go-l2tp, I don't know of any user.

Because it's basically undocumented from the point of view of L2TP
people.

> > > I mean, these files document the API of their corresponding modules,
> > > their scope should be limitted to that (the PPP and L2TP layers are
> > > really different).
> > 
> > I wouldn't call
> > 
> > +        ret = ioctl(ppp_chan_fd, PPPIOCBRIDGECHAN, &chindx2);
> > +        close(ppp_chan_fd);
> > +        if (ret < 0)
> > +                return -errno;
> > 
> > documentation...
> 
> The documentation is in ppp_generic.rst.

Yes. and I *definitely* agree on that, and exactly what I'm all talking
about. I'm here just arguing about putting these _*_4 lines of code_*_
example in l2tp.rst, _*_nothing more_*_. See the subject of this thread:
"code snippets". Not documentation.

> Does it really make sense to you to have the doc there

There is basically no doc in what I am proposing.

> and the sample code in l2tp.rst?

Yes, because then L2TP people can be sure to understand how things plug
altogether before writing code...

... instead of having to try various things until understanding how it's
all supposed to fit altogether.

Samuel

Re: [PATCH] PPPoL2TP: Add more code snippets

[Tom Parkin]

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

On  Tue, Apr 18, 2023 at 16:18:20 +0200, Samuel Thibault wrote:
> Guillaume Nault, le mar. 18 avril 2023 15:38:00 +0200, a ecrit:
> > On Tue, Apr 18, 2023 at 01:54:09PM +0200, Samuel Thibault wrote:
> > > Guillaume Nault, le mar. 18 avril 2023 13:25:38 +0200, a ecrit:
> > > > As I said in my previous reply, a simple L2TP example that goes until PPP
> > > > channel and unit creation is fine. But any more advanced use of the PPP
> > > > API should be documented in the PPP documentation.
> > > 
> > > When it's really advanced, yes. But here it's just about tunnel
> > > bridging, which is a very common L2TP thing to do.
> > 
> > I can't undestand why you absolutely want this covered in l2tp.rst.
> 
> Because that's where people working on L2TP software will look for it.

Sorry to have not commented earlier, and thank you Samuel for working
on improving the L2TP documentation.

I think documentation like l2tp.rst is best when it provides a high
level overview of how things fit together.

When it comes to actually implementing a userspace L2TP/PPP daemon,
I feel that at a certain point you're better off referring to existing
userspace code alongside the kernel sources themselves, as any summary is
inevitably going to leave gaps.  From that perspective I'd almost sooner
we didn't have the code snippet in l2tp.rst.

That said, I can't see the harm in improving the code snippet, given
that we have it already.  Having no mention of PPPIOCBRIDGECHAN given
that it can be used to implement tunnel switching is an oversight
really.

FWIW I agree the term "tunnel switching" is a bit misleading, and of
course the PPP ioctl supports bridging any flavour of channel, not
just PPPoL2TP.  However from the L2TP perspective people perhaps have
something along the lines of this IETF draft in mind:

https://datatracker.ietf.org/doc/html/draft-ietf-l2tpext-tunnel-switching-08

...which we could perhaps link to to clarify the intent in the context
of the L2TP codebase?

> > Also, it's probably a desirable feature, but certainly not a common
> > thing on Linux. This interface was added a bit more than 2 years ago,
> > which is really recent considering the age of the code.
> 
> Yes, and in ISPs we have been in need for it for something like
> decades. I can find RFC drafts around 2000.
> 
> Or IPs have just baked their own kernel implementation (xl2tpd,
> accel-ppp, etc.)

Yes.  It's sad that support wasn't available sooner in the kernel, but
I'm not sure that's indicative of lack of desire for the feature
necessarily.

> > Appart from maybe go-l2tp, I don't know of any user.
> 

I confirm that go-l2tp does use it :-)

-- 
Tom Parkin
Katalix Systems Ltd
https://katalix.com
Catalysts for your Embedded Linux software development

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