Re: [PATCHv4] PPPoL2TP: Add more code snippets

[Tom Parkin]

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
Katalix Systems Ltd
https://katalix.com
Catalysts for your Embedded Linux software development

Attachment: signature.asc
Description: PGP signature