[PATCHv4] 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

Difference from v3:
- fix some variables references
- explicit inputs of the code snippets
- explicit that bridging is supported for l2tp with PPP pseudowire type.
- explicit that after bridging only the pppox sockets need to be kept
- explicit that bridging can also be done with other types of ppp
  channels
  

On  Mon, Feb 12, 2024 at 23:23:44 +0100, Samuel Thibault wrote:
> --- a/Documentation/networking/l2tp.rst
> +++ b/Documentation/networking/l2tp.rst
> @@ -386,12 +386,17 @@ Sample userspace code:
>  
>    - Create session PPPoX data socket::
>  
> +        /* Input: the L2TP tunnel UDP socket `tunnel_fd`, which needs to be
> +         * bound already, otherwise it will not be ready. */
> +

Sorry for the nit :-| but we should adhere to the kernel coding style
I think -- so the comment block should look like this:

    /* Input: the L2TP tunnel UDP socket `tunnel_fd`, which needs to be
     * bound already, otherwise it will not be ready.
     */

>          struct sockaddr_pppol2tp sax;
> -        int fd;
> +        int session_fd;
> +        int ret;
> +
> +        session_fd = socket(AF_PPPOX, SOCK_DGRAM, PX_PROTO_OL2TP);
> +        if (session_fd < 0)
> +                return -errno;
>  
> -        /* Note, the tunnel socket must be bound already, else it
> -         * will not be ready
> -         */
>          sax.sa_family = AF_PPPOX;
>          sax.sa_protocol = PX_PROTO_OL2TP;
>          sax.pppol2tp.fd = tunnel_fd;
> @@ -406,11 +411,117 @@ 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::
> +
> +        /* Input: the session PPPoX data socket session_fd which was created as
> +         * described above. */

Again, I think this comment should adhere to the kernel coding style.

Do we need backticks for `session_fd` for consistency?

> +
> +        int chindx;
> +        int ppp_chan_fd;

...and we should follow netdev-style Christmas-tree declaration
ordering, i.e:

    int ppp_chan_fd;
    int chindx;

> +
> +        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::
> +
> +        /* Input: the PPP channel ppp_chan_fd which was created as described
> +         * above */

Again, I think this comment should adhere to the kernel coding style.

Do we need backticks for `ppp_chan_fd` for consistency?

> +
> +        int ppp_if_fd;
> +        int ifunit = -1;

netdev-style Christmas-tree declaration ordering here too please.

> +
> +        ppp_if_fd = open("/dev/ppp", O_RDWR);
> +        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);

I think this should be:

    ret = ioctl(ppp_chan_fd, PPPIOCCONNECT, &ifunit);

(note the missing '&' in the patch).

> +        if (ret < 0) {
> +                close(ppp_if_fd);
>                  return -errno;
>          }
> -        return 0;
> +
> +        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.
> +
> +  - Bridging L2TP sessions which have PPP pseudowire types (this is also called
> +    L2TP tunnel switching or L2TP multihop) is supported by bridging the ppp
> +    channels of the two L2TP sessions to be bridged::

s/ppp/PPP/ for the indented block here.

The device name `ppp<ifunit>` should be lowercase still since that's
what the actual device name will be.

> +
> +        /* Input: the session PPPoX data sockets session_fd1 and session_fd2
> +         * which were created as described further above. */
> +

Again, I think this comment should adhere to the kernel coding style.

Do we need backticks for `session_fd1` and `session_fd2` for consistency?

> +        int chindx1;
> +        int chindx2;
> +        int ppp_chan_fd;
> +

netdev-style Christmas-tree declaration ordering here too please.

> +        ret = ioctl(session_fd1, PPPIOCGCHAN, &chindx1);
> +        if (ret < 0)
> +                return -errno;
> +
> +        ret = ioctl(session_fd2, PPPIOCGCHAN, &chindx2);
> +        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, &chindx1);
> +        if (ret < 0) {
> +                close(ppp_chan_fd);
> +                return -errno;
> +        }

I think we should drop the PPPIOCATTCHAN ioctl call here.

The input file descriptors are called out as being PPPoX sockets
created as described earlier, in which case they should both
already be attached to a channel.

It would make more sense IMO to call out the two ppp_chan_fd file
descriptors as being input parameters alongside the PPPoX session file
descriptors.

> +
> +        ret = ioctl(ppp_chan_fd, PPPIOCBRIDGECHAN, &chindx2);
> +        close(ppp_chan_fd);
> +        if (ret < 0)
> +                return -errno;
> +
> +It can be noted that in this case no PPP interface is needed, and the PPP
> +channel does not need to be kept open.  Only the session PPPoX data sockets need
> +to be kept open.

Is it true to say that the PPP channel file descriptors can be closed
by userspace?  I'd need to re-review the ppp_generic.c refcounting to
be sure -- maybe you have some code which proves this assertion?

