[PATCHv3] PPPoL2TP: Add more code snippets

  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 mentioned, so that people were thinking
it was not supported, while it actually is.

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

---
Difference from v1:
- follow kernel coding style
- check for failures
- also mention netlink and ip for configuring the link
- fix bridging channels

Difference from v2:
- fix text alignment

 Documentation/networking/l2tp.rst |   99 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 95 insertions(+), 4 deletions(-)
  

On Sat, 3 Feb 2024 23:35:13 +0100 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 mentioned, so that people were thinking
> it was not supported, while it actually is.
> 
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

James, Tom, looks good?
  

Thanks Samuel, comments inline below.

On  Sat, Feb 03, 2024 at 23:35:13 +0100, 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 mentioned, so that people were thinking
> it was not supported, while it actually is.
> 
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> 
> ---
> Difference from v1:
> - follow kernel coding style
> - check for failures
> - also mention netlink and ip for configuring the link
> - fix bridging channels
> 
> Difference from v2:
> - fix text alignment
> 
>  Documentation/networking/l2tp.rst |   99 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 95 insertions(+), 4 deletions(-)
> 
> --- a/Documentation/networking/l2tp.rst
> +++ b/Documentation/networking/l2tp.rst
> @@ -387,11 +387,16 @@ Sample userspace code:
>    - Create session PPPoX data socket::
>  
>          struct sockaddr_pppol2tp sax;
> -        int fd;
> +        int session_fd;
> +        int ret;
>  
>          /* Note, the tunnel socket must be bound already, else it
>           * will not be ready
>           */
> +        session_fd = socket(AF_PPPOX, SOCK_DGRAM, PX_PROTO_OL2TP);
> +        if (session_fd < 0)
> +                return -errno;
> +
>          sax.sa_family = AF_PPPOX;
>          sax.sa_protocol = PX_PROTO_OL2TP;
>          sax.pppol2tp.fd = tunnel_fd;
> @@ -406,11 +411,97 @@ 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 ) {
> +                close(session_fd);
> +                return -errno;
> +        }
> +
> +        return session_fd;
> +
> +L2TP control packets will still be available for read on `tunnel_fd`.
> +
> +  - Create PPP channel::
> +
> +        int chindx;
> +        int ppp_chan_fd;
> +
> +        ret = ioctl(session_fd, PPPIOCGCHAN, &chindx);
> +        if (ret < 0)
> +                return -errno;
> +
> +        ppp_chan_fd = open("/dev/ppp", O_RDWR);
> +        if (ppp_chan_fd < 0)
> +                return -errno;
> +
> +        ret = ioctl(ppp_chan_fd, PPPIOCATTCHAN, &chindx);
> +        if (ret < 0) {
> +                close(ppp_chan_fd);
> +                return -errno;
> +        }
> +
> +        return ppp_chan_fd;
> +
> +LCP PPP frames will be available for read on `ppp_chan_fd`.
> +
> +  - Create PPP interface::
> +
> +        int ppp_if_fd;
> +        int ifunit = -1;
> +
> +        ppp_if_fd = open("/dev/ppp", O_RDWR);
> +        if (ppp_chan_fd < 0)

I think this should be 'if (ppp_if_fd < 0)' ..?

> +                return -errno;
> +
> +        ret = ioctl(ppp_if_fd, PPPIOCNEWUNIT, &ifunit);
> +        if (ret < 0) {
> +                close(ppp_if_fd);
> +                return -errno;
> +        }
> +
> +        ret = ioctl(ppp_chan_fd, PPPIOCCONNECT, ifunit);
> +        if (ret < 0) {
> +                close(ppp_if_fd);
> +                return -errno;
> +        }
> +
> +        return ppp_chan_fd;

...and this should be 'return ppp_if_fd'.

> +
> +IPCP/IPv6CP PPP frames will be available for read on `ppp_if_fd`.
> +
> +The ppp<ifunit> interface can then be configured as usual with netlink's
> +RTM_NEWLINK, RTM_NEWADDR, RTM_NEWROUTE, or ioctl's SIOCSIFMTU, SIOCSIFADDR,
> +SIOCSIFDSTADDR, SIOCSIFNETMASK, SIOCSIFFLAGS, or with the `ip` command.
> +
> +  - L2TP session bridging (also called L2TP tunnel switching or L2TP multihop)
> +    is supported by bridging the ppp channels of the two L2TP sessions to be
> +    bridged::

