Details

Reviewers

wblock
julian
mav
adrian
glebius
franco_opnsense.org
eugen_grosbein.net

Group Reviewers

manpages
network

Commits

rS331059: MFC r329279: add support for user-supplied Host-Uniq tag to ng_pppoe(4).
rS331058: MFC r329279: add support for user-supplied Host-Uniq tag to ng_pppoe(4).
rS329279: ng_pppoe(8): add support for user-supplied Host-Uniq tag.

Summary

Add support for user-supplied Host-Uniq tag in Netgraph PPPoE.
A few ISP filter PADI requests based on such tag, to force the use of their own routers.
The custom Host-Uniq tag is passed in the NGM_PPPOE_CONNECT control message, so it can be used with FreeBSD ppp(8) and mpd without any other change.

Add support to send and receive PADM messages, HURL and MOTM, often used by service providers to provide ACS information and other configuration settings to the user CPE.

Test Plan

I've tested the three possible Host-Uniq tags (auto-generated as before, long plain string and long binary string) with my own ISP connection.

I've tested the PADM messages with MPD5, as both client and server.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Herald added a reviewer: manpages. · View Herald TranscriptJan 21 2017, 8:12 AM

Herald added a subscriber: imp. · View Herald Transcript

Considering how much work went into writing this and the fact that we had to get it approved by verizon (or whatever they were called at the time) its amazing that I remember almost nothing about how it works.

share/man/man4/ng_pppoe.4
110	There are ways to set binary values. though this will do. I f we decide it's limiting, we can improve it.
sys/netgraph/ng_pppoe.c
918	can you fix the typo here while you are here.. of -> or

as far as I can see it looks good. Same default value as before..

sys/netgraph/ng_pppoe.c
1015	same typo..guess I copied it

This revision is now accepted and ready to land.Jan 22 2017, 12:19 PM