The user of PPPIOCBRIDGECHAN that I'm familiar with (go-l2tp) keeps
the PPP channel fds for the PPPoE and PPPoL2TP channels around for the
duration of the connection.  Possibly it's not necessary, but I'd
prefer we're totally sure before we call it out in the docs.

Maybe we could say something like this:

    "When bridging PPP channels, the PPP session is not locally terminated,
     and no local PPP device is created.  PPP frames arriving on one channel
     are directly passed to the other channel, and vice versa."

?

> +
> +More generally, it is also possible in the same way to e.g. bridge a PPPol2tp
> +ppp channel with other types of ppp channels, such as PPPoE.

s/PPPol2tp/PPPoL2TP/

s/ppp/PPP/

> +
> +See more details for the PPP side in ppp_generic.rst.
>  
>  Old L2TPv2-only API
>  -------------------

Thanks again for your work.  I know the above is all quite nit-picky
on the whole, so apologies -- but that's probably a sign we're getting
close :-)

Tom
  

Tom Parkin, le mar. 13 févr. 2024 10:51:08 +0000, a ecrit:
> > +        ret = ioctl(session_fd1, PPPIOCGCHAN, &chindx1);
> > +        if (ret < 0)
> > +                return -errno;
> > +
> > +        ret = ioctl(session_fd2, PPPIOCGCHAN, &chindx2);
> > +        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, &chindx1);
> > +        if (ret < 0) {
> > +                close(ppp_chan_fd);
> > +                return -errno;
> > +        }
> 
> I think we should drop the PPPIOCATTCHAN ioctl call here.
> 
> The input file descriptors are called out as being PPPoX sockets
> created as described earlier, in which case they should both
> already be attached to a channel.
> 
> It would make more sense IMO to call out the two ppp_chan_fd file
> descriptors as being input parameters alongside the PPPoX session file
> descriptors.
> 
> > +
> > +        ret = ioctl(ppp_chan_fd, PPPIOCBRIDGECHAN, &chindx2);
> > +        close(ppp_chan_fd);
> > +        if (ret < 0)
> > +                return -errno;
> > +
> > +It can be noted that in this case no PPP interface is needed, and the PPP
> > +channel does not need to be kept open.  Only the session PPPoX data sockets need
> > +to be kept open.
> 
> Is it true to say that the PPP channel file descriptors can be closed
> by userspace?

In our code we do it
https://code.ffdn.org/sthibaul/l2tpns/-/blob/kernel/l2tpns.c?ref_type=heads#L1295
and it works all fine indeed (and avoids that fd per session).

That's actually one of the reason why I made the snipped only take the
pppox sockets, and make it create the ppp chan fd only temporarily. AIUI
the pppox socket already has a ppp chan (returned by PPPIOCGCHAN), and
the ppp chan fd is there only for performing the bridging ioctl.

Samuel
  

On  Tue, Feb 13, 2024 at 12:53:04 +0100, Samuel Thibault wrote:
> Tom Parkin, le mar. 13 févr. 2024 10:51:08 +0000, a ecrit:
> > > +        ret = ioctl(session_fd1, PPPIOCGCHAN, &chindx1);
> > > +        if (ret < 0)
> > > +                return -errno;
> > > +
> > > +        ret = ioctl(session_fd2, PPPIOCGCHAN, &chindx2);
> > > +        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, &chindx1);
> > > +        if (ret < 0) {
> > > +                close(ppp_chan_fd);
> > > +                return -errno;
> > > +        }
> > 
> > I think we should drop the PPPIOCATTCHAN ioctl call here.
> > 
> > The input file descriptors are called out as being PPPoX sockets
> > created as described earlier, in which case they should both
> > already be attached to a channel.
> > 
> > It would make more sense IMO to call out the two ppp_chan_fd file
> > descriptors as being input parameters alongside the PPPoX session file
> > descriptors.
> > 
> > > +
> > > +        ret = ioctl(ppp_chan_fd, PPPIOCBRIDGECHAN, &chindx2);
> > > +        close(ppp_chan_fd);
> > > +        if (ret < 0)
> > > +                return -errno;
> > > +
> > > +It can be noted that in this case no PPP interface is needed, and the PPP
> > > +channel does not need to be kept open.  Only the session PPPoX data sockets need
> > > +to be kept open.
> > 
> > Is it true to say that the PPP channel file descriptors can be closed
> > by userspace?
> 
> In our code we do it
> https://code.ffdn.org/sthibaul/l2tpns/-/blob/kernel/l2tpns.c?ref_type=heads#L1295
> and it works all fine indeed (and avoids that fd per session).
> 
> That's actually one of the reason why I made the snipped only take the
> pppox sockets, and make it create the ppp chan fd only temporarily. AIUI
> the pppox socket already has a ppp chan (returned by PPPIOCGCHAN), and
> the ppp chan fd is there only for performing the bridging ioctl.

Thanks for the code reference -- that makes it clearer.  And I'm glad
someone (else) is using PPPIOCBRIDGECHAN :-)

It's a while since I was looking in ppp_generic.c and you're right
about the ppp channel fd.