search

ppp-project / ppp

Paul's PPP Package: PPP daemon and associated utilities | Official GitHub repo: https://github.com/ppp-project/ppp
https://github.com/ppp-project/ppp
Other
384 stars 228 forks source link

We should avoid code duplication between rp-pppoe and this project #383

Open dfskoll opened 1 year ago

dfskoll commented 1 year ago

The pppd/plugins/pppoe directory contains a substantial amount of code from the rp-pppoe project, and unfortunately that code has diverged from upstream.

I think we should synchronize the projects so the code isn't duplicated, and possibly remove the pppoe plugin from the ppp project once rp-pppoe builds and installs properly alongside ppp.

Created this issue in response to https://github.com/ppp-project/ppp/pull/379#issuecomment-1372472906

paulusmack commented 1 year ago

Sounds like a good idea; how do you suggest we go about it?

jkroonza commented 1 year ago

Just my thoughts:

rp-pppoe I believe was the original implementation.

MOST users will never need the servers, the bridge mechanisms and most of the other tools provided by the rp-pppoe package, only the pppoe module as per current available in ppp.

From a perspective of "if rp-pppoe requires additional options in pppoe.so module" it makes sense for me to package the ppp module with rp-pppoe.

I do, however, think that unlikely. The way I would go personally if this was up to me and I controlled both packages (I'm not responsible for either, but believe I've got insight into both) is to put the pppoe module in ppp package as it's more involved with the ppp internals than pppoe-server (for example). Further to this, rp-pppoe (and specifically pppoe-server) cannot stand without ppp (even if the module is

I don't know if there is much sense to keep the pppoe userspace implementation around any more.

One other option which may be considered but which has a bit higher administrative overhead is a git submodule for the pppoe.so file ppp module specifically which can be imported into both projects (even though rp-pppoe cannot stand without ppp), or just an outright separate repository for that.

Neustradamus commented 1 year ago

I think that rp-pppoe can be in the "ppp-project" organization near ppp.

dfskoll commented 1 year ago

I think the first step should be to examine how the C source files have diverged between the ppp project and upstream rp-pppoe and reconcile those divergences. Once that's done, we can look at what functions in the common C files are needed by both the PPPoE client and server software and try to minimize those. One example is parsePacket, which is used by both the client and server.

I'm not sure if it makes sense to have a library in the ppp project against which the pppoe-server links, or just duplicate the common code if there's only a small amount of it.

My personal preference would be to remove all the PPPoE stuff from the ppp project and just keep it in rp-pppoe, but I don't think that's going to fly :) I also don't think moving all of rp-pppoe under the ppp project makes sense; the server code is really different from everything else.

But anyway... resolving the divergence of the source should be the first step.

dfskoll commented 1 year ago

Oh, and I'm OK with getting rid of the userspace PPPoE code, but that means it's irrevocably tied to Linux. I know at some point, user-mode rp-pppoe used to work on BSD and even Solaris, but I suspect that has long bit-rotted.

jkroonza commented 1 year ago

Oh, and I'm OK with getting rid of the userspace PPPoE code, but that means it's irrevocably tied to Linux. I know at some point, user-mode rp-pppoe used to work on BSD and even Solaris, but I suspect that has long bit-rotted.

ppp itself doesn't work on BSD any more, but it does on Solaris. I accidentally tested user-mode and it works, but most likely won't perform as well, so until it breaks proper I suggest keeping it is probably the best option.

Could you also please clarify what you mean with "I'm not sure if it makes sense to have a library in the ppp project against which the pppoe-server links" - do you mean to say there is code that goes into both the pppoe module (ie, the ppp plugin) as well as the other pppoe- binaries? This seems to be accurate upon actually looking into more detail, specifically the discovery portion of the pppoe plugin, and specifically parsePacket() seems to relate. This is a reasonably simple function from the looks of it, the rest of the common.c file looks* mostly distinct functions.

discovery.c at least has diverged quite far. And here are the interesting commits:

commit dbfeebc9adcf76a50c1d4e9035d5d481914edb43
Author: Michal Ostrowski <mostrows@speakeasy.net>
Date:   Fri Dec 14 02:55:20 2001 +0000

    Switch to using RoaringPenguin-based PPPOE plugin

commit cdbb124cea5feeb4468ad3f2e5b5d17de583da6d
Author: Paul Mackerras <paulus@samba.org>
Date:   Thu Jul 26 20:03:32 2001 +0000

    pppoe plugin stuff - Michal Ostrowski's version, lightly hacked

The first change after these were then on the last day of 2020 in order to rename from rp-pppoe to pppoe, and then there were a bunch of changes.

Unfortunately I don't have full VCS history for the current rp-pppoe version, so can't really comment if much has happened on that front?

dfskoll commented 1 year ago

Yes; I meant having a library in the ppp project that contains the functions common to pppoe-server and the pppoe plugin. As you say, there are not many of them and they are pretty simple, so maybe duplicating those is not a big deal. Probably would be a good idea to split common.c into two files... one that contains functions only used by the client and one that contains functions also used by the server.

