| Message ID | 20240203223513.f2nfgaamgffz6dno@begin (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [PATCHv3] PPPoL2TP: Add more code snippets | expand |
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
--- 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) + 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; + +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:: + + 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); + if (ret < 0) + return -errno; + + ppp_chan_fd = open("/dev/ppp", O_RDWR); + 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. 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 Documentation/networking/l2tp.rst | 99 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 95 insertions(+), 4 deletions(-)