| Message ID | 20240212222344.xtv233r5sixme32h@begin (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [PATCHv4] PPPoL2TP: Add more code snippets | expand |
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.
--- 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. */ + 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. */ + + 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:: + + /* Input: the PPP channel ppp_chan_fd which was created as described + * above */ + + int ppp_if_fd; + int ifunit = -1; + + 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); + 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:: + + /* Input: the session PPPoX data sockets session_fd1 and session_fd2 + * which were created as described further above. */ + + int chindx1; + int chindx2; + int ppp_chan_fd; + + 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; + } + + 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. + +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. + +See more details for the PPP side in ppp_generic.rst. Old L2TPv2-only API -------------------
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