Could we have concrete examples for the added HOST-UNIQ tags in the manual page, one working example per possible syntax (and possibly anonymized from ale@'s particular ISP, but maintaining syntax)? Please?

Fixed typos and improved man page.

This revision now requires review to proceed.Jan 22 2017, 2:16 PM

Herald edited edge metadata. · View Herald TranscriptJan 22 2017, 2:16 PM

ale marked 2 inline comments as done.Jan 22 2017, 2:17 PM

adrian accepted this revision.Jan 23 2017, 6:54 PM

adrian added a reviewer: adrian.

This revision is now accepted and ready to land.Jan 23 2017, 6:54 PM

poolroom_gmail.com added a subscriber: poolroom_gmail.com.Feb 25 2017, 12:27 AM

add client and server support for PADM messages, HURL and MOTM
Fix a regression in PPPoE server introduced by previous patch version
handle missing service name tag in PADI requests to cope with non fully compliant client implementations, treat it as empty service name

This revision now requires review to proceed.Mar 1 2017, 10:35 PM

Herald edited edge metadata. · View Herald TranscriptMar 1 2017, 10:35 PM

It's been years since I worked on this, but I see no problems from this.

This revision is now accepted and ready to land.Mar 2 2017, 5:43 AM

wblock added a subscriber: wblock.Mar 3 2017, 5:13 PM

wblock added inline comments.

share/man/man4/ng_pppoe.4
107	"host uniq" should be capitalized or otherwise identified with markup. Since it's used below, just be consistent and say `Host-Uniq`.
107–119	(Not yours, I know.) s/broadcasted/broadcast/
108	The formatting looks odd, but is apparently meant to show that there is an optional AC-Name followed by double backslashes if present, followed by an optional Host-Uniq with a vertical bar if present. Does this render correctly?
123	s/the following/this/
124	Markup could show these with out-of-band highlighting rather than quotes. But I'm not sure which markup to use.
285	Rewording: The argument is the URL to be delivered to the client:
292	The argument is the message to be delivered to the client:
303	"may" usually implies "you have permission", while "can" means "it is possible". So: message with a HURL tag is received, and contains a URL which the Host can Should Host be capitalized?
308	As above: Minute which the Host can display to the user.

Thanks for your comment, I'll improve the man page.

share/man/man4/ng_pppoe.4
107	Are you fine with lower case "access concentrator"?
108	The double backslash is just for man page escaping, it's actually a single backslash. Both AC-Name and Host-Uniq are optional, that's the reason for different separators.
124	I'll make a few tries, .Sq might be a good choice
303	It's MAY in RFC 2119 meaning, but I'll change it to "can". I capitalized the Host to make it clearer what's the meaning of the H in HURL, but probably is not needed.

Man page improvements

This revision now requires review to proceed.Mar 4 2017, 4:50 PM

Herald edited edge metadata. · View Herald TranscriptMar 4 2017, 4:50 PM

ale marked 14 inline comments as done.Mar 4 2017, 4:52 PM

One suggestion, but looks good otherwise. Thank you!

share/man/man4/ng_pppoe.4
115	Would prefer "like" or "for example" to exempli gratia here (see `igor -y ng_pppoe.4`), but if you want to stick with that, it should be "e.g.,". Yes, it's clunky. In this case, I prefer "like".

This revision is now accepted and ready to land.Mar 24 2017, 4:03 PM

Who is going to commit it in the src tree and merge in 11 branch?

This revision now requires review to proceed.Apr 1 2017, 7:21 AM

I got few minor comments.

sys/netgraph/ng_pppoe.c
1138	This is deprecated macro. Please use m_gethdr(M_NOWAIT, MT_DATA);
1147	This is already done by the allocator. Not needed.
1148	Looks like m_pkthdr.len is never read before it is overwritten later in L1167.
1183	Same comments on this block as on SEND_HURL.

This revision now requires changes to proceed.Apr 4 2017, 2:37 PM

The three mentioned comments apply also to the ng_pppoe_disconnect function at line 2030 from which I took inspiration, do you want me to change that function, too?

Addressed latest comments, updated also the pppoe disconnect function.
Please check the correctness, and commit the patch if you are satisfied.

ale marked 5 inline comments as done.Apr 4 2017, 8:20 PM

mleone87_hotmail.com added a subscriber: mleone87_hotmail.com.Jun 1 2017, 2:31 PM

franco_opnsense.org added a subscriber: franco_opnsense.org.Jun 8 2017, 11:25 AM

In D9270#212547, @ale wrote:

Addressed latest comments, updated also the pppoe disconnect function.
Please check the correctness, and commit the patch if you are satisfied.

How the tag should be defined in mpd.conf then?

set auth host-uniq "string" ?

Thanks

How the tag should be defined in mpd.conf then?

set auth host-uniq "string" ?

No, to just set the host-uniq string you should use:

set pppoe service "string|"

We do seem to have a persistent problem with this patch in some PPPoE environments that will cause a crash in ng_pppoe_rcvdata_ether():

https://ibb.co/mRWKHF
https://ibb.co/iOxRxF
https://forum.opnsense.org/index.php?topic=5697.0

Please advise.

In D9270#251087, @franco_opnsense.org wrote:

We do seem to have a persistent problem with this patch in some PPPoE environments that will cause a crash in ng_pppoe_rcvdata_ether():

https://ibb.co/mRWKHF
https://ibb.co/iOxRxF
https://forum.opnsense.org/index.php?topic=5697.0

Please advise.

Your problem does not seem to be connected to this review. Please use Bugzilla https://bugs.freebsd.org/ and submit bug report there specifying exact FreeBSD version your system is based on and steps needed to reproduce the problem.

Thanks, but we've narrowed it down to this commit.

In D9270#251108, @franco_opnsense.org wrote:

Thanks, but we've narrowed it down to this commit.

Anyway, Bugzilla should be used for bug reports. Don't forget to describe how did you narrowed it to the commit in your PR.

No, I'm not asking for support that would take a few weeks of ping pong in a bug tracker, if any. This is real world feedback for this review. Take it or leave it. :)

No, I'm not asking for support that would take a few weeks of ping pong in a bug tracker, if any. This is real world feedback for this review. Take it or leave it. :)

I was going to deal with this problem but I need mentioned details to start with. But if you are not going to submit them in a PR, you hardly will get a solution. And this is NOT right place for bug processing, Bugzilla is.

garga added a subscriber: garga.Aug 22 2017, 12:07 PM

The updated patch fixes the reported issue on multiple PADO replies after the session has been already established.

this one is stable, thank you @ale

eugen_grosbein.net accepted this revision.Aug 29 2017, 4:18 PM

mandree removed a subscriber: mandree.Aug 29 2017, 9:42 PM

Man page changes look good to me. Please remember to bump .Dd. Thanks!

Any news on this? :)

I'd like to see this in the next 11.2-RELEASE. @eugen_grosbein.net can you please commit it and MFH?

This revision was not accepted when it landed; it landed in state Needs Review.Feb 14 2018, 9:18 PM

Closed by commit rS329279: ng_pppoe(8): add support for user-supplied Host-Uniq tag. (authored by eugen). · Explain Why

This revision was automatically updated to reflect the committed changes.

Add support for user-supplied Host-Uniq tag and handle PADM messages in Netgraph PPPoE
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 32487

share/man/man4/ng_pppoe.4

sys/netgraph/ng_pppoe.h

sys/netgraph/ng_pppoe.c

Add support for user-supplied Host-Uniq tag and handle PADM messages in Netgraph PPPoEClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 32487

share/man/man4/ng_pppoe.4

sys/netgraph/ng_pppoe.h

sys/netgraph/ng_pppoe.c

Add support for user-supplied Host-Uniq tag and handle PADM messages in Netgraph PPPoE
ClosedPublic
Actions

Revision Contents
Changeset List