Re: npppd(8): remove "pipex" option
Hi, On Wed, 1 Feb 2023 21:32:29 +0300 Vitaliy Makkoveev wrote: > On Wed, Feb 01, 2023 at 09:00:13PM +0900, YASUOKA Masahiko wrote: >> ... >> >> But I think we should keep the part since it is needed when adding a >> tunneling protocol which is not supported by pipex, or running npppd >> on another OS. >> >> >> If having "pipex yes/no" configuration is misleading, we can improve >> >> the man page or the configuration itself. >> > >> > pipex yes | no >> > Specify whether npppd(8) uses pipex(4). The default is >> > “yes”. The sysctl(8) variable net.pipex.enable should >> > also be enabled to use pipex(4). >> > >> > There is no misleading. But with "pipex no" npppd(8) is usable with >> > pppac(4), but with pppx(4) it is not. Also, I don't like that it >> > successfully creates connection. Guess, it better to deny "pipex no" >> > for pppx(4). >> >> I agree both. >> > > So, deny "pipex no" for pppx(4) interfaces. ok yasuoka Thanks, > Index: usr.sbin/npppd/npppd/npppd.conf.5 > === > RCS file: /cvs/src/usr.sbin/npppd/npppd/npppd.conf.5,v > retrieving revision 1.30 > diff -u -p -r1.30 npppd.conf.5 > --- usr.sbin/npppd/npppd/npppd.conf.5 31 Mar 2022 17:27:30 - 1.30 > +++ usr.sbin/npppd/npppd/npppd.conf.5 1 Feb 2023 18:28:29 - > @@ -362,6 +362,11 @@ variable > .Va net.pipex.enable > should also be enabled to use > .Xr pipex 4 . > +This value must be > +.Dq yes > +for > +.Xr pppx 4 > +interfaces. > .It Ic debug-dump-pktin Ar protocol ... > If this option is specified, > .Xr npppd 8 > Index: usr.sbin/npppd/npppd/parse.y > === > RCS file: /cvs/src/usr.sbin/npppd/npppd/parse.y,v > retrieving revision 1.25 > diff -u -p -r1.25 parse.y > --- usr.sbin/npppd/npppd/parse.y 15 Oct 2021 15:01:28 - 1.25 > +++ usr.sbin/npppd/npppd/parse.y 1 Feb 2023 18:28:29 - > @@ -924,6 +924,14 @@ bind : BIND TUNNEL FROM STRING AUTHENTI > free($9); > YYERROR; > } > + if (tunn->pipex == 0 && iface->is_pppx) { > + yyerror("pipex should be enabled for" > + " interface %s", $9); > + free($4); > + free($7); > + free($9); > + YYERROR; > + } > if ((n = malloc(sizeof(struct confbind))) == NULL) { > yyerror("out of memory"); > free($4); >
Re: npppd(8): remove "pipex" option
On Wed, Feb 01, 2023 at 09:00:13PM +0900, YASUOKA Masahiko wrote: > Hi > > ... > > But I think we should keep the part since it is needed when adding a > tunneling protocol which is not supported by pipex, or running npppd > on another OS. > > >> If having "pipex yes/no" configuration is misleading, we can improve > >> the man page or the configuration itself. > > > > pipex yes | no > > Specify whether npppd(8) uses pipex(4). The default is > > “yes”. The sysctl(8) variable net.pipex.enable should > > also be enabled to use pipex(4). > > > > There is no misleading. But with "pipex no" npppd(8) is usable with > > pppac(4), but with pppx(4) it is not. Also, I don't like that it > > successfully creates connection. Guess, it better to deny "pipex no" > > for pppx(4). > > I agree both. > So, deny "pipex no" for pppx(4) interfaces. Index: usr.sbin/npppd/npppd/npppd.conf.5 === RCS file: /cvs/src/usr.sbin/npppd/npppd/npppd.conf.5,v retrieving revision 1.30 diff -u -p -r1.30 npppd.conf.5 --- usr.sbin/npppd/npppd/npppd.conf.5 31 Mar 2022 17:27:30 - 1.30 +++ usr.sbin/npppd/npppd/npppd.conf.5 1 Feb 2023 18:28:29 - @@ -362,6 +362,11 @@ variable .Va net.pipex.enable should also be enabled to use .Xr pipex 4 . +This value must be +.Dq yes +for +.Xr pppx 4 +interfaces. .It Ic debug-dump-pktin Ar protocol ... If this option is specified, .Xr npppd 8 Index: usr.sbin/npppd/npppd/parse.y === RCS file: /cvs/src/usr.sbin/npppd/npppd/parse.y,v retrieving revision 1.25 diff -u -p -r1.25 parse.y --- usr.sbin/npppd/npppd/parse.y15 Oct 2021 15:01:28 - 1.25 +++ usr.sbin/npppd/npppd/parse.y1 Feb 2023 18:28:29 - @@ -924,6 +924,14 @@ bind : BIND TUNNEL FROM STRING AUTHENTI free($9); YYERROR; } + if (tunn->pipex == 0 && iface->is_pppx) { + yyerror("pipex should be enabled for" + " interface %s", $9); + free($4); + free($7); + free($9); + YYERROR; + } if ((n = malloc(sizeof(struct confbind))) == NULL) { yyerror("out of memory"); free($4);
Re: npppd(8): remove "pipex" option
I'm sorry for sending 2 mails. First one was a draft. Please read second one.
Re: npppd(8): remove "pipex" option
Hi On Tue, 31 Jan 2023 11:30:43 +0300 Vitaliy Makkoveev wrote: > On Tue, Jan 31, 2023 at 01:40:19PM +0900, YASUOKA Masahiko wrote: >> Hi, >> >> On Sun, 29 Jan 2023 14:35:05 +0300 >> Vitaliy Makkoveev wrote: >> > While switchind pppx(4) and pppac(4) from selwakeup() to KNOTE(9), I >> > found npppd(8) doesn't create pppx interface with "pipex no" in >> > npppd.conf, but successfully connects the client. So packets don't flow. >> > However, the pppac(4) has no this problem, because corresponding pppac >> > interface always created when npppd(8) opened device node. >> > >> > In fact, npppd(8) will not work with pppx(4) interfaces without pipex(4) >> > support. Otherwise npppd(8) should create pppx(4) sessions with not >> > pipex(4) specific PIPEXASESSION ioctl(2) command. >> > >> > I propose to remove "pipex" option from npppd(8). We already have >> > "net.pipex.enable" sysctl MIB to control pipex behaviour. In the case >> > then "net.pipex.enable" is set to 0, pipex(4) sessions will be always >> > created, but the traffic will go outside pipex(4) layer. >> > >> > The "ifdef USE_NPPPD_PIPEX" left as is. If we decide to remove them, I >> > will do this with the next diffs. >> >> Will the next diff remove the networking part (MPPE, IP) as well? >> >> > Please note, we never have complains about the problem described above, >> > so I doubt someone uses npppd(8) with "pipex no" in the npppd.conf(5). >> >> I don't know why you configured "pipex no", I suppose it was for >> debug. I also actually use "pipex no" when debug or development. > > I used this option to test my/visa@ diff which replaced selwakeup() by > KNOTE(9) and found that pppx(4) case is broken if this option is set to > "no". So you used "pipex no" for test. That is the purpose of "pipex yes/no". > Since we have the ability of enable/disable pipex(4) globally, I > propose to remove this option. No, they have different purposes. Without "pipex yes/no" option, we can't test the networking part (IP, MPPE) of npppd without pipex. > in fact, for the pppx(4) case npppd(8) is absolutely useless without > pipex(4) support, so I don't see any reasons to build it without > pipex(4). I don't propose to remove the whole "ifdef USE_NPPPD_PIPEX" > blocks, only preprocessor directives. Sorry, if my suggestion was not > clear. Note that the networking part (IP, MPPE) of npppd works with/without pipex. So just removing #ifdef lines, npppd still has "the networking part without pipex". As the result of removing "pipex yes/no" option, we can't test that part. If you are to remove the networking part (IP, MPPE) of npppd completely, removing "pipex yes/no" option makes sense since we don't need to test it anymore. But I think we should keep the part since it is needed when adding a tunneling protocol which is not supported by pipex, or running npppd on another OS. >> If having "pipex yes/no" configuration is misleading, we can improve >> the man page or the configuration itself. > > pipex yes | no > Specify whether npppd(8) uses pipex(4). The default is > “yes”. The sysctl(8) variable net.pipex.enable should > also be enabled to use pipex(4). > > There is no misleading. But with "pipex no" npppd(8) is usable with > pppac(4), but with pppx(4) it is not. Also, I don't like that it > successfully creates connection. Guess, it better to deny "pipex no" > for pppx(4). I agree both.
Re: npppd(8): remove "pipex" option
On Tue, 31 Jan 2023 11:30:43 +0300 Vitaliy Makkoveev wrote: > On Tue, Jan 31, 2023 at 01:40:19PM +0900, YASUOKA Masahiko wrote: >> Hi, >> >> On Sun, 29 Jan 2023 14:35:05 +0300 >> Vitaliy Makkoveev wrote: >> > While switchind pppx(4) and pppac(4) from selwakeup() to KNOTE(9), I >> > found npppd(8) doesn't create pppx interface with "pipex no" in >> > npppd.conf, but successfully connects the client. So packets don't flow. >> > However, the pppac(4) has no this problem, because corresponding pppac >> > interface always created when npppd(8) opened device node. >> > >> > In fact, npppd(8) will not work with pppx(4) interfaces without pipex(4) >> > support. Otherwise npppd(8) should create pppx(4) sessions with not >> > pipex(4) specific PIPEXASESSION ioctl(2) command. >> > >> > I propose to remove "pipex" option from npppd(8). We already have >> > "net.pipex.enable" sysctl MIB to control pipex behaviour. In the case >> > then "net.pipex.enable" is set to 0, pipex(4) sessions will be always >> > created, but the traffic will go outside pipex(4) layer. >> > >> > The "ifdef USE_NPPPD_PIPEX" left as is. If we decide to remove them, I >> > will do this with the next diffs. >> >> Will the next diff remove the networking part (MPPE, IP) as well? >> >> > Please note, we never have complains about the problem described above, >> > so I doubt someone uses npppd(8) with "pipex no" in the npppd.conf(5). >> >> I don't know why you configured "pipex no", I suppose it was for >> debug. I also actually use "pipex no" when debug or development. > > I used this option to test my/visa@ diff which replaced selwakeup() by > KNOTE(9) and found that pppx(4) case is broken if this option is set to > "no". So you used "pipex no" for test. That exactly is the purpose of "pipex yes/no". > Since we have the ability of enable/disable pipex(4) globally, I > propose to remove this option. No, they have different purposes. Without "pipex yes/no" option, we can't test the networking part (IP, MPPE) of npppd for without pipex. > in fact, for the pppx(4) case npppd(8) is absolutely useless without > pipex(4) support, so I don't see any reasons to build it without > pipex(4). I don't propose to remove the whole "ifdef USE_NPPPD_PIPEX" > blocks, only preprocessor directives. Sorry, if my suggestion was not > clear. Note that the networking part (IP, MPPE) of npppd works with/without pipex. So just removing #ifdef lines, npppd still has "the networking part without pipex". As the result of removing "pipex yes/no" option, we can't test that part. If you are to remove the networking part (IP, MPPE) of npppd completely, removing "pipex yes/no" option makes sense since we don't need to test it anymore. But I'd like to keep the networking part since it is needed when adding a tunneling protocol which is not supported by pipex, or running npppd on another OS. >> If having "pipex yes/no" configuration is misleading, we can improve >> the man page or the configuration itself. > > pipex yes | no > Specify whether npppd(8) uses pipex(4). The default is > “yes”. The sysctl(8) variable net.pipex.enable should > also be enabled to use pipex(4). > > There is no misleading. But with "pipex no" npppd(8) is usable with > pppac(4), but with pppx(4) it is not. Also, I don't like that it > successfully creates connection. Guess, it better to deny "pipex no" > for pppx(4). >
Re: npppd(8): remove "pipex" option
On Tue, Jan 31, 2023 at 01:40:19PM +0900, YASUOKA Masahiko wrote: > Hi, > > On Sun, 29 Jan 2023 14:35:05 +0300 > Vitaliy Makkoveev wrote: > > While switchind pppx(4) and pppac(4) from selwakeup() to KNOTE(9), I > > found npppd(8) doesn't create pppx interface with "pipex no" in > > npppd.conf, but successfully connects the client. So packets don't flow. > > However, the pppac(4) has no this problem, because corresponding pppac > > interface always created when npppd(8) opened device node. > > > > In fact, npppd(8) will not work with pppx(4) interfaces without pipex(4) > > support. Otherwise npppd(8) should create pppx(4) sessions with not > > pipex(4) specific PIPEXASESSION ioctl(2) command. > > > > I propose to remove "pipex" option from npppd(8). We already have > > "net.pipex.enable" sysctl MIB to control pipex behaviour. In the case > > then "net.pipex.enable" is set to 0, pipex(4) sessions will be always > > created, but the traffic will go outside pipex(4) layer. > > > > The "ifdef USE_NPPPD_PIPEX" left as is. If we decide to remove them, I > > will do this with the next diffs. > > Will the next diff remove the networking part (MPPE, IP) as well? > > > Please note, we never have complains about the problem described above, > > so I doubt someone uses npppd(8) with "pipex no" in the npppd.conf(5). > > I don't know why you configured "pipex no", I suppose it was for > debug. I also actually use "pipex no" when debug or development. I used this option to test my/visa@ diff which replaced selwakeup() by KNOTE(9) and found that pppx(4) case is broken if this option is set to "no". Since we have the ability of enable/disable pipex(4) globally, I propose to remove this option. in fact, for the pppx(4) case npppd(8) is absolutely useless without pipex(4) support, so I don't see any reasons to build it without pipex(4). I don't propose to remove the whole "ifdef USE_NPPPD_PIPEX" blocks, only preprocessor directives. Sorry, if my suggestion was not clear. > > If having "pipex yes/no" configuration is misleading, we can improve > the man page or the configuration itself. pipex yes | no Specify whether npppd(8) uses pipex(4). The default is “yes”. The sysctl(8) variable net.pipex.enable should also be enabled to use pipex(4). There is no misleading. But with "pipex no" npppd(8) is usable with pppac(4), but with pppx(4) it is not. Also, I don't like that it successfully creates connection. Guess, it better to deny "pipex no" for pppx(4).
Re: npppd(8): remove "pipex" option
Hi, On Sun, 29 Jan 2023 14:35:05 +0300 Vitaliy Makkoveev wrote: > While switchind pppx(4) and pppac(4) from selwakeup() to KNOTE(9), I > found npppd(8) doesn't create pppx interface with "pipex no" in > npppd.conf, but successfully connects the client. So packets don't flow. > However, the pppac(4) has no this problem, because corresponding pppac > interface always created when npppd(8) opened device node. > > In fact, npppd(8) will not work with pppx(4) interfaces without pipex(4) > support. Otherwise npppd(8) should create pppx(4) sessions with not > pipex(4) specific PIPEXASESSION ioctl(2) command. > > I propose to remove "pipex" option from npppd(8). We already have > "net.pipex.enable" sysctl MIB to control pipex behaviour. In the case > then "net.pipex.enable" is set to 0, pipex(4) sessions will be always > created, but the traffic will go outside pipex(4) layer. > > The "ifdef USE_NPPPD_PIPEX" left as is. If we decide to remove them, I > will do this with the next diffs. Will the next diff remove the networking part (MPPE, IP) as well? > Please note, we never have complains about the problem described above, > so I doubt someone uses npppd(8) with "pipex no" in the npppd.conf(5). I don't know why you configured "pipex no", I suppose it was for debug. I also actually use "pipex no" when debug or development. If having "pipex yes/no" configuration is misleading, we can improve the man page or the configuration itself. > I tested both pppac(4) and pppx(4) cases with both "net.pipex.enable=1" > and "net.pipex.enable=0". > > Index: usr.sbin/npppd/npppd/npppd.c > === > RCS file: /cvs/src/usr.sbin/npppd/npppd/npppd.c,v > retrieving revision 1.53 > diff -u -p -r1.53 npppd.c > --- usr.sbin/npppd/npppd/npppd.c 1 Jul 2022 09:57:24 - 1.53 > +++ usr.sbin/npppd/npppd/npppd.c 29 Jan 2023 11:04:30 - > @@ -235,14 +235,12 @@ npppd_get_npppd() > int > npppd_init(npppd *_this, const char *config_file) > { > - int i, status = -1, value; > + int i, status = -1; > const char *pidpath0; > FILE*pidfp = NULL; > struct tunnconf *tunn; > struct ipcpconf *ipcpconf; > struct ipcpstat *ipcpstat; > - int mib[] = { CTL_NET, PF_PIPEX, PIPEXCTL_ENABLE }; > - size_t size; > > memset(_this, 0, sizeof(npppd)); > #ifndef NO_ROUTE_FOR_POOLED_ADDRESS > @@ -294,17 +292,6 @@ npppd_init(npppd *_this, const char *con > if ((status = npppd_reload_config(_this)) != 0) > return status; > > - TAILQ_FOREACH(tunn, &_this->conf.tunnconfs, entry) { > - if (tunn->pipex) { > - size = sizeof(value); > - if (!sysctl(mib, nitems(mib), , , NULL, 0) > - && value == 0) > - log_printf(LOG_WARNING, > - "pipex(4) is disabled by sysctl"); > - break; > - } > - } > - > if ((_this->map_user_ppp = hash_create( > (int (*) (const void *, const void *))strcmp, str_hash, > NPPPD_USER_HASH_SIZ)) == NULL) { > @@ -1052,7 +1039,6 @@ npppd_ppp_pipex_enable(npppd *_this, npp > > NPPPD_ASSERT(ppp != NULL); > NPPPD_ASSERT(ppp->phy_context != NULL); > - NPPPD_ASSERT(ppp->use_pipex != 0); > > pipex_setup_common(ppp, ); > > Index: usr.sbin/npppd/npppd/npppd.conf.5 > === > RCS file: /cvs/src/usr.sbin/npppd/npppd/npppd.conf.5,v > retrieving revision 1.30 > diff -u -p -r1.30 npppd.conf.5 > --- usr.sbin/npppd/npppd/npppd.conf.5 31 Mar 2022 17:27:30 - 1.30 > +++ usr.sbin/npppd/npppd/npppd.conf.5 29 Jan 2023 11:04:30 - > @@ -349,19 +349,6 @@ the address assigned by > for the link. > The default value is > .Dq no . > -.It Ic pipex Ar yes | no > -Specify whether > -.Xr npppd 8 > -uses > -.Xr pipex 4 . > -The default is > -.Dq yes . > -The > -.Xr sysctl 8 > -variable > -.Va net.pipex.enable > -should also be enabled to use > -.Xr pipex 4 . > .It Ic debug-dump-pktin Ar protocol ... > If this option is specified, > .Xr npppd 8 > Index: usr.sbin/npppd/npppd/npppd.h > === > RCS file: /cvs/src/usr.sbin/npppd/npppd/npppd.h,v > retrieving revision 1.19 > diff -u -p -r1.19 npppd.h > --- usr.sbin/npppd/npppd/npppd.h 12 Aug 2017 11:20:34 - 1.19 > +++ usr.sbin/npppd/npppd/npppd.h 29 Jan 2023 11:04:30 - > @@ -133,8 +133,6 @@ struct tunnconf { > bool ingress_filter; > intcallnum_check; > > - bool pipex; > - > u_int debug_dump_pktin; > u_int debug_dump_pktout; > }; > Index:
npppd(8): remove "pipex" option
Hi, While switchind pppx(4) and pppac(4) from selwakeup() to KNOTE(9), I found npppd(8) doesn't create pppx interface with "pipex no" in npppd.conf, but successfully connects the client. So packets don't flow. However, the pppac(4) has no this problem, because corresponding pppac interface always created when npppd(8) opened device node. In fact, npppd(8) will not work with pppx(4) interfaces without pipex(4) support. Otherwise npppd(8) should create pppx(4) sessions with not pipex(4) specific PIPEXASESSION ioctl(2) command. I propose to remove "pipex" option from npppd(8). We already have "net.pipex.enable" sysctl MIB to control pipex behaviour. In the case then "net.pipex.enable" is set to 0, pipex(4) sessions will be always created, but the traffic will go outside pipex(4) layer. The "ifdef USE_NPPPD_PIPEX" left as is. If we decide to remove them, I will do this with the next diffs. Please note, we never have complains about the problem described above, so I doubt someone uses npppd(8) with "pipex no" in the npppd.conf(5). I tested both pppac(4) and pppx(4) cases with both "net.pipex.enable=1" and "net.pipex.enable=0". Index: usr.sbin/npppd/npppd/npppd.c === RCS file: /cvs/src/usr.sbin/npppd/npppd/npppd.c,v retrieving revision 1.53 diff -u -p -r1.53 npppd.c --- usr.sbin/npppd/npppd/npppd.c1 Jul 2022 09:57:24 - 1.53 +++ usr.sbin/npppd/npppd/npppd.c29 Jan 2023 11:04:30 - @@ -235,14 +235,12 @@ npppd_get_npppd() int npppd_init(npppd *_this, const char *config_file) { - int i, status = -1, value; + int i, status = -1; const char *pidpath0; FILE*pidfp = NULL; struct tunnconf *tunn; struct ipcpconf *ipcpconf; struct ipcpstat *ipcpstat; - int mib[] = { CTL_NET, PF_PIPEX, PIPEXCTL_ENABLE }; - size_t size; memset(_this, 0, sizeof(npppd)); #ifndefNO_ROUTE_FOR_POOLED_ADDRESS @@ -294,17 +292,6 @@ npppd_init(npppd *_this, const char *con if ((status = npppd_reload_config(_this)) != 0) return status; - TAILQ_FOREACH(tunn, &_this->conf.tunnconfs, entry) { - if (tunn->pipex) { - size = sizeof(value); - if (!sysctl(mib, nitems(mib), , , NULL, 0) - && value == 0) - log_printf(LOG_WARNING, - "pipex(4) is disabled by sysctl"); - break; - } - } - if ((_this->map_user_ppp = hash_create( (int (*) (const void *, const void *))strcmp, str_hash, NPPPD_USER_HASH_SIZ)) == NULL) { @@ -1052,7 +1039,6 @@ npppd_ppp_pipex_enable(npppd *_this, npp NPPPD_ASSERT(ppp != NULL); NPPPD_ASSERT(ppp->phy_context != NULL); - NPPPD_ASSERT(ppp->use_pipex != 0); pipex_setup_common(ppp, ); Index: usr.sbin/npppd/npppd/npppd.conf.5 === RCS file: /cvs/src/usr.sbin/npppd/npppd/npppd.conf.5,v retrieving revision 1.30 diff -u -p -r1.30 npppd.conf.5 --- usr.sbin/npppd/npppd/npppd.conf.5 31 Mar 2022 17:27:30 - 1.30 +++ usr.sbin/npppd/npppd/npppd.conf.5 29 Jan 2023 11:04:30 - @@ -349,19 +349,6 @@ the address assigned by for the link. The default value is .Dq no . -.It Ic pipex Ar yes | no -Specify whether -.Xr npppd 8 -uses -.Xr pipex 4 . -The default is -.Dq yes . -The -.Xr sysctl 8 -variable -.Va net.pipex.enable -should also be enabled to use -.Xr pipex 4 . .It Ic debug-dump-pktin Ar protocol ... If this option is specified, .Xr npppd 8 Index: usr.sbin/npppd/npppd/npppd.h === RCS file: /cvs/src/usr.sbin/npppd/npppd/npppd.h,v retrieving revision 1.19 diff -u -p -r1.19 npppd.h --- usr.sbin/npppd/npppd/npppd.h12 Aug 2017 11:20:34 - 1.19 +++ usr.sbin/npppd/npppd/npppd.h29 Jan 2023 11:04:30 - @@ -133,8 +133,6 @@ struct tunnconf { bool ingress_filter; intcallnum_check; - bool pipex; - u_int debug_dump_pktin; u_int debug_dump_pktout; }; Index: usr.sbin/npppd/npppd/parse.y === RCS file: /cvs/src/usr.sbin/npppd/npppd/parse.y,v retrieving revision 1.25 diff -u -p -r1.25 parse.y --- usr.sbin/npppd/npppd/parse.y15 Oct 2021 15:01:28 - 1.25 +++ usr.sbin/npppd/npppd/parse.y29 Jan 2023 11:04:30 - @@ -125,7 +125,7 @@ typedef struct { %token L2TP_HELLO_INTERVAL L2TP_HELLO_TIMEOUT L2TP_ACCEPT_DIALIN %token MPPE MPPE_KEY_LENGTH MPPE_KEY_STATE %token IDLE_TIMEOUT TCP_MSS_ADJUST