| Message ID | 20230416220704.xqk4q6uwjbujnqpv@begin (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | PPPoL2TP: Add more code snippets | expand |
Samuel Thibault wrote on Mon, Apr 17, 2023 at 12:07:04AM +0200: > 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 described, so that people were thinking > it was not supported, while it actually is. > > Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org> Just passing by, thanks! The update made me ask a couple more questions so I've commented below, but I think this is already useful in itself so don't hold back for me. > --- > Documentation/networking/l2tp.rst | 59 ++++++++++++++++++++++++++++++++++++-- > 1 file changed, 56 insertions(+), 3 deletions(-) > > --- a/Documentation/networking/l2tp.rst > +++ b/Documentation/networking/l2tp.rst > @@ -387,11 +387,12 @@ Sample userspace code: > - Create session PPPoX data socket:: > > struct sockaddr_pppol2tp sax; > - int fd; > + int ret; > > /* Note, the tunnel socket must be bound already, else it > * will not be ready > */ > + int session_fd = socket(AF_PPPOX, SOCK_DGRAM, PX_PROTO_OL2TP); > sax.sa_family = AF_PPPOX; > sax.sa_protocol = PX_PROTO_OL2TP; > sax.pppol2tp.fd = tunnel_fd; > @@ -406,12 +407,64 @@ 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 ) { > return -errno; > } > return 0; > > + - Create PPP channel:: > + > + int chindx; > + ret = ioctl(session_fd, PPPIOCGCHAN, &chindx); > + if (ret < 0) > + return -errno; > + > + int ppp_chan_fd = open("/dev/ppp", O_RDWR); > + > + ret = ioctl(ppp_chan_fd, PPPIOCATTCHAN, &chindx); > + if (ret < 0) > + return -errno; > + > +Non-data PPP frames will be available for read on `ppp_chan_fd`. > + > + - Create PPP interface:: > + > + int ppp_if_fd = open("/dev/ppp", O_RDWR); > + > + int ifunit; > + ret = ioctl(ppp_if_fd, PPPIOCNEWUNIT, &ifunit); > + if (ret < 0) > + return -errno; > + > + ret = ioctl(ppp_chan_fd, PPPIOCCONNECT, ifunit); > + if (ret < 0) > + return -errno; > + > +The ppp<ifunit> interface can then be configured as usual with SIOCSIFMTU, > +SIOCSIFADDR, SIOCSIFDSTADDR, SIOCSIFNETMASK, and activated by setting IFF_UP > +with SIOCSIFFLAGS (That somewhat makes it sounds like the "new" netlink interface cannot be used (e.g. ip command); although I guess sommeone implementing this would be more likely to use the ioctls than not so having the names can be a timesaver?) Also, this got me wondering if the 'if' fd can be closed immediately or if the interface will be removed when the fd is closed (probably not?) If the fd can immediately be closed, could the chan fd or another fd be used instead, saving an open? > + > + - Tunnel switching is supported by bridging channels:: > + > + int chindx; > + ret = ioctl(session_fd, PPPIOCGCHAN, &chindx); > + if (ret < 0) > + return -errno; > + > + int chindx2; > + ret = ioctl(session_fd2, PPPIOCGCHAN, &chind2x); > + if (ret < 0) > + return -errno; > + > + int ppp_chan_fd = open("/dev/ppp", O_RDWR); > + > + ret = ioctl(ppp_chan_fd, PPPIOCBRIDGECHAN, &chindx2); > + if (ret < 0) > + return -errno; > + > + close(ppp_chan_fd); > + > Old L2TPv2-only API > ------------------- >
Dominique Martinet, le lun. 17 avril 2023 07:26:41 +0900, a ecrit: > > +The ppp<ifunit> interface can then be configured as usual with SIOCSIFMTU, > > +SIOCSIFADDR, SIOCSIFDSTADDR, SIOCSIFNETMASK, and activated by setting IFF_UP > > +with SIOCSIFFLAGS > > (That somewhat makes it sounds like the "new" netlink interface cannot > be used (e.g. ip command); Ah, right... > although I guess sommeone implementing this would be more likely to > use the ioctls than not so having the names can be a timesaver?) It's indeed a timesaver to have the ioctl names, but perhaps we can replace this part with a pointer to a if-configuration documentation? > Also, this got me wondering if the 'if' fd can be closed immediately or > if the interface will be removed when the fd is closed (probably not?) Closing the fd would close the if, yes. AIUI one really has to keep the pppox socket (for stats), the ppp chan (for non-data ppp packets), and the ppp if (for the if). Samuel
On Mon, Apr 17, 2023 at 12:43:16AM +0200, Samuel Thibault wrote: > Dominique Martinet, le lun. 17 avril 2023 07:26:41 +0900, a ecrit: > > Also, this got me wondering if the 'if' fd can be closed immediately or > > if the interface will be removed when the fd is closed (probably not?) > > Closing the fd would close the if, yes. AIUI one really has to keep the > pppox socket (for stats), the ppp chan (for non-data ppp packets), and > the ppp if (for the if). L2TP has control and data packets. The L2TP socket is there to handle L2TP control packets in user space, as the kernel only handles L2TP data packets. You have to keep the L2TP session socket open, otherwise you can't handle the session anymore. Then there are the PPP file descriptors. A PPP channel is used to send and receive PPP frames. It has to be associated with a lower transport, for example an L2TP session if you want to encapsulate PPP into L2TP. But that could be something else (a PPPoE session, a serial link, etc.). Same as for L2TP session sockets, you need to keep the PPP channel fd open, otherwise you can't handle the PPP session anymore. Finally there are PPP units. A PPP unit represents the PPP networking device (like ppp0). A PPP unit has to be associated with a PPP channel (or several PPP channels for multilink PPP, but better avoid that). A PPP unit doesn't know how to send packets, it delegates that to its PPP channels. You can avoid creating PPP units if you don't need a PPP network device. In particular, if you're forwarding a PPP session between a PPPoE and an L2TP session (or between two different L2TP sesions), you typically don't need to create any PPP unit. You handle the initial LCP negociation and PPP authentication using a PPP channel on the incoming side, then you create another PPP channel on the other side (where you want to forward the incoming PPP session) and finally bridge them together with PPPIOCBRIDGECHAN. > Samuel >
On Mon, Apr 17, 2023 at 07:26:41AM +0900, Dominique Martinet wrote: > Samuel Thibault wrote on Mon, Apr 17, 2023 at 12:07:04AM +0200: > (That somewhat makes it sounds like the "new" netlink interface cannot > be used (e.g. ip command); although I guess sommeone implementing this > would be more likely to use the ioctls than not so having the names can > be a timesaver?) I don't understand what you mean by 'the "new" netlink interface'. You can create a PPP interface either with the PPPIOCNEWUNIT ioctl or with netlink. But no matter how you create it, you need a /dev/ppp file descriptor associated to the PPP network device. Other than that, and no matter how you create them, PPP network devices can be used and configured like any other network interface. You absolutely can use "ip link" to manage you ppp interfaces.
On Mon, Apr 17, 2023 at 12:07:04AM +0200, 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 described, so that people were thinking > it was not supported, while it actually is. > > Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org> > > --- > Documentation/networking/l2tp.rst | 59 ++++++++++++++++++++++++++++++++++++-- > 1 file changed, 56 insertions(+), 3 deletions(-) > > --- a/Documentation/networking/l2tp.rst > +++ b/Documentation/networking/l2tp.rst > @@ -387,11 +387,12 @@ Sample userspace code: > - Create session PPPoX data socket:: > > struct sockaddr_pppol2tp sax; > - int fd; > + int ret; > > /* Note, the tunnel socket must be bound already, else it > * will not be ready > */ > + int session_fd = socket(AF_PPPOX, SOCK_DGRAM, PX_PROTO_OL2TP); Please declare session_fd with the other variables. Also, check the return value of the socket() call. > sax.sa_family = AF_PPPOX; > sax.sa_protocol = PX_PROTO_OL2TP; > sax.pppol2tp.fd = tunnel_fd; > @@ -406,12 +407,64 @@ 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 ) { Now you also need to close session_fd. > return -errno; > } > return 0; > > + - Create PPP channel:: > + > + int chindx; > + ret = ioctl(session_fd, PPPIOCGCHAN, &chindx); > + if (ret < 0) > + return -errno; > + > + int ppp_chan_fd = open("/dev/ppp", O_RDWR); > + > + ret = ioctl(ppp_chan_fd, PPPIOCATTCHAN, &chindx); > + if (ret < 0) > + return -errno; > + > +Non-data PPP frames will be available for read on `ppp_chan_fd`. > + > + - Create PPP interface:: > + > + int ppp_if_fd = open("/dev/ppp", O_RDWR); Check for errors please. > + > + int ifunit; Also, keep kernel style formatting: * All variable declarations in one block (ordered from longest to shortest line). * New line between variable declarations and code. > + ret = ioctl(ppp_if_fd, PPPIOCNEWUNIT, &ifunit); You need to initialise ifunit first. Use -1 to let the kernel pick a free unit index. > + if (ret < 0) > + return -errno; > + > + ret = ioctl(ppp_chan_fd, PPPIOCCONNECT, ifunit); > + if (ret < 0) > + return -errno; > + > +The ppp<ifunit> interface can then be configured as usual with SIOCSIFMTU, > +SIOCSIFADDR, SIOCSIFDSTADDR, SIOCSIFNETMASK, and activated by setting IFF_UP > +with SIOCSIFFLAGS > + > + - Tunnel switching is supported by bridging channels:: This is a PPP feature not an L2TP one. PPPIOCBRIDGECHAN's description belongs to Documentation/networking/ppp_generic.rst, where it's already documented. If documentation needs to be improved, that should be done there. If necessary, you can link to ppp_generic.rst here. Also, calling this feature 'tunnel switching' is misleading. Switching happens between L2TP sessions (or more generally between any PPP transports), not between L2TP tunnels (which are just L2TP session multiplexers).
Guillaume Nault, le mar. 18 avril 2023 10:34:03 +0200, a ecrit: > On Mon, Apr 17, 2023 at 12:07:04AM +0200, Samuel Thibault wrote: > > sax.sa_family = AF_PPPOX; > > sax.sa_protocol = PX_PROTO_OL2TP; > > sax.pppol2tp.fd = tunnel_fd; > > @@ -406,12 +407,64 @@ 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 ) { > > Now you also need to close session_fd. ? No, we need it for PPPIOCGCHAN, and also PPPIOCGL2TPSTATS. I'll put return session_fd instead. > > +The ppp<ifunit> interface can then be configured as usual with SIOCSIFMTU, > > +SIOCSIFADDR, SIOCSIFDSTADDR, SIOCSIFNETMASK, and activated by setting IFF_UP > > +with SIOCSIFFLAGS > > + > > + - Tunnel switching is supported by bridging channels:: > > This is a PPP feature not an L2TP one. > > PPPIOCBRIDGECHAN's description > belongs to Documentation/networking/ppp_generic.rst, where it's already > documented. Yes but that's hard to find out when you're looking from the L2TP end. > If necessary, you can link to ppp_generic.rst here. > > Also, calling this feature 'tunnel switching' is misleading. That's how I have seen it is called in L2TP jargon. Samuel
On Tue, Apr 18, 2023 at 10:53:23AM +0200, Samuel Thibault wrote: > Guillaume Nault, le mar. 18 avril 2023 10:34:03 +0200, a ecrit: > > On Mon, Apr 17, 2023 at 12:07:04AM +0200, Samuel Thibault wrote: > > > sax.sa_family = AF_PPPOX; > > > sax.sa_protocol = PX_PROTO_OL2TP; > > > sax.pppol2tp.fd = tunnel_fd; > > > @@ -406,12 +407,64 @@ 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 ) { > > > > Now you also need to close session_fd. > > ? No, we need it for PPPIOCGCHAN, and also PPPIOCGL2TPSTATS. connect() failed. You can't do anything with this socket. > I'll put return session_fd instead. What's the point of returning session_fd if connect() failed? How will the caller know if session_fd is connected or not? Why would it even be interested in a half-created session fd? > > > +The ppp<ifunit> interface can then be configured as usual with SIOCSIFMTU, > > > +SIOCSIFADDR, SIOCSIFDSTADDR, SIOCSIFNETMASK, and activated by setting IFF_UP > > > +with SIOCSIFFLAGS > > > + > > > + - Tunnel switching is supported by bridging channels:: > > > > This is a PPP feature not an L2TP one. > > > > PPPIOCBRIDGECHAN's description > > belongs to Documentation/networking/ppp_generic.rst, where it's already > > documented. > > Yes but that's hard to find out when you're looking from the L2TP end. That's why I proposed linking to ppp_generic.rst. > > If necessary, you can link to ppp_generic.rst here. > > > > Also, calling this feature 'tunnel switching' is misleading. > > That's how I have seen it is called in L2TP jargon. That still doesn't describe the kernel feature. We can add a 'so called "tunnel switching" in L2TP jargon' into parenthesis to give a hint to the people using this terminology. > Samuel >
Guillaume Nault, le mar. 18 avril 2023 11:06:51 +0200, a ecrit: > On Tue, Apr 18, 2023 at 10:53:23AM +0200, Samuel Thibault wrote: > > Guillaume Nault, le mar. 18 avril 2023 10:34:03 +0200, a ecrit: > > > On Mon, Apr 17, 2023 at 12:07:04AM +0200, Samuel Thibault wrote: > > > > sax.sa_family = AF_PPPOX; > > > > sax.sa_protocol = PX_PROTO_OL2TP; > > > > sax.pppol2tp.fd = tunnel_fd; > > > > @@ -406,12 +407,64 @@ 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 ) { > > > > > > Now you also need to close session_fd. > > > > ? No, we need it for PPPIOCGCHAN, and also PPPIOCGL2TPSTATS. > > connect() failed. You can't do anything with this socket. Ah, you were talking about the failure case, ok. > > > > +The ppp<ifunit> interface can then be configured as usual with SIOCSIFMTU, > > > > +SIOCSIFADDR, SIOCSIFDSTADDR, SIOCSIFNETMASK, and activated by setting IFF_UP > > > > +with SIOCSIFFLAGS > > > > + > > > > + - Tunnel switching is supported by bridging channels:: > > > > > > This is a PPP feature not an L2TP one. > > > > > > PPPIOCBRIDGECHAN's description > > > belongs to Documentation/networking/ppp_generic.rst, where it's already > > > documented. > > > > Yes but that's hard to find out when you're looking from the L2TP end. > > That's why I proposed linking to ppp_generic.rst. Yes, but it's still not obvious to L2TP people that it's a ppp channel that you have to bridge. Really, having that 20-line snippet available would have saved me some head-scratching time. Samuel
On Tue, Apr 18, 2023 at 11:11:48AM +0200, Samuel Thibault wrote: > Guillaume Nault, le mar. 18 avril 2023 11:06:51 +0200, a ecrit: > > On Tue, Apr 18, 2023 at 10:53:23AM +0200, Samuel Thibault wrote: > > > Guillaume Nault, le mar. 18 avril 2023 10:34:03 +0200, a ecrit: > > > > PPPIOCBRIDGECHAN's description > > > > belongs to Documentation/networking/ppp_generic.rst, where it's already > > > > documented. > > > > > > Yes but that's hard to find out when you're looking from the L2TP end. > > > > That's why I proposed linking to ppp_generic.rst. > > Yes, but it's still not obvious to L2TP people that it's a ppp channel > that you have to bridge. Really, having that 20-line snippet available > would have saved me some head-scratching time. But the reverse is also true: someone looking at the PPP documentation is probably not going to realise that PPP sample code have been put in the L2TP doc. We can instead add PPP specific code in the PPP documentation. Then add a line in l2tp.rst saying something like "Handling of the PPP layer is described in ppp_generic." If you feel like having sample code for a full example covering the whole process of creating and setting up the file descriptiors needed for bridging PPP channels (UDP, L2TP tunnels, L2TP sessions and PPP channels, and why not PPPoE), then I feel a specific file would be more indicated, as this would cross several different subsystems. It'd be okay to to have a few PPP-specific calls in l2tp.rst since L2TPv2 is tied to PPP and that could give a more complete example of how to use the L2TP uAPI. But that should be limitted to the most basic PPP features (creating a channel and a unit). > Samuel >
Guillaume Nault, le mar. 18 avril 2023 12:17:55 +0200, a ecrit: > On Tue, Apr 18, 2023 at 11:11:48AM +0200, Samuel Thibault wrote: > > Guillaume Nault, le mar. 18 avril 2023 11:06:51 +0200, a ecrit: > > > On Tue, Apr 18, 2023 at 10:53:23AM +0200, Samuel Thibault wrote: > > > > Guillaume Nault, le mar. 18 avril 2023 10:34:03 +0200, a ecrit: > > > > > PPPIOCBRIDGECHAN's description > > > > > belongs to Documentation/networking/ppp_generic.rst, where it's already > > > > > documented. > > > > > > > > Yes but that's hard to find out when you're looking from the L2TP end. > > > > > > That's why I proposed linking to ppp_generic.rst. > > > > Yes, but it's still not obvious to L2TP people that it's a ppp channel > > that you have to bridge. Really, having that 20-line snippet available > > would have saved me some head-scratching time. > > But the reverse is also true: someone looking at the PPP documentation > is probably not going to realise that PPP sample code have been put in > the L2TP doc. Yes, but for PPP people it is obvious that you'll want to bridge two channels. The point of the code is not really the bridging ioctl call, but the fact that you have to use PPPIOCGCHAN over the two sessions, then open a ppp channel, to be able to make the bridging ioctl call. *That* is what is really not obvious, and will not actually fit in the PPP documentation. Of course we could move the few ppp-only lines to the PPP documentation, but I really don't see the point: that part is obvious in the PPP context. Samuel
On Tue, Apr 18, 2023 at 12:31:40PM +0200, Samuel Thibault wrote: > Guillaume Nault, le mar. 18 avril 2023 12:17:55 +0200, a ecrit: > > On Tue, Apr 18, 2023 at 11:11:48AM +0200, Samuel Thibault wrote: > > > Guillaume Nault, le mar. 18 avril 2023 11:06:51 +0200, a ecrit: > > > > On Tue, Apr 18, 2023 at 10:53:23AM +0200, Samuel Thibault wrote: > > > > > Guillaume Nault, le mar. 18 avril 2023 10:34:03 +0200, a ecrit: > > > > > > PPPIOCBRIDGECHAN's description > > > > > > belongs to Documentation/networking/ppp_generic.rst, where it's already > > > > > > documented. > > > > > > > > > > Yes but that's hard to find out when you're looking from the L2TP end. > > > > > > > > That's why I proposed linking to ppp_generic.rst. > > > > > > Yes, but it's still not obvious to L2TP people that it's a ppp channel > > > that you have to bridge. Really, having that 20-line snippet available > > > would have saved me some head-scratching time. > > > > But the reverse is also true: someone looking at the PPP documentation > > is probably not going to realise that PPP sample code have been put in > > the L2TP doc. > > Yes, but for PPP people it is obvious that you'll want to bridge two > channels. > > The point of the code is not really the bridging ioctl call, but the > fact that you have to use PPPIOCGCHAN over the two sessions, then open > a ppp channel, to be able to make the bridging ioctl call. *That* > is what is really not obvious, and will not actually fit in the PPP > documentation. Of course we could move the few ppp-only lines to the PPP > documentation, but I really don't see the point: that part is obvious in > the PPP context. For PPPIOCGCHAN, I agree it should be documented in l2tp.rst. This ioctl is common to all PPPOX sockets, but it wouldn't make sense to have a separate document just for it. And L2TP is the only PPPOX user that is documented as far as I know. As I said in my previous reply, a simple L2TP example that goes until PPP channel and unit creation is fine. But any more advanced use of the PPP API should be documented in the PPP documentation. I mean, these files document the API of their corresponding modules, their scope should be limitted to that (the PPP and L2TP layers are really different). That shouldn't preclude anyone from describing how to combine L2TP, PPP and others to cover more advanced use cases. It's just better done in a different file. > Samuel >
Guillaume Nault, le mar. 18 avril 2023 13:25:38 +0200, a ecrit: > As I said in my previous reply, a simple L2TP example that goes until PPP > channel and unit creation is fine. But any more advanced use of the PPP > API should be documented in the PPP documentation. When it's really advanced, yes. But here it's just about tunnel bridging, which is a very common L2TP thing to do. > I mean, these files document the API of their corresponding modules, > their scope should be limitted to that (the PPP and L2TP layers are > really different). I wouldn't call + ret = ioctl(ppp_chan_fd, PPPIOCBRIDGECHAN, &chindx2); + close(ppp_chan_fd); + if (ret < 0) + return -errno; documentation... > That shouldn't preclude anyone from describing how to combine L2TP, PPP > and others to cover more advanced use cases. It's just better done in a > different file. A more complete example, yes. I don't plan on taking time to do it. Samuel
On Tue, Apr 18, 2023 at 01:54:09PM +0200, Samuel Thibault wrote: > Guillaume Nault, le mar. 18 avril 2023 13:25:38 +0200, a ecrit: > > As I said in my previous reply, a simple L2TP example that goes until PPP > > channel and unit creation is fine. But any more advanced use of the PPP > > API should be documented in the PPP documentation. > > When it's really advanced, yes. But here it's just about tunnel > bridging, which is a very common L2TP thing to do. I can't undestand why you absolutely want this covered in l2tp.rst. This feature also works on PPPoE. Also, it's probably a desirable feature, but certainly not a common thing on Linux. This interface was added a bit more than 2 years ago, which is really recent considering the age of the code. Appart from maybe go-l2tp, I don't know of any user. > > I mean, these files document the API of their corresponding modules, > > their scope should be limitted to that (the PPP and L2TP layers are > > really different). > > I wouldn't call > > + ret = ioctl(ppp_chan_fd, PPPIOCBRIDGECHAN, &chindx2); > + close(ppp_chan_fd); > + if (ret < 0) > + return -errno; > > documentation... The documentation is in ppp_generic.rst. Does it really make sense to you to have the doc there and the sample code in l2tp.rst? Anyway, I'm not going to argue any longer. You have my opinion. You're always free to ignore feedbacks. > > That shouldn't preclude anyone from describing how to combine L2TP, PPP > > and others to cover more advanced use cases. It's just better done in a > > different file. > > A more complete example, yes. I don't plan on taking time to do it. > > Samuel >
Guillaume Nault, le mar. 18 avril 2023 15:38:00 +0200, a ecrit: > On Tue, Apr 18, 2023 at 01:54:09PM +0200, Samuel Thibault wrote: > > Guillaume Nault, le mar. 18 avril 2023 13:25:38 +0200, a ecrit: > > > As I said in my previous reply, a simple L2TP example that goes until PPP > > > channel and unit creation is fine. But any more advanced use of the PPP > > > API should be documented in the PPP documentation. > > > > When it's really advanced, yes. But here it's just about tunnel > > bridging, which is a very common L2TP thing to do. > > I can't undestand why you absolutely want this covered in l2tp.rst. Because that's where people working on L2TP software will look for it. > This feature also works on PPPoE. Then PPPoE documentation shouls also contain mentions of it. Note (and I'll repeat myself again below) I'm not talking about *documentation* (which belongs to ppp_generic.rst, but *mentions* of it. Networking is complex. If each protocol only speaks for itself and doesn't take the effort of showing how they glue with others, it's hard for people to grasp the whole ideas. > Also, it's probably a desirable feature, but certainly not a common > thing on Linux. This interface was added a bit more than 2 years ago, > which is really recent considering the age of the code. Yes, and in ISPs we have been in need for it for something like decades. I can find RFC drafts around 2000. Or IPs have just baked their own kernel implementation (xl2tpd, accel-ppp, etc.) > Appart from maybe go-l2tp, I don't know of any user. Because it's basically undocumented from the point of view of L2TP people. > > > I mean, these files document the API of their corresponding modules, > > > their scope should be limitted to that (the PPP and L2TP layers are > > > really different). > > > > I wouldn't call > > > > + ret = ioctl(ppp_chan_fd, PPPIOCBRIDGECHAN, &chindx2); > > + close(ppp_chan_fd); > > + if (ret < 0) > > + return -errno; > > > > documentation... > > The documentation is in ppp_generic.rst. Yes. and I *definitely* agree on that, and exactly what I'm all talking about. I'm here just arguing about putting these _*_4 lines of code_*_ example in l2tp.rst, _*_nothing more_*_. See the subject of this thread: "code snippets". Not documentation. > Does it really make sense to you to have the doc there There is basically no doc in what I am proposing. > and the sample code in l2tp.rst? Yes, because then L2TP people can be sure to understand how things plug altogether before writing code... ... instead of having to try various things until understanding how it's all supposed to fit altogether. Samuel
On Tue, Apr 18, 2023 at 16:18:20 +0200, Samuel Thibault wrote: > Guillaume Nault, le mar. 18 avril 2023 15:38:00 +0200, a ecrit: > > On Tue, Apr 18, 2023 at 01:54:09PM +0200, Samuel Thibault wrote: > > > Guillaume Nault, le mar. 18 avril 2023 13:25:38 +0200, a ecrit: > > > > As I said in my previous reply, a simple L2TP example that goes until PPP > > > > channel and unit creation is fine. But any more advanced use of the PPP > > > > API should be documented in the PPP documentation. > > > > > > When it's really advanced, yes. But here it's just about tunnel > > > bridging, which is a very common L2TP thing to do. > > > > I can't undestand why you absolutely want this covered in l2tp.rst. > > Because that's where people working on L2TP software will look for it. Sorry to have not commented earlier, and thank you Samuel for working on improving the L2TP documentation. I think documentation like l2tp.rst is best when it provides a high level overview of how things fit together. When it comes to actually implementing a userspace L2TP/PPP daemon, I feel that at a certain point you're better off referring to existing userspace code alongside the kernel sources themselves, as any summary is inevitably going to leave gaps. From that perspective I'd almost sooner we didn't have the code snippet in l2tp.rst. That said, I can't see the harm in improving the code snippet, given that we have it already. Having no mention of PPPIOCBRIDGECHAN given that it can be used to implement tunnel switching is an oversight really. FWIW I agree the term "tunnel switching" is a bit misleading, and of course the PPP ioctl supports bridging any flavour of channel, not just PPPoL2TP. However from the L2TP perspective people perhaps have something along the lines of this IETF draft in mind: https://datatracker.ietf.org/doc/html/draft-ietf-l2tpext-tunnel-switching-08 ...which we could perhaps link to to clarify the intent in the context of the L2TP codebase? > > Also, it's probably a desirable feature, but certainly not a common > > thing on Linux. This interface was added a bit more than 2 years ago, > > which is really recent considering the age of the code. > > Yes, and in ISPs we have been in need for it for something like > decades. I can find RFC drafts around 2000. > > Or IPs have just baked their own kernel implementation (xl2tpd, > accel-ppp, etc.) Yes. It's sad that support wasn't available sooner in the kernel, but I'm not sure that's indicative of lack of desire for the feature necessarily. > > Appart from maybe go-l2tp, I don't know of any user. > I confirm that go-l2tp does use it :-)
--- a/Documentation/networking/l2tp.rst +++ b/Documentation/networking/l2tp.rst @@ -387,11 +387,12 @@ Sample userspace code: - Create session PPPoX data socket:: struct sockaddr_pppol2tp sax; - int fd; + int ret; /* Note, the tunnel socket must be bound already, else it * will not be ready */ + int session_fd = socket(AF_PPPOX, SOCK_DGRAM, PX_PROTO_OL2TP); sax.sa_family = AF_PPPOX; sax.sa_protocol = PX_PROTO_OL2TP; sax.pppol2tp.fd = tunnel_fd; @@ -406,12 +407,64 @@ 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 ) { return -errno; } return 0; + - Create PPP channel:: + + int chindx; + ret = ioctl(session_fd, PPPIOCGCHAN, &chindx); + if (ret < 0) + return -errno; + + int ppp_chan_fd = open("/dev/ppp", O_RDWR); + + ret = ioctl(ppp_chan_fd, PPPIOCATTCHAN, &chindx); + if (ret < 0) + return -errno; + +Non-data PPP frames will be available for read on `ppp_chan_fd`. + + - Create PPP interface:: + + int ppp_if_fd = open("/dev/ppp", O_RDWR); + + int ifunit; + ret = ioctl(ppp_if_fd, PPPIOCNEWUNIT, &ifunit); + if (ret < 0) + return -errno; + + ret = ioctl(ppp_chan_fd, PPPIOCCONNECT, ifunit); + if (ret < 0) + return -errno; + +The ppp<ifunit> interface can then be configured as usual with SIOCSIFMTU, +SIOCSIFADDR, SIOCSIFDSTADDR, SIOCSIFNETMASK, and activated by setting IFF_UP +with SIOCSIFFLAGS + + - Tunnel switching is supported by bridging channels:: + + int chindx; + ret = ioctl(session_fd, PPPIOCGCHAN, &chindx); + if (ret < 0) + return -errno; + + int chindx2; + ret = ioctl(session_fd2, PPPIOCGCHAN, &chind2x); + if (ret < 0) + return -errno; + + int ppp_chan_fd = open("/dev/ppp", O_RDWR); + + ret = ioctl(ppp_chan_fd, PPPIOCBRIDGECHAN, &chindx2); + if (ret < 0) + return -errno; + + close(ppp_chan_fd); + 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 described, so that people were thinking it was not supported, while it actually is. Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org> --- Documentation/networking/l2tp.rst | 59 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 56 insertions(+), 3 deletions(-)