Page MenuHomeFreeBSD

ng_ether should notify lower and orphan nodes when underlying interface link layer address is changed
Needs ReviewPublic

Authored by eugen_grosbein.net on Aug 20 2017, 4:13 PM.

Details

Reviewers
avg
mav
Summary

ng_ether(4) represents ethernet interfaces for netraph(4) kernel networking subsystem. It can have lower and/or orphan hooks connected to nodes sending raw Ethernet frames like ng_pppoe(4). Such peers generally need to known actual link layer address of the interface in question.

This change teaches ng_ether(4) to notify its peers when underlying interface link layer address is changed so they can update their notion of the address.

Test Plan

Run pair of net/mpd5 instances, one as PPPoE server and anothere as PPPoE client.
Establish PPPoE connection, then change MAC address of client's NIC with ifconfig(8).
The connection does not break if patch is applied and tcpdump(1) shows that ng_pppoe follows new MAC.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

While I understand your valid motivation, I can't say that I very much like this in patch for several reasons:

  • NGM_ETHER_SET_ENADDR is a control command supposed to be targeted to ng_ether node, not from it, and it is a action request, not notification. I suppose if you connect two ng_ether nodes with lower hooks to create poor mans bridge, this won't work as expected.
  • ng_pppoe has method to explicitly set MAC address, and it may get overwritten by this, which may or may not be correct.
  • We just fought one locking issue, and here I don't see any hook locks or even references (I understand that your problem is narrower, but still).
sys/netgraph/ng_ether.c
464

Do you think the case when lower and orphan hooks are connected to the same node is realistic enough to make this code so tangled?

PS: Read how to create patches with context for the phabricator. May be it is a problem of phabricator and it modified the diff after you, if I download the raw diff here, there is no even normal diff -p context, which is almost always useful.

In D12086#258234, @mav wrote:

While I understand your valid motivation, I can't say that I very much like this in patch for several reasons:

  • NGM_ETHER_SET_ENADDR is a control command supposed to be targeted to ng_ether node, not from it, and it is a action request, not notification.

Why cannot we extent its meaning then?

I suppose if you connect two ng_ether nodes with lower hooks to create poor mans bridge, this won't work as expected.

Why? A bridge should have single MAC address anyway.

  • ng_pppoe has method to explicitly set MAC address, and it may get overwritten by this, which may or may not be correct.

Yes. We have no "authorization" at this level and that would be caller's responsibility to supply correct address at correct moment.

  • We just fought one locking issue, and here I don't see any hook locks or even references (I understand that your problem is narrower, but still).

I do not see why additional locking may be needed here.

In D12086#258234, @mav wrote:

Do you think the case when lower and orphan hooks are connected to the same node is realistic enough to make this code so tangled?

I think 1) code should be correct and cover all cases, 2) it should be fast in common case (so the change starts with "lower" hook), 3) code bloat is bad, and 4) yes, code should be understandable enough.

As for 4, I admit the code is a bit too dense but it is small enough to understand, so 1-3 justifies it. Nevertheless, I just expanded commentary to describe it better.