Since we're in L2TP-world here it is probably worth making it clear
that this only applies to PPP pseudowire types.

> +
> +        int chindx1;
> +        int chindx2;
> +        int ppp_chan_fd;
> +
> +        ret = ioctl(session_fd1, PPPIOCGCHAN, &chindx1);
> +        if (ret < 0)
> +                return -errno;
> +
> +        ret = ioctl(session_fd2, PPPIOCGCHAN, &chind2x);

Typo here I think: s/chind2x/chindx2/ ?

> +        if (ret < 0)
> +                return -errno;
> +
> +        ppp_chan_fd = open("/dev/ppp", O_RDWR);

Missing a check on ppp_chan_fd -- we might as well check it since
we're checking returns everywhere else.

> +        ret = ioctl(ppp_chan_fd, PPPIOCATTCHAN, &chindx1);
> +        if (ret < 0) {
> +                close(ppp_chan_fd);
>                  return -errno;
>          }
> -        return 0;
> +
> +        ret = ioctl(ppp_chan_fd, PPPIOCBRIDGECHAN, &chindx2);
> +        close(ppp_chan_fd);
> +        if (ret < 0)
> +                return -errno;
> +
> +See more details for the PPP side in ppp_generic.rst.

I think we need to be clear here in this example what session_fd1 and
session_fd2 are, and how they have come to be, since they haven't been
mentioned in the examples so far.

I'm not sure whether it helps or not, but when we were working on l2tp-ktest
initially we had tests for the bridge ioctl.  The tests bridged a PPPoE
channel with a PPPoL2TP one (which was the original motivation for
PPPIOCBRIDGECHAN).  The code is here:

https://github.com/katalix/l2tp-ktest/blob/master/src/util.c#L592

So in that codebase we have a pppoe fd and a pppol2tp fd, both of
which have had been attached using PPPIOCATTCHAN.

We then bridge those two channels using PPPIOCBRIDGECHAN.

I think the bridging is a complex use-case for what is already quite
an involved API (lots of file descriptors and indices to keep track
of!).  So I think the code snippet needs to be as clear as we can make
it.

Thanks again for your work on the documentation.
  

On  Fri, Feb 09, 2024 at 08:20:46 -0800, Jakub Kicinski wrote:
> On Sat, 3 Feb 2024 23:35:13 +0100 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 mentioned, so that people were thinking
> > it was not supported, while it actually is.
> > 
> > Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> 
> James, Tom, looks good?

Sorry for the silence -- I think for v2 we had some discussion of
whether the PPPIOCBRIDGECHAN docs really belonged in l2tp.rst or not
and I wasn't sure whether the same issue would be raised again.

For me I'm happy enough having it mentioned in the l2tp documentation,
so long as the example is clear and accurate.  I've responded to the
patch now with a couple of suggestions on that front.

Thanks,
Tom
  

Hello,

Thanks for reviewing, I'll send a v4.

Tom Parkin, le lun. 12 févr. 2024 21:25:32 +0000, a ecrit:
> > +        ret = ioctl(session_fd1, PPPIOCGCHAN, &chindx1);
> > +        if (ret < 0)
> > +                return -errno;
> > +
> > +        ret = ioctl(session_fd2, PPPIOCGCHAN, &chind2x);
> 
> I think we need to be clear here in this example what session_fd1 and
> session_fd2 are, and how they have come to be, since they haven't been
> mentioned in the examples so far.

Right (though it's called session_fd above), I have added explicit input
descriptions.

> I'm not sure whether it helps or not, but when we were working on l2tp-ktest
> initially we had tests for the bridge ioctl.  The tests bridged a PPPoE
> channel with a PPPoL2TP one (which was the original motivation for
> PPPIOCBRIDGECHAN).  The code is here:
> 
> https://github.com/katalix/l2tp-ktest/blob/master/src/util.c#L592
> 
> So in that codebase we have a pppoe fd and a pppol2tp fd, both of
> which have had been attached using PPPIOCATTCHAN.
> 
> We then bridge those two channels using PPPIOCBRIDGECHAN.
> 
> I think the bridging is a complex use-case for what is already quite
> an involved API (lots of file descriptors and indices to keep track
> of!).  So I think the code snippet needs to be as clear as we can make
> it.

Yes, I kept the simple l2tp-to-l2tp example, but mentioned that other
types of ppp channels can also be bridged.  The important part is that
the code snippets show exactly what kind of fd is expected where :)

Samuel