Message-ID: <ZcqMzP1RQye9o4eB@katalix.com>
Date: Mon, 12 Feb 2024 21:25:32 +0000
From: Tom Parkin <tparkin@...alix.com>
To: Samuel Thibault <samuel.thibault@...-lyon.org>,
	James Chapman <jchapman@...alix.com>, edumazet@...gle.com,
	gnault@...hat.com, davem@...emloft.net, kuba@...nel.org,
	pabeni@...hat.com, corbet@....net, netdev@...r.kernel.org,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCHv3] PPPoL2TP: Add more code snippets

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

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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists