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

Authored by ale on Jan 21 2017, 8:12 AM.

Details

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
Lint
Lint Skipped
Unit
Unit Tests Skipped
There are a very large number of changes, so older changes are hidden. Show Older Changes
ale set the repository for this revision to rS FreeBSD src repository.Jan 21 2017, 8:12 AM
ale added subscribers: freebsd-net-list, mandree.

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

julian accepted this revision.Jan 22 2017, 12:19 PM

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
mandree added a comment.EditedJan 22 2017, 12:35 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?

ale updated this revision to Diff 24314.Jan 22 2017, 2:16 PM

Fixed typos and improved man page.

This revision now requires review to proceed.Jan 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
ale updated this revision to Diff 25849.Mar 1 2017, 10:35 PM
ale retitled this revision from Add support for user-supplied Host-Uniq tag in Netgraph PPPoE to Add support for user-supplied Host-Uniq tag and handle PADM messages in Netgraph PPPoE.
ale updated this object.
ale edited the test plan for this revision. (Show Details)
ale added a subscriber: mav.
  • 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
mav accepted this revision.Mar 2 2017, 5:43 AM
mav added a reviewer: mav.

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.
ale added a comment.Mar 3 2017, 10:13 PM

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.

ale updated this revision to Diff 25977.Mar 4 2017, 4:50 PM

Man page improvements

This revision now requires review to proceed.Mar 4 2017, 4:50 PM
ale marked 14 inline comments as done.Mar 4 2017, 4:52 PM
wblock accepted this revision.Mar 24 2017, 4:03 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
ale added a comment.Apr 1 2017, 7:21 AM

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
glebius requested changes to this revision.Apr 4 2017, 2:37 PM
glebius added a subscriber: glebius.

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
ale added a comment.Apr 4 2017, 7:40 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?

ale updated this revision to Diff 27059.Apr 4 2017, 8:19 PM

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
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

ale added a comment.Jun 8 2017, 3:30 PM

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.

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.

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
ale updated this revision to Diff 32487.Aug 29 2017, 3:41 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

mandree removed a subscriber: mandree.Aug 29 2017, 9:42 PM
wblock added a comment.Sep 1 2017, 8:41 PM

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