As I may have mentioned, I can't make the full PPPoE git history available because in the past I worked on some proprietary code for a customer, and that's in the git history. :(

Neustradamus commented 1 year ago

When I speak about "ppp-project" is to have the code here: https://github.com/ppp-project/rp-pppoe.

https://github.com/ppp-project/ppp and https://github.com/ppp-project/rp-pppoe are two repositories at a same place to improve the code...

@dfskoll: Since I have proposed you to put the code on GitHub, new contributions have been done...

dfskoll commented 1 year ago

What is the advantage of having under ppp-project rather than under dfskoll? It seems like the only difference is who controls the project, and at this point, I prefer to keep control.

enaess commented 1 year ago

I think @dfskoll is right in that the first step is to review the code differences between the two. And then perhaps if possible consolidation of said code. I don't think there should be any difference between these two, and that kernel mode vs user-mode is really just a feature of said plugin (dependent on how you chose to configure and compile it).

Second to that, I am not a big fan of putting the code of (rp-)pppoe directly in the code tree of pppd. It's a bit of a cluster ** with automake/autoconf and having to generate multiple config.h files. It feels like the pppoe plugin (as well as several other plugins) should be standalone projects. Perhaps under the ppp-project umbrella, or some other entity.

Typically, any project can create and distribute a plugin for pppd, but I think the (rp-)pppoe module is different in that it can be used for both client and server. And people who would like to use this as a client, is not interested in the server piece of it. Establishing a repository in ppp-project/rp-ppope-plugin or something similar would require

Also, it should be possible with packaging of rp-pppoe project to configure a dependency on the rp-pppoe-plugin being installed.

Ultimately, I don't think maintaining two repositories for rp-pppoe-plugin (or forking @dfskoll is a nice thing) is inherently difficult. It also confuses users, issue tracking, maintenance, etc.

enaess commented 1 year ago

@Neustradamus if you / Paul were to make a https://github.com/ppp-project/rp-pppoe-plugin (doesn't exist yet), would it be possible to assign "ownership" of the project (or joint ownership) to @dfskoll

Note the name difference here, just saying rp-pppoe implies the entire project from @dfskoll. I think it somehow should be named with "plugin" somehow to make distinction that it only provides the plugin.

enaess commented 1 year ago

@dfskoll is rp-pppoe (server) and utilities distributed as a package per Debian or Ubuntu? If not, you should probably file a intent to package or request to package for this software with Debian and cook up a package recipe for this.

dfskoll commented 1 year ago

The Debian package is called pppoe and includes the server, relay, etc.

See https://packages.debian.org/bullseye/amd64/pppoe/filelist

It does not include the pppoe.so plugin, which is part of the ppp package:

https://packages.debian.org/bullseye/amd64/ppp/filelist

enaess commented 1 year ago

Yeah, I just found it by apt-get source pppoe, gets me rp-pppoe-3.12.

So rp-pppoe.so file (which supports kernel mode?) isn't distributed on Debian?

dfskoll commented 1 year ago

So rp-pppoe.so file (which supports kernel mode) isn't distributed on Debian?

It is, but in the ppp package, not the pppoe package.

enaess commented 1 year ago

So users like @jkroonza needs to go out of their way to get rp-pppoe sources and compile the plugin? Yeah, I see that by the control file require ppp >= 2.3.10-1.

dfskoll commented 1 year ago

I think they can use the plugin that's included with ppp and ignore the one included with (rp-)pppoe. But @jkroonza can confirm.

enaess commented 1 year ago

Yeah, I am curious now. I was under the impression it had to do with performance...

dfskoll commented 1 year ago

The plugin included with ppp and the one in the rp-pppoe sources are very similar; they should have identical performance since their code only comes into play for PPPoE discovery packets. The kernel completely handles PPPoE session packets.

enaess commented 1 year ago

@dfskoll that is good to know. I'd like to hear it from @jkroonza to why he needed the one from rp-pppoe package (or better yet trying to understand why anyone would want the one from rp-pppoe package). Again, this is some of the pain / confusion of forking a project ...

pali commented 1 year ago

I did cleanup of pppoe.so plugin in ppp repository, removed and changed code better fit in ppp repository. I'm not sure if such changes are useful / suitable for rp-pppoe server project. Other changes are mostly fixups for the client code.

As jkroonza said, most users do not need server part of pppoe. I think that rp-pppoe server code should work with pppoe.so plugin from ppp repository.

So what about putting all client code with pppoe.so plugin into ppp project and all server code (without pppoe.so plugin) into rp-pppoe? Or is there is too many common code which makes sense to somehow share between ppp and rp-pppoe projects?

dfskoll commented 1 year ago

That's what I'm getting at. You changed the pppoe.so code. But so did I, upstream, independently. We should try to merge the code together because some of the upstream changes might be good to have in ppp/plugins/pppoe.

Eventually, when the changes are merged, I guess it makes sense to have all of the plugin code live only in the ppp project, and have rp-pppoe concentrate on supplying the server and relay implementations. To the extent that there's common code between the rp-pppoe server and the pppoe plugin, there will be code duplication. I have to look and see how much duplicated code there really is.

enaess commented 1 year ago

One of the things I remember being fixed in the ppp/pppoe version is compiler warnings (that still exist in the rp-pppoe project when compiling with the plugin). And yes, I do recall @pali fixing a bunch of things there as well. My two cents is that there should be only one single source of the truth, and it should be able to handle server and client mode, user and kernel mode version in the same plugin. Also, I would love to see a wiki enabled on the GitHub project with some documentation to how to configure either of these.

enaess commented 1 year ago

And yes, there will be some duplication of code. That's just a fact when you split a project in two.

dfskoll commented 1 year ago

I don't see any compiler warnings with the latest rp-pppoe git HEAD. This is on Debian 11.

$ make rp-pppoe.so
gcc -DPLUGIN=1 '-DRP_VERSION="3.15"' -g -O2 -fno-strict-aliasing -Wall -Wstrict-prototypes    '-DPPPOE_PATH="/usr/sbin/pppoe"' '-DPPPD_PATH="/sbin/pppd"' '-DPLUGIN_PATH="/etc/ppp/plugins/rp-pppoe.so"' '-DPPPOE_SERVER_OPTIONS="/etc/ppp/pppoe-server-options"' -Ilibevent -I/usr/include -c -o plugin/plugin.o -fPIC plugin.c
gcc -DPLUGIN=1 -g -O2 -fno-strict-aliasing -Wall -Wstrict-prototypes    '-DPPPOE_PATH="/usr/sbin/pppoe"' '-DPPPD_PATH="/sbin/pppd"' '-DPLUGIN_PATH="/etc/ppp/plugins/rp-pppoe.so"' '-DPPPOE_SERVER_OPTIONS="/etc/ppp/pppoe-server-options"' -Ilibevent '-DRP_VERSION="3.15"' -I/usr/include -c -o plugin/discovery.o -fPIC discovery.c
gcc -DPLUGIN=1 -g -O2 -fno-strict-aliasing -Wall -Wstrict-prototypes    '-DPPPOE_PATH="/usr/sbin/pppoe"' '-DPPPD_PATH="/sbin/pppd"' '-DPLUGIN_PATH="/etc/ppp/plugins/rp-pppoe.so"' '-DPPPOE_SERVER_OPTIONS="/etc/ppp/pppoe-server-options"' -Ilibevent '-DRP_VERSION="3.15"' -I/usr/include -c -o plugin/if.o -fPIC if.c
gcc -DPLUGIN=1 -g -O2 -fno-strict-aliasing -Wall -Wstrict-prototypes    '-DPPPOE_PATH="/usr/sbin/pppoe"' '-DPPPD_PATH="/sbin/pppd"' '-DPLUGIN_PATH="/etc/ppp/plugins/rp-pppoe.so"' '-DPPPOE_SERVER_OPTIONS="/etc/ppp/pppoe-server-options"' -Ilibevent '-DRP_VERSION="3.15"' -I/usr/include -c -o plugin/common.o -fPIC common.c
gcc -DPLUGIN=1 -g -O2 -fno-strict-aliasing -Wall -Wstrict-prototypes    '-DPPPOE_PATH="/usr/sbin/pppoe"' '-DPPPD_PATH="/sbin/pppd"' '-DPLUGIN_PATH="/etc/ppp/plugins/rp-pppoe.so"' '-DPPPOE_SERVER_OPTIONS="/etc/ppp/pppoe-server-options"' -Ilibevent '-DRP_VERSION="3.15"' -I/usr/include -c -o plugin/debug.o -fPIC debug.c
ar -rc plugin/libplugin.a plugin/discovery.o plugin/if.o plugin/common.o plugin/debug.o
gcc -o rp-pppoe.so -shared plugin/plugin.o plugin/libplugin.a

No warnings with just a plain make that builds everything, either.

enaess commented 1 year ago

@dfskoll

This maybe because I am on a different distribution than what you are running. Currently, I am on a Ubuntu Cloud / Minimal (uvt-kvm create) using Ubuntu 22.04 Jammy Jellyfish, install gcc and build-essentials and I have the following (using git repository). I don't have such warnings in the logs when compiling pppd project.

gcc -g -O2 -fno-strict-aliasing -Wall -Wstrict-prototypes    '-DPPPOE_PATH="/usr/sbin/pppoe"' '-DPPPD_PATH="/usr/sbin/pppd"' '-DPLUGIN_PATH="/etc/ppp/plugins/rp-pppoe.so"' '-DPPPOE_SERVER_OPTIONS="/etc/ppp/pppoe-server-options"' -Ilibevent '-DRP_VERSION="3.15"' -c -o pppoe.o pppoe.c
gcc -g -O2 -fno-strict-aliasing -Wall -Wstrict-prototypes    '-DPPPOE_PATH="/usr/sbin/pppoe"' '-DPPPD_PATH="/usr/sbin/pppd"' '-DPLUGIN_PATH="/etc/ppp/plugins/rp-pppoe.so"' '-DPPPOE_SERVER_OPTIONS="/etc/ppp/pppoe-server-options"' -Ilibevent '-DRP_VERSION="3.15"' -c -o if.o if.c
gcc -g -O2 -fno-strict-aliasing -Wall -Wstrict-prototypes    '-DPPPOE_PATH="/usr/sbin/pppoe"' '-DPPPD_PATH="/usr/sbin/pppd"' '-DPLUGIN_PATH="/etc/ppp/plugins/rp-pppoe.so"' '-DPPPOE_SERVER_OPTIONS="/etc/ppp/pppoe-server-options"' -Ilibevent '-DRP_VERSION="3.15"' -c -o debug.o debug.c
gcc -g -O2 -fno-strict-aliasing -Wall -Wstrict-prototypes    '-DPPPOE_PATH="/usr/sbin/pppoe"' '-DPPPD_PATH="/usr/sbin/pppd"' '-DPLUGIN_PATH="/etc/ppp/plugins/rp-pppoe.so"' '-DPPPOE_SERVER_OPTIONS="/etc/ppp/pppoe-server-options"' -Ilibevent '-DRP_VERSION="3.15"' -c -o common.o common.c
common.c: In function ‘dropPrivs’:
common.c:230:9: warning: ignoring return value of ‘setegid’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
  230 |         setegid(getgid());
      |         ^~~~~~~~~~~~~~~~~
common.c:231:9: warning: ignoring return value of ‘seteuid’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
  231 |         seteuid(getuid());
      |         ^~~~~~~~~~~~~~~~~
gcc -g -O2 -fno-strict-aliasing -Wall -Wstrict-prototypes    '-DPPPOE_PATH="/usr/sbin/pppoe"' '-DPPPD_PATH="/usr/sbin/pppd"' '-DPLUGIN_PATH="/etc/ppp/plugins/rp-pppoe.so"' '-DPPPOE_SERVER_OPTIONS="/etc/ppp/pppoe-server-options"' -Ilibevent '-DRP_VERSION="3.15"' -c -o ppp.o ppp.c
gcc -g -O2 -fno-strict-aliasing -Wall -Wstrict-prototypes    '-DPPPOE_PATH="/usr/sbin/pppoe"' '-DPPPD_PATH="/usr/sbin/pppd"' '-DPLUGIN_PATH="/etc/ppp/plugins/rp-pppoe.so"' '-DPPPOE_SERVER_OPTIONS="/etc/ppp/pppoe-server-options"' -Ilibevent '-DRP_VERSION="3.15"' -c -o discovery.o discovery.c
discovery.c: In function ‘sendPADR’:
discovery.c:575:8: warning: array subscript ‘PPPoETag {aka struct PPPoETagStruct}[0]’ is partly outside array bounds of ‘PPPoEPacket[1]’ {aka ‘struct PPPoEPacketStruct[1]’} [-Warray-bounds]
  575 |     svc->type = TAG_SERVICE_NAME;
      |        ^~
discovery.c:554:17: note: while referencing ‘packet’
  554 |     PPPoEPacket packet;
      |                 ^~~~~~
discovery.c:576:8: warning: array subscript ‘PPPoETag {aka struct PPPoETagStruct}[0]’ is partly outside array bounds of ‘PPPoEPacket[1]’ {aka ‘struct PPPoEPacketStruct[1]’} [-Warray-bounds]
  576 |     svc->length = htons(namelen);
      |        ^~
discovery.c:554:17: note: while referencing ‘packet’
  554 |     PPPoEPacket packet;
      |                 ^~~~~~
discovery.c: In function ‘sendPADI’:
discovery.c:359:12: warning: array subscript ‘PPPoETag {aka struct PPPoETagStruct}[0]’ is partly outside array bounds of ‘PPPoEPacket[1]’ {aka ‘struct PPPoEPacketStruct[1]’} [-Warray-bounds]
  359 |         svc->type = TAG_SERVICE_NAME;
      |            ^~
discovery.c:332:17: note: while referencing ‘packet’
  332 |     PPPoEPacket packet;
      |                 ^~~~~~
discovery.c:360:12: warning: array subscript ‘PPPoETag {aka struct PPPoETagStruct}[0]’ is partly outside array bounds of ‘PPPoEPacket[1]’ {aka ‘struct PPPoEPacketStruct[1]’} [-Warray-bounds]
  360 |         svc->length = htons(namelen);
      |            ^~
discovery.c:332:17: note: while referencing ‘packet’
  332 |     PPPoEPacket packet;
      |                 ^~~~~~
gcc -o pppoe pppoe.o if.o debug.o common.o ppp.o discovery.o 
gcc -g -O2 -fno-strict-aliasing -Wall -Wstrict-prototypes    '-DPPPOE_PATH="/usr/sbin/pppoe"' '-DPPPD_PATH="/usr/sbin/pppd"' '-DPLUGIN_PATH="/etc/ppp/plugins/rp-pppoe.so"' '-DPPPOE_SERVER_OPTIONS="/etc/ppp/pppoe-server-options"' -Ilibevent '-DRP_VERSION="3.15"' -c -o pppoe-server.o pppoe-server.c
pppoe-server.c: In function ‘main’:
pppoe-server.c:1659:9: warning: ignoring return value of ‘fread’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
 1659 |         fread(&x, 1, sizeof(x), fp);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~
pppoe-server.c:1661:9: warning: ignoring return value of ‘fread’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
 1661 |         fread(&CookieSeed, 1, SEED_LEN, fp);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
pppoe-server.c:1838:9: warning: ignoring return value of ‘chdir’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
 1838 |         chdir("/");
      |         ^~~~~~~~~~
pppoe-server.c:1878:9: warning: ignoring return value of ‘ftruncate’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
 1878 |         ftruncate(LockFD, 0);
      |         ^~~~~~~~~~~~~~~~~~~~
pppoe-server.c:1880:9: warning: ignoring return value of ‘write’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
 1880 |         write(LockFD, buf, strlen(buf));
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
pppoe-server.c:1892:9: warning: ignoring return value of ‘write’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
 1892 |         write(KidPipe[1], "X", 1);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~
gcc -g -O2 -fno-strict-aliasing -Wall -Wstrict-prototypes    '-DPPPOE_PATH="/usr/sbin/pppoe"' '-DPPPD_PATH="/usr/sbin/pppd"' '-DPLUGIN_PATH="/etc/ppp/plugins/rp-pppoe.so"' '-DPPPOE_SERVER_OPTIONS="/etc/ppp/pppoe-server-options"' -Ilibevent '-DRP_VERSION="3.15"' -c -o md5.o md5.c
cc -g -O2 -fno-strict-aliasing -Wall -Wstrict-prototypes    '-DPPPOE_PATH="/usr/sbin/pppoe"' '-DPPPD_PATH="/usr/sbin/pppd"' '-DPLUGIN_PATH="/etc/ppp/plugins/rp-pppoe.so"' '-DPPPOE_SERVER_OPTIONS="/etc/ppp/pppoe-server-options"' -Ilibevent   -c -o control_socket.o control_socket.c
cd libevent && make DEFINES=""
make[1]: Entering directory '/home/ubuntu/projects/rp-pppoe/src/libevent'
gcc -g -O2 -fno-strict-aliasing -Wall -Wstrict-prototypes -I..  -c -o event.o event.c
gcc -g -O2 -fno-strict-aliasing -Wall -Wstrict-prototypes -I..  -c -o event_tcp.o event_tcp.c
gcc -g -O2 -fno-strict-aliasing -Wall -Wstrict-prototypes -I..  -c -o hash.o hash.c
gcc -g -O2 -fno-strict-aliasing -Wall -Wstrict-prototypes -I..  -c -o event_sig.o event_sig.c
event_sig.c: In function ‘sig_handler’:
event_sig.c:146:5: warning: ignoring return value of ‘write’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
  146 |     write(Pipe[1], &sig, 1);
      |     ^~~~~~~~~~~~~~~~~~~~~~~
rm -f libevent.a
ar -cq libevent.a event.o event_tcp.o hash.o event_sig.o
ranlib libevent.a
make[1]: Leaving directory '/home/ubuntu/projects/rp-pppoe/src/libevent'
gcc -o pppoe-server  pppoe-server.o if.o debug.o common.o md5.o control_socket.o libevent/libevent.a    -Llibevent -levent
gcc -g -O2 -fno-strict-aliasing -Wall -Wstrict-prototypes    '-DPPPOE_PATH="/usr/sbin/pppoe"' '-DPPPD_PATH="/usr/sbin/pppd"' '-DPLUGIN_PATH="/etc/ppp/plugins/rp-pppoe.so"' '-DPPPOE_SERVER_OPTIONS="/etc/ppp/pppoe-server-options"' -Ilibevent '-DRP_VERSION="3.15"' -c -o pppoe-sniff.o pppoe-sniff.c
gcc -o pppoe-sniff pppoe-sniff.o if.o common.o debug.o 
gcc -g -O2 -fno-strict-aliasing -Wall -Wstrict-prototypes    '-DPPPOE_PATH="/usr/sbin/pppoe"' '-DPPPD_PATH="/usr/sbin/pppd"' '-DPLUGIN_PATH="/etc/ppp/plugins/rp-pppoe.so"' '-DPPPOE_SERVER_OPTIONS="/etc/ppp/pppoe-server-options"' -Ilibevent '-DRP_VERSION="3.15"' -c -o relay.o relay.c
relay.c: In function ‘main’:
relay.c:343:9: warning: ignoring return value of ‘chdir’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
  343 |         chdir("/");
      |         ^~~~~~~~~~
relay.c: In function ‘relayLoop’:
relay.c:802:13: warning: ignoring return value of ‘read’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
  802 |             read(CleanPipe[0], &dummy, 1);
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
relay.c: In function ‘alarmHandler’:
relay.c:1528:9: warning: ignoring return value of ‘write’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
 1528 |         write(CleanPipe[1], "", 1);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~
gcc -o pppoe-relay relay.o if.o debug.o common.o 
gcc -DPLUGIN=1 '-DRP_VERSION="3.15"' -g -O2 -fno-strict-aliasing -Wall -Wstrict-prototypes    '-DPPPOE_PATH="/usr/sbin/pppoe"' '-DPPPD_PATH="/usr/sbin/pppd"' '-DPLUGIN_PATH="/etc/ppp/plugins/rp-pppoe.so"' '-DPPPOE_SERVER_OPTIONS="/etc/ppp/pppoe-server-options"' -Ilibevent -I/usr/include -c -o plugin/plugin.o -fPIC plugin.c
plugin.c:67:12: warning: ‘seen_devnam’ defined but not used [-Wunused-variable]
   67 | static int seen_devnam[2] = {0, 0};
      |            ^~~~~~~~~~~
gcc -DPLUGIN=1 -g -O2 -fno-strict-aliasing -Wall -Wstrict-prototypes    '-DPPPOE_PATH="/usr/sbin/pppoe"' '-DPPPD_PATH="/usr/sbin/pppd"' '-DPLUGIN_PATH="/etc/ppp/plugins/rp-pppoe.so"' '-DPPPOE_SERVER_OPTIONS="/etc/ppp/pppoe-server-options"' -Ilibevent '-DRP_VERSION="3.15"' -I/usr/include -c -o plugin/discovery.o -fPIC discovery.c
discovery.c: In function ‘sendPADR’:
discovery.c:575:8: warning: array subscript ‘PPPoETag {aka struct PPPoETagStruct}[0]’ is partly outside array bounds of ‘PPPoEPacket[1]’ {aka ‘struct PPPoEPacketStruct[1]’} [-Warray-bounds]
  575 |     svc->type = TAG_SERVICE_NAME;
      |        ^~
discovery.c:554:17: note: while referencing ‘packet’
  554 |     PPPoEPacket packet;
      |                 ^~~~~~
discovery.c:576:8: warning: array subscript ‘PPPoETag {aka struct PPPoETagStruct}[0]’ is partly outside array bounds of ‘PPPoEPacket[1]’ {aka ‘struct PPPoEPacketStruct[1]’} [-Warray-bounds]
  576 |     svc->length = htons(namelen);
      |        ^~
discovery.c:554:17: note: while referencing ‘packet’
  554 |     PPPoEPacket packet;
      |                 ^~~~~~
discovery.c: In function ‘sendPADI’:
discovery.c:359:12: warning: array subscript ‘PPPoETag {aka struct PPPoETagStruct}[0]’ is partly outside array bounds of ‘PPPoEPacket[1]’ {aka ‘struct PPPoEPacketStruct[1]’} [-Warray-bounds]
  359 |         svc->type = TAG_SERVICE_NAME;
      |            ^~
discovery.c:332:17: note: while referencing ‘packet’
  332 |     PPPoEPacket packet;
      |                 ^~~~~~
discovery.c:360:12: warning: array subscript ‘PPPoETag {aka struct PPPoETagStruct}[0]’ is partly outside array bounds of ‘PPPoEPacket[1]’ {aka ‘struct PPPoEPacketStruct[1]’} [-Warray-bounds]
  360 |         svc->length = htons(namelen);
      |            ^~
discovery.c:332:17: note: while referencing ‘packet’
  332 |     PPPoEPacket packet;
      |                 ^~~~~~
gcc -DPLUGIN=1 -g -O2 -fno-strict-aliasing -Wall -Wstrict-prototypes    '-DPPPOE_PATH="/usr/sbin/pppoe"' '-DPPPD_PATH="/usr/sbin/pppd"' '-DPLUGIN_PATH="/etc/ppp/plugins/rp-pppoe.so"' '-DPPPOE_SERVER_OPTIONS="/etc/ppp/pppoe-server-options"' -Ilibevent '-DRP_VERSION="3.15"' -I/usr/include -c -o plugin/if.o -fPIC if.c
gcc -DPLUGIN=1 -g -O2 -fno-strict-aliasing -Wall -Wstrict-prototypes    '-DPPPOE_PATH="/usr/sbin/pppoe"' '-DPPPD_PATH="/usr/sbin/pppd"' '-DPLUGIN_PATH="/etc/ppp/plugins/rp-pppoe.so"' '-DPPPOE_SERVER_OPTIONS="/etc/ppp/pppoe-server-options"' -Ilibevent '-DRP_VERSION="3.15"' -I/usr/include -c -o plugin/common.o -fPIC common.c
common.c: In function ‘dropPrivs’:
common.c:230:9: warning: ignoring return value of ‘setegid’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
  230 |         setegid(getgid());
      |         ^~~~~~~~~~~~~~~~~
common.c:231:9: warning: ignoring return value of ‘seteuid’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
  231 |         seteuid(getuid());
      |         ^~~~~~~~~~~~~~~~~
gcc -DPLUGIN=1 -g -O2 -fno-strict-aliasing -Wall -Wstrict-prototypes    '-DPPPOE_PATH="/usr/sbin/pppoe"' '-DPPPD_PATH="/usr/sbin/pppd"' '-DPLUGIN_PATH="/etc/ppp/plugins/rp-pppoe.so"' '-DPPPOE_SERVER_OPTIONS="/etc/ppp/pppoe-server-options"' -Ilibevent '-DRP_VERSION="3.15"' -I/usr/include -c -o plugin/debug.o -fPIC debug.c
ar -rc plugin/libplugin.a plugin/discovery.o plugin/if.o plugin/common.o plugin/debug.o
gcc -o rp-pppoe.so -shared plugin/plugin.o plugin/libplugin.a 

Type 'make install' as root to install the software.
dfskoll commented 1 year ago

Ugh, wow. :slightly_smiling_face:

OK, when I have time, I will look at the fixes done in the ppp tree and merge them into rp-pppoe (and will probably need to make additional fixes of my own for code that's not in the ppp tree.)

I'll set up an Ubuntu LXC container at some point... but $DAY_JOB is becoming a bit demanding, so no ETA on this.

Regards,

Dianne.

jkroonza commented 1 year ago

@dfskoll that is good to know. I'd like to hear it from @jkroonza to why he needed the one from rp-pppoe package (or better yet trying to understand why anyone would want the one from rp-pppoe package). Again, this is some of the pain / confusion of forking a project ...

They are in my experience inter-changeable. On Gentoo we've been renaming rp-pppoe from ppp package to pppoe for a while now to avoid the naming conflict, and just installed both, user can choose which he prefers to use.

The plugin is only required for kernel mode. user-mode uses pty option which runs a binary specifically encapsulating and decapsulating the pppoe traffic in userspace compared to kernel mode. This is inefficient and won't scale, but it's sufficient in some cases (like running a small number of connections).

My recommendation remains:

Plugin in ppp project (for the pure and simple reason that it's convenient for the majority of pppoe users, just install ppp and you've got pppoe client support - majority of pppoe users).

Everything else in rp-pppoe package.

Gentoo originally had rp-pppoe merged predominantly for client support (early 2000's). I highly doubt the server components would have worked reliably until the work I did in the last couple of years.

dfskoll commented 1 year ago

OK. I think the way forward is to clean up the code so the plugin in the ppp project is up-to-date with the changes that have been made in rp-pppoe; it will then become the One True PPPoE implementation.

On the rp-pppoe side, I think we should get rid of the user-space program pppoe since if ppp ships with the plugin, there's no reason for it. Probably also clean things up to get rid of the DLPI code, the tkpppoe GUI and the pppoe-connect, etc scripts that (I think) nobody uses any more.

rp-pppoe will then become just a relay and server implementation, dependent on the plugin that ships with the ppp project. Does that make sense?

enaess commented 1 year ago

@jkroonza I am slightly confused by your response. Yes, if you use pppd via a pty option to start the pppoe negotiation; I would understand it, but if you run it directly:

/usr/sbin/pppd user testuser@test.iewc.co.za password secret debug maxfail 0 persist connect true plugin pppoe.so bond0.3 nodetach

Wouldn't that start the pppoe via the Ethernet interface directly using a AF_PPPOX socket? Let there be known, I don't know the data path of these packets by just reviewing the code. Looks like rp-pppoe in addition has capability to use BPF filters or AF_PACKET / RAW sockets to grab packets it is interested in. While the rp-pppoe.so does have additional code in if.c (1100 lines vs 250 lines in pppd/pppoe's if.c), I don't see them being so much different.

Could someone please enlighten me? It seems like using kernel mode should be possible using either version of the plugin, and if not; there should be possible to update the pppd/pppoe plugin to do so.

@dfskoll You may want to consider the change history in pppd/pppoe to figure out what have changed and to why. Looks like @pali helped fixing a bunch of stuff here too.

enaess commented 1 year ago

Also, that command line resembles very much how network-manager configures pppd when configuring a pppoe interface: https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/blob/main/src/core/ppp/nm-ppp-manager.c#L798

jkroonza commented 1 year ago

And that is kernel mode :). What we mean with userspace mode is that where kernel mode support is not available pppoe-server will pass pty '/usr/sbin/pppoe -n -I %s -e %u...... -S '%s' to the pppd process, and not use the pppoe.so pppd plugin at all.

With pppoe.so plugin, the plugin will perform discovery etc from userspace, and then pass the details to the kernel such that the pppoe session data does not pass through userspace at all. I believe this is the AF_PPPOX socket mechanism.

I guess it's possible to include userspace support straight into the pppoe.so plugin in cases where it cannot hand-off to the kernel, but I've yet to bump into this being a problem. To be honest, scanning the code for pppoe.c I'm not actually sure how packets are sent/received, but each and every packet passes through this process. Which is why we call it userspace mode.

dfskoll commented 1 year ago

Yes, user mode vs. kernel-mode refers to the session packets. In user mode, they all pass through pppoe but in kernel mode, the session packets are handled entirely by the kernel.

This is the part of pppoe.c that reads data from the pty created by the pppd process and sends a PPPoE packet:

    if (FD_ISSET(0, &readable)) {
        if (conn->synchronous) {
        syncReadFromPPP(conn, &packet);
        } else {
        asyncReadFromPPP(conn, &packet);
        }
    }

and conversely, this bit reads PPPoE packets from the Ethernet interface and sends them to pppd's pty:

    if (FD_ISSET(conn->sessionSocket, &readable)) {
        do {
        if (conn->synchronous) {
            syncReadFromEth(conn, conn->sessionSocket, optClampMSS);
        } else {
            asyncReadFromEth(conn, conn->sessionSocket, optClampMSS);
        }
        } while (BPF_BUFFER_HAS_DATA);
    }
enaess commented 1 year ago

Interesting

Based on my past experiences, I think there was maybe a common misconception that rp-pppoe provided the kernel mode support for configuring pppoe and thus the pppoe.so was taken from the rp-pppoe project. Or perhaps that feature was later implemented in pppd's version of pppoe (or after my experience with pppoe dated ca 2008). When @jkroonza said he needed the rp-pppoe's project's rp-pppoe.so it was because he needed it in context of running a pppoe-server, and that is what pppd's pppoe.so couldn't provide?

In conclusion, either version of (rp-)pppoe supports configuring pppoe w/kernel support where control packets are being handled in user space, but session packets are handled by kernel space. If this is true, then the only reason one really would want to go through the trouble of installing rp-pppoe's version of pppoe.so is if they are running a pppoe-server, right?

Not to add to the confusion, but it looks like the Debian guys package the pppd's version of pppoe.so (with a symlink from rp-pppoe.so that later gets overwritten if one would install the rp-pppoe version of the plugin - @bootc @rfc1036?). Gentoo has been using the rp-pppoe's version of the plugin all along (@jkroonza @samsam?). Not sure what RedHat does, but maybe @yarda would be able to confirm what version they are using, but it all points out that we should be driving this from a single repository at some point.

I guess we need to actually dig in and find the differences in the code and what needs to be carried forward. Another point here is we do know the entire change history since 2002 to pppoe.so via pppd (it's public). What would it take to bring it up to add the server support needed for running it with pppoe-server software? Looking at e.g. if.c, there is a bunch of code removed for handling cases e.g. reading packets via BPF or RAW sockets? Do we still needed these in a "modern" version of pppoe.so?

Lastly, any of this conversation in this thread / issue, it would need a buy-in from @paulusmack for it to even happen.

dfskoll commented 1 year ago

pppd's pppoe.so plugin was taken from rp-pppoe. Both sets of plugins are functionally identical.

In my opinion, the way forward to clean up the code is:

enaess commented 1 year ago

One concern with this approach is that we'll lose the change history from pppd's version of pppoe.so. You repository, the history is lacking between for a time-period of 10+ years, no? One additional question I have is, would it be less work to start from a import from pppd's version of pppoe? What would need to be added?

dfskoll commented 1 year ago

we could merge rp-pppoe --> pppd

PPPoE is an old protocol and IMO there's little of value in the change history. But I have no issues with merging rp-pppoe into pppd if you want to preserve the history.

enaess commented 1 year ago

I don't have a particular beef with any of the two approaches. It's just another point of view, and I wonder if some of the check-points you listed would be a no-op if you started from the other repository. Basically, the least amount of work / changes principle. I don't have the depth of knowledge as to the history of either projects, or the actual code / functionality.

In either case, with a new repository; you'd probably need some of my help setting up autoconf/automake and I can do that.

pali commented 1 year ago

So to make it clear. Linux kernel provides support for encapsulating and decapsulating of IP/IPv6 packets from PPPoE packets on eth interface directly to ppp interface without putting packets into userspace. All other PPPoE packets (e.g. IPCP, LCP, ...) are still passed to userspace where pppd can handle authentication ip address exchange etc...

Normally kernel PPP API requires tty-capable file descriptor on which it starts/creates ppp network interface. Linux kernel allows also file descriptor of connected AF_PPPOX socket. And one option for AF_PPPOX socket is PX_PROTO_OE (PPPoE) with connect address of type struct sockaddr_pppox, which contains interface name of eth interface, MAC address of the peer and PPPoE session id.

ppp's plugin pppoe.so uses only above kernel interface. There is no userspace support of PPPoE implemented in that plugin. So it is Linux-only.

Note that discovery of PPPoE server/concentrator is done in userspace (in pppoe.so plugin) because as described above, discovery is not part of the kernel. And thanks to this, it is possible to use ppp's pppoe.so plugin also for PPPoE server. It is just needed to change userspace session code by another code, which rp-pppoe server implements.

Userspace pppoe solution (which is not part of the ppp project; but is in rp-pppoe) needs to receive raw PPPoE packets from eth inerface, unpack it and then raw PPP packets put into tty (pts) file descriptor which is connected to kernel's ppp.

I hope that I did not make mistake in above description. (if yes, please correct me)

dfskoll commented 1 year ago

ppp's plugin pppoe.so uses only above kernel interface. There is no userspace support of PPPoE implemented in that plugin. So it is Linux-only.

Not quite... PPPoE discovery packets are handled in user-space. The kernel handles PPPoE session packets itself.

enaess commented 1 year ago

@pali - I think I understand now and thank you for that detailed explanation. Even if rp-pppoe's pppoe.so plugin supports user-mode only operation, I am not sure if this is a needed feature of a "modern" pppoe.so plugin. Every essential distribution of Linux seems to provide the AF_PPPOX interface with PX_PROTO_OE out of the box. Either way, if this pppoe plugin is made to be Linux only, I also don't see the need to pull in additional code for user-mode pppoe, and I think this is aligned with what @dfskoll was proposing.

pali commented 1 year ago

Not quite... PPPoE discovery packets are handled in user-space.

Of course! (I wrote this in next sentence of my post.)

I also don't see the need to pull in additional code for user-mode pppoe

Yes! I also do not see any reason for userspace pppoe support (meaning userspace replacement of PX_PROTO_OE) too. AF_PPPOX is available in Linux kernel for a long time.

jkroonza commented 1 year ago

Interesting

Based on my past experiences, I think there was maybe a common misconception that rp-pppoe provided the kernel mode support for configuring pppoe and thus the pppoe.so was taken from the rp-pppoe project. Or perhaps that feature was later implemented in pppd's version of pppoe (or after my experience with pppoe dated ca 2008). When @jkroonza said he needed the rp-pppoe's project's rp-pppoe.so it was because he needed it in context of running a pppoe-server, and that is what pppd's pppoe.so couldn't provide?

The pppoe-server.c code in rp-pppoe assumes the plugin is called rp-pppoe. Thus I "need" it in terms of

./src/Makefile.in:PLUGIN_PATH=$(PLUGIN_DIR)/rp-pppoe.so

Yea, I can probably override that at build time looking at that properly, but at INSTALL time that'll cause a file conflict with ppp's variant (which is the same one really).

In conclusion, either version of (rp-)pppoe supports configuring pppoe w/kernel support where control packets are being handled in user space, but session packets are handled by kernel space. If this is true, then the only reason one really would want to go through the trouble of installing rp-pppoe's version of pppoe.so is if they are running a pppoe-server, right?

No, we don't need it ever. We can use the one from ppp. They are functionally the same, including supported arguments. It's cosmetic changes really.

Not to add to the confusion, but it looks like the Debian guys package the pppd's version of pppoe.so (with a symlink from rp-pppoe.so that later gets overwritten if one would install the rp-pppoe version of the plugin - @bootc @rfc1036?). Gentoo has been using the rp-pppoe's version of the plugin all along (@jkroonza @samsam?). Not sure what RedHat does, but maybe @yarda would be able to confirm what version they are using, but it all points out that we should be driving this from a single repository at some point.

Gentoo installed both, user gets to choose. Which is confusing. pppoe-server resulted in rp-pppoe being used, however, in a client scenario either one would suffice.

I guess we need to actually dig in and find the differences in the code and what needs to be carried forward. Another point here is we do know the entire change history since 2002 to pppoe.so via pppd (it's public). What would it take to bring it up to add the server support needed for running it with pppoe-server software? Looking at e.g. if.c, there is a bunch of code removed for handling cases e.g. reading packets via BPF or RAW sockets? Do we still needed these in a "modern" version of pppoe.so?

Nothing. I've tested pppoe.so by creating a symlink to rp-pppoe.so (after removing the one from rp-pppoe) and it works.

Lastly, any of this conversation in this thread / issue, it would need a buy-in from @paulusmack for it to even happen.

No not really. No changes required to ppp, only on rp-pppoe side. If there were changes made to rp-pppoe.so between ppp forking it and now, @dfskoll should have the history and should be able to decide what should be rebased onto the pppoe from ppp, and file PRs which can then be evaluated on a case by case basis.

One other option should such a merge really be a problem, would be to leave rp-pppoe.so in rp-pppoe, but make compiling it optional, but even when not compiling it allow kernel-mode configuration in pppoe-server and allow specifying the plugin name via argument, eg -k will simply do kernel mode using rp-pppoe.so, but -K pppoe.so could signify to pass plugin pppoe.so rather than plugin rp-pppoe.so ... for example.

I would not make any other changes (including dropping user-space support which is available in rp-pppoe - this can be done at a later stage independently of this endeavour here, I would however highly recommend kernel mode be made the default). There are a bunch of really outdated scripts part of the rp-pppoe project (@dfskoll actually raised this in another discussion), but those kind of changes are to rp-pppoe project and outside the scope of this discussion in my opinion.

Thus in my opinion only really two approaches:

  1. Drop the rp-pppoe.so plugin entirely from rp-pppoe package, and always call pppd with "plugin pppoe.so". This should go with a merge of any relevant changes that @dfskoll made in rp-pppoe to the plugin over the years into the ppp package.
  2. Detach pppoe-server kernel mode support from compiling the package, and make compiling the plugin a separate from that.

In both cases I suspect making the argument to pppd's plugin option start-time configurable at the very least.

I'd recommend option 1 purely for the sake of having one source of truth. The changes @dfskoll made over the years could be done as a patch series on ppp, thus thereby gaining the change history. Of course it would need to be rebased then. I'd offer to do this work, but unfortunately only @dfskoll has this history available.

dfskoll commented 1 year ago

The problem with continuing to support user-mode PPPoE in rp-pppoe is that there would be a substantial amount of source code overlap with the plugin in ppp. This, IMO, argues for removing pppoe.so from ppp and having rp-pppoe be the single source of truth.

I think we should drop user-mode PPPoE support. There's no reason for it any more.

As for patches to ppp's plugin code, I wasn't planning on going through all of my git history anyway. I was just going to compare the code and extract the differences and merge as appropriate. (But this won't happen for a while as I am busy at the moment...)

enaess commented 1 year ago

In either approach I would advocate for removing the pppoe.so from both pppd and rp-pppoe project in favor of establishing a separate git repository under the ppp-project umbrella. This would require @paulusmack to create / set it up, import the files, adding a separate autoconf / automake files, add additional patches, communicate these changes with packagers for various platforms. As I pointed out earlier, there are two ways of going about it, import the files from rp-pppoe's project, or import the files from pppd's project. In the end, I think you'll find that the proposed changes for removing the user-space code, and misc cleanup already done in pppd may be the shorter path. Also, a bonus would be to do this in a fashion that would preserve git history. Another thing that could favor an import from pppd's pppoe.so module is that it's been in a public git repository dating back to when the fork was made. @dfskoll's repository was scrubbed for various changes pertaining to work performed in conjunction with a company. This of course runs the risk of proprietary code bleeding over unless that process was carefully done.

dfskoll commented 1 year ago

This of course runs the risk of proprietary code bleeding over unless that process was carefully done.

The proprietary code was kept completely separate right from the beginning; there's no proprietary code in the public rp-pppoe repos.

There have been fixes in rp-pppoe that have not been made to the version of pppoe.so in the ppp project.

jkroonza commented 1 year ago

@enaess I do not understand the motivation for a separate repository here. Could you please clarify? Not my call at all, but I kind feel responsible for this tangent getting started.

pppoe-server cannot run without ppp anyway.

I believe pppoe-bridge should be able to, but honestly I'm struggling to think of a practical use-case for pppoe-bridge.

Killing the user-space implementation may completely get rid of the pppoe-discovery code in rp-pppoe, thus eliminating even that small amount of duplicated code. There may be more duplicated code than this, but that is which I'm aware off.

Currently I know ppp also functions on Solaris, it looks like BSD support was discontinued.

How does pppoe and pppol2tp work on solaris - if at all?

enaess commented 1 year ago

@jkroonza I believe the entire motivation behind this discussion was to establish a single source of "truth" - a single code base / repository going forward. This is to

The pppoe and pppol2tp was never ported or built on Solaris. In fact the entire set of plugin directories is not built on Solaris.

jkroonza commented 1 year ago

@enaess that is my understanding too. Agree with everything you stated above.

My understanding from the above is that there is no pppoe on Solaris without the userspace program from rp-pppoe?

So if there are users using that, it means that dropping that would move things backwards.

If we drop that, then I don't see any reason for creating a third repository.

If we want to keep it (I personally could not care less seeing that I don't use that, but I also don't want to anger users for no good reason) that would then motivate a possible third repo for pppoe rather, let's call it ppp-pppoe for the sake of just giving it a name which can build either the userspace program, or the ppp plugin (on linux at least), or both.

dfskoll commented 1 year ago

My understanding from the above is that there is no pppoe on Solaris without the userspace program from rp-pppoe?

It's my understanding that Solaris has its own kernel-mode implementation of PPPoE that no longer uses rp-pppoe, and I believe the same for FreeBSD.

The DLPI and BPF code in rp-pppoe clutters the code with #ifdefs and makes it hard to follow. I'm nuking it; RP-PPPoE will become Linux-only. And I will also nuke support for non-kernel-mode PPPoE since nobody should be using that in 2023. That will significantly simplify and clean up the code.

I also do not see the need for a third repo. The PPPoE plugin can live under pppd/plugins and RP-PPPoE (the server and relay implementation) can stay where they are; I don't see any advantage to putting them under the ppp project.

Regards,

Dianne.