Page MenuHomeFreeBSD

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

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
julian edited edge metadata.Jan 22 2017, 11:37 AM

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
111 ↗(On Diff #24264)

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
855 ↗(On Diff #24264)

can you fix the typo here while you are here.. of -> or

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

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

sys/netgraph/ng_pppoe.c
952 ↗(On Diff #24264)

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
ale edited edge metadata.

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 edited edge metadata.
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
108 ↗(On Diff #25849)

"host uniq" should be capitalized or otherwise identified with markup. Since it's used below, just be consistent and say Host-Uniq.

109 ↗(On Diff #25849)

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?

113 ↗(On Diff #25849)

(Not yours, I know.)
s/broadcasted/broadcast/

117 ↗(On Diff #25849)

s/the following/this/

118 ↗(On Diff #25849)

Markup could show these with out-of-band highlighting rather than quotes. But I'm not sure which markup to use.

274 ↗(On Diff #25849)

Rewording:

The argument is the URL to be delivered to the client:
281 ↗(On Diff #25849)
The argument is the message to be delivered to the client:
292 ↗(On Diff #25849)

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

297 ↗(On Diff #25849)

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
108 ↗(On Diff #25849)

Are you fine with lower case "access concentrator"?

109 ↗(On Diff #25849)

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.

118 ↗(On Diff #25849)

I'll make a few tries, .Sq might be a good choice

292 ↗(On Diff #25849)

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
ale edited edge metadata.

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 ↗(On Diff #25977)

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
1135 ↗(On Diff #25977)

This is deprecated macro. Please use m_gethdr(M_NOWAIT, MT_DATA);

1144 ↗(On Diff #25977)

This is already done by the allocator. Not needed.

1145 ↗(On Diff #25977)

Looks like m_pkthdr.len is never read before it is overwritten later in L1167.

1180 ↗(On Diff #25977)

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
ale edited edge metadata.

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!

Any news on this? :)

ale added a comment.Feb 13 2018, 4:47 PM

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, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.