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

[ale]

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.

[julian]

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]

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

[mandree]

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]

Fixed typos and improved man page.

[ale]

• 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

[mav]

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

[wblock]

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]

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]

Man page improvements

[wblock]

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

[ale]

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

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

[ale]

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]

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

[mleone87_hotmail.com]

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]

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

[franco_opnsense.org]

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.

[eugen_grosbein.net]

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.

[franco_opnsense.org]

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

[eugen_grosbein.net]

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.

[franco_opnsense.org]

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. :)

[eugen_grosbein.net]

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.

[ale]

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

[franco_opnsense.org]

this one is stable, thank you @ale

[wblock]

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

[franco_opnsense.org]

Any news on this? :)

[ale]

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