On Tue, May 26, 2020 at 11:31 AM Claudio Jeker <cje...@diehard.n-r-g.com> wrote:
> On Tue, May 26, 2020 at 09:22:28AM +0200, Martin Pieuchot wrote:
> > On 25/05/20(Mon) 21:42, Sergey Ryazanov wrote:
> > > Add dedicated option to activate kernel L2TP acceleration via
> > > the pipex(4). The options should be passed by a L2TP tunnel
> > > management daemon (e.g. xl2tpd).
> >
> > What is the difference between npppd(8) and pppd(8)? Aren't those two
> > redundant? Why did you choose to modify pppd(8) and not npppd(8)?
>
> npppd(8) is server only it can not establish a connection. pppd(8) on the
> other hand is more client side (but I think it can do both).
True.
> I think the split in userland makes sense (using npppd as LNS/LAC and pppd
> to connect to a ISP via PPP (even though we also use sppp/pppoe(4) for
> that). Now for the kernel side it would be great if the interface used
> would be the same for all ppp users. In this regard this is a step in the
> right direction.
Comparing addition of client support to npppd and reworking pppd to
support pppx(4), I will choose the npppd reworking. The pppd code is
the mess that is too tightly connected with TTY.
Moreover, pppd is designed to establish the PPP connection over any
type of serial line, including RS232 connected Dial-Up modem. So
introducing the pppx(4) support will require preserving of all
existing code *and* creation of a big module that is responsible
pppx(4) handling in case of L2TP connection. And such feature will
require a hard xl2tpd daemon reworking too. It is easier to rework
npppd or ever create a new L2TP client daemon from the scratch.
So I found a 3rd solution - add a pipex(4) support for the good
old-fashioned ppp(4) and a tiny option for pppd. And this my plan if
someone do not have a better solution.
> > > diff --git usr.sbin/pppd/ipcp.c usr.sbin/pppd/ipcp.c
> > > index 1296d897b14..e7a6dbd6ee7 100644
> > > --- usr.sbin/pppd/ipcp.c
> > > +++ usr.sbin/pppd/ipcp.c
> > > @@ -1251,6 +1251,10 @@ ipcp_up(f)
> > > return;
> > > }
> > >
> > > + /* enable pipex(4), keep working if failed */
> > > + if (pipex_conf.pr_protocol && !apipex(go->ouraddr, ho->hisaddr, mask))
> > > + IPCPDEBUG((LOG_WARNING, "apipex failed"));
> > > +
> > > #if (defined(SVR4) && (defined(SNI) || defined(__USLC__)))
> > > if (!sifaddr(f->unit, go->ouraddr, ho->hisaddr, mask)) {
> > > IPCPDEBUG((LOG_WARNING, "sifaddr failed"));
> > > @@ -1304,6 +1308,7 @@ ipcp_down(f)
> > > if (demand) {
> > > sifnpmode(f->unit, PPP_IP, NPMODE_QUEUE);
> > > } else {
> > > + dpipex();
> > > sifdown(f->unit);
> > > ipcp_clear_addrs(f->unit);
> > > }
> > > diff --git usr.sbin/pppd/options.c usr.sbin/pppd/options.c
> > > index 828f7cdce65..fe0e2e6b54b 100644
> > > --- usr.sbin/pppd/options.c
> > > +++ usr.sbin/pppd/options.c
> > > @@ -144,6 +144,9 @@ struct bpf_program pass_filter;/* Filter program for
> > > packets to pass */
> > > struct bpf_program active_filter; /* Filter program for link-active
> > > pkts */
> > > pcap_t pc; /* Fake struct pcap so we can compile
> > > expr */
> > > #endif
> > > +struct pipex_session_req pipex_conf = { /* pipex(4) session
> > > configuration */
> > > + .pr_protocol = 0,
> > > +};
> > >
> > > /*
> > > * Prototypes
> > > @@ -253,6 +256,8 @@ static int setactivefilter(char **);
> > > static int setmslanman(char **);
> > > #endif
> > >
> > > +static int setpipexl2tp(char **);
> > > +
> > > static int number_option(char *, u_int32_t *, int);
> > > static int int_option(char *, int *);
> > > static int readable(int fd);
> > > @@ -391,6 +396,8 @@ static struct cmd {
> > > {"ms-lanman", 0, setmslanman}, /* Use LanMan psswd when using
> > > MS-CHAP */
> > > #endif
> > >
> > > + {"pipex-l2tp", 6, setpipexl2tp}, /* set pipex(4) L2TP
> > > parameters */
> > > +
> > > {NULL, 0, NULL}
> > > };
> > >
> > > @@ -2283,3 +2290,78 @@ setmslanman(argv)
> > > return (1);
> > > }
> > > #endif
> > > +
> > > +static int
> > > +inet_atoss(cp, ss)
> > > + char *cp;
> > > + struct sockaddr_storage *ss;
> > > +{
> > > + struct sockaddr_in *sin = (struct sockaddr_in *)ss;
> > > + char *c, *p;
> > > + unsigned port;
> > > +
> > > + if ((c = strchr(cp, ':')) == NULL)
> > > + return 0;
> > > + *c = '\0';
> > > + if (!inet_aton(cp, &sin->sin_addr))
> > > + return 0;
> > > + if (!(port = strtoul(c + 1, &p, 10)) || *p != '\0')
> > > + return 0;
> > > + sin->sin_port = htons(port);
> > > + sin->sin_family = AF_INET;
> > > + ss->ss_len = sizeof(*sin);
> > > +
> > > + return 1;
> > > +}
> > > +
> > > +static int
> > > +strtou16(str, valp)
> > > + char *str;
> > > + uint16_t *valp;
> > > +{
> > > + char *ptr;
> > > +
> > > + *valp = strtoul(str, &ptr, 0);
> > > +
> > > + return *ptr == '\0' ? 1 : 0;
> > > +}
> > > +
> > > +static int
> > > +setpipexl2tp(argv)
> > > + char **argv;
> > > +{
> > > + BZERO((char *)&pipex_conf, sizeof(pipex_conf));
> > > +
> > > + if (!inet_atoss(argv[0], &pipex_conf.pr_local_address)) {
> > > + option_error("pipex-l2tp: invalid local address of tunnel: %s",
> > > argv[0]);
> > > + return 0;
> > > + }
> > > + if (!inet_atoss(argv[1], &pipex_conf.pr_peer_address)) {
> > > + option_error("pipex-l2tp: invalid peer address of tunnel: %s",
> > > argv[1]);
> > > + return 0;
> > > + }
> > > + if (!strtou16(argv[2], &pipex_conf.pr_proto.l2tp.tunnel_id)) {
> > > + option_error("pipex-l2tp: invalid local tunnel id: %s", argv[2]);
> > > + return 0;
> > > + }
> > > + if (!strtou16(argv[3], &pipex_conf.pr_proto.l2tp.peer_tunnel_id)) {
> > > + option_error("pipex-l2tp: invalid peer tunnel id: %s", argv[3]);
> > > + return 0;
> > > + }
> > > + if (!strtou16(argv[4], &pipex_conf.pr_session_id)) {
> > > + option_error("pipex-l2tp: invalid local call id: %s", argv[4]);
> > > + return 0;
> > > + }
> > > + if (!strtou16(argv[5], &pipex_conf.pr_peer_session_id)) {
> > > + option_error("pipex-l2tp: invalid peer call id: %s", argv[5]);
> > > + return 0;
> > > + }
> > > +
> > > + /* Indicate address field presense */
> > > + pipex_conf.pr_ppp_flags = PIPEX_PPP_HAS_ACF;
> > > +
> > > + /* Finally set the protocol type to implicitly indicate config
> > > validity */
> > > + pipex_conf.pr_protocol = PIPEX_PROTO_L2TP;
> > > +
> > > + return 1;
> > > +}
> > > diff --git usr.sbin/pppd/pppd.8 usr.sbin/pppd/pppd.8
> > > index 5fba6f1714d..6a7f6e01c09 100644
> > > --- usr.sbin/pppd/pppd.8
> > > +++ usr.sbin/pppd/pppd.8
> > > @@ -829,6 +829,24 @@ option is used.
> > > .It Cm xonxoff
> > > Use software flow control (i.e., XON/XOFF) to control the flow of data on
> > > the serial port.
> > > +.It Cm pipex-l2tp Ar ltunaddr ptunaddr ltunid ptunid lcallid pcallid
> > > +OpenBSD specific. Activate and configure kernel L2TP acceleration. Set
> > > +pipex(4) local tunnel address to
> > > +.Ar ltunaddr ,
> > > +peer tunnel address to
> > > +.Ar ptunaddr ,
> > > +local L2TP tunnel id to
> > > +.Ar ltunid ,
> > > +peer L2TP tunnel id to
> > > +.Ar ptunid ,
> > > +local L2TP call id (local session id in terms of pipex(4)) to
> > > +.Ar lcallid
> > > +and peer L2TP call id (peer session id in terms of pipex(4)) to
> > > +.Ar pcallid .
> > > +This option should not be specified in the options file. Instead it
> > > +should be passed to
> > > +.Nm pppd
> > > +by a L2TP daemon, that is responsible for L2TP tunnel establishing.
> > > .El
> > > .Sh OPTIONS FILES
> > > Options can be taken from files as well as the command line.
> > > diff --git usr.sbin/pppd/pppd.h usr.sbin/pppd/pppd.h
> > > index 9cd332939a3..27ddc45bd78 100644
> > > --- usr.sbin/pppd/pppd.h
> > > +++ usr.sbin/pppd/pppd.h
> > > @@ -51,7 +51,10 @@
> > >
> > > #include <sys/types.h> /* for u_int32_t, if defined */
> > > #include <sys/time.h> /* for struct timeval */
> > > +#include <netinet/in.h>
> > > #include <net/ppp_defs.h>
> > > +#include <net/if.h>
> > > +#include <net/pipex.h>
> > > #include <stdio.h> /* for FILE */
> > > #include <stdarg.h>
> > >
> > > @@ -128,6 +131,7 @@ extern int refuse_chap; /* Don't wanna auth.
> > > ourselves with CHAP */
> > > extern struct bpf_program pass_filter; /* Filter for pkts to pass
> > > */
> > > extern struct bpf_program active_filter; /* Filter for link-active
> > > pkts */
> > > #endif
> > > +extern struct pipex_session_req pipex_conf; /* pipex(4) session
> > > configuration */
> > >
> > > #ifdef MSLANMAN
> > > extern int ms_lanman; /* Nonzero if use LanMan password instead of
> > > NT */
> > > @@ -306,6 +310,9 @@ int cifproxyarp(int, u_int32_t);
> > > u_int32_t GetMask(u_int32_t); /* Get appropriate netmask for
> > > address */
> > > int lock(char *); /* Create lock file for device */
> > > void unlock(void); /* Delete previously-created lock file */
> > > +int apipex(u_int32_t, u_int32_t, u_int32_t);
> > > + /* Add pipex(4) to interface */
> > > +void dpipex(void); /* Delete pipex(4) from interface */
> > > int daemon(int, int); /* Detach us from terminal session */
> > > void logwtmp(const char *, const char *, const char *);
> > > /* Write entry to wtmp file */
> > > diff --git usr.sbin/pppd/sys-bsd.c usr.sbin/pppd/sys-bsd.c
> > > index e8deee6d2ff..085888cf06f 100644
> > > --- usr.sbin/pppd/sys-bsd.c
> > > +++ usr.sbin/pppd/sys-bsd.c
> > > @@ -801,6 +801,15 @@ ppp_recv_config(unit, mru, asyncmap, pcomp, accomp)
> > > syslog(LOG_ERR, "ioctl(PPPIOCSFLAGS): %m");
> > > quit();
> > > }
> > > + /* Update pipex(4) config */
> > > + if (pcomp)
> > > + pipex_conf.pr_ppp_flags |= PIPEX_PPP_PFC_ACCEPTED;
> > > + else
> > > + pipex_conf.pr_ppp_flags &= ~PIPEX_PPP_PFC_ACCEPTED;
> > > + if (accomp)
> > > + pipex_conf.pr_ppp_flags |= PIPEX_PPP_ACFC_ACCEPTED;
> > > + else
> > > + pipex_conf.pr_ppp_flags &= ~PIPEX_PPP_ACFC_ACCEPTED;
> > > }
> > >
> > > /*
> > > @@ -1510,3 +1519,50 @@ unlock()
> > > lock_file = NULL;
> > > }
> > > }
> > > +
> > > +/*
> > > + * apipex - enable pipex(4) and add session
> > > + */
> > > +int
> > > +apipex(o, h, m)
> > > + u_int32_t o, h, m;
> > > +{
> > > + int mode = 1;
> > > +
> > > + if (ioctl(ttyfd, PIPEXSMODE, &mode) == -1) {
> > > + syslog(LOG_WARNING, "Couldn't enable pipex: %m");
> > > + return -1;
> > > + }
> > > +
> > > + /* Complete session creation request */
> > > + pipex_conf.pr_ip_srcaddr.s_addr = o;
> > > + pipex_conf.pr_ip_address.s_addr = h;
> > > + pipex_conf.pr_ip_netmask.s_addr = m;
> > > +
> > > + if (ioctl(ttyfd, PIPEXASESSION, &pipex_conf) == -1) {
> > > + syslog(LOG_WARNING, "Couldn't add pipex session: %m");
> > > + return -1;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/*
> > > + * dpipex - del session and disable pipex(4)
> > > + */
> > > +void
> > > +dpipex()
> > > +{
> > > + struct pipex_session_close_req cr;
> > > + int mode = 0;
> > > +
> > > + /* Copy required data from the session addition request */
> > > + cr.psr_protocol = pipex_conf.pr_protocol;
> > > + cr.psr_session_id = pipex_conf.pr_session_id;
> > > +
> > > + if (ioctl(ttyfd, PIPEXDSESSION, &cr) == -1)
> > > + syslog(LOG_WARNING, "Couldn't del pipex session: %m");
> > > +
> > > + if (ioctl(ttyfd, PIPEXSMODE, &mode) == -1)
> > > + syslog(LOG_WARNING, "Couldn't disable pipex: %m");
> > > +}
>
> Is pppd(8) still using K&R function declarations? Can we please add new
> functions with ANSI declarations instead and convert the rest as well.
> Also it looks like something strange is going on with indentation (just
> look at the last function above, it seems lines start with 4 spaces
> instead of 1 tab). Again this could be bad pppd code.
Yes, this stranger indentation style is common for pppd code. I just
mimic it in this diff to keep code style consistent. And I preffer to
keep code style as it to avoid too much difference with the upstream
version. Is this excuse sounds convincing? :)
--
Sergey
