netgraph/ng_bridge: Introduce "uplink" ports without MAC learning

[donner]

Summary

The ng_bridge(4) node is designed to work in moderately small environments. Connecting such a node to a larger network rapidly fills the MAC table for no reason. It even become complicated to obtain data from the gettable message, because the result is too large to transmit.

This patch introduces, two new functionality bits on the hooks:

• Allow or disallow MAC address learning for incoming patckets.
• Allow or disallow sending unknown MACs through this hook.

Uplinks are characterized by denied learing while sending out unknowns.
Normal links are charaterized by allowed learning and sending out unknowns.

In a later patch the node will have a "private" mode, where normal links do not sent out unknowns. This mode will be extended to more "intelligent" filtering, so that ARP or ND will only be passed to the correct link, not distributed to all others.

A further step is to implement ICMP sniffing and controlling multicast distribution.

Test Plan

First create a bridge between host and outside world and attach a sniffer node.

# ngctl
+ mkpeer bridge b link99
+ connect bge0: b upper link0
+ connect bge0: b lower uplink1
+ mkpeer b eiface link1 ether
# tcpdump -nei ngeth1

Observe the traffic for a while, then query the MAC table:

# ngctl msg: bge0.upper gettable

For an unpatched node, you will see either a large table, a binary dump, or an error indicating insufficent socket space.

For a patched node, you will see your own host mac only.

[afedorov]

Is it really useful to have multiple uplinks?

[donner]

In D23963#526951, @aleksandr.fedorov_itglobal.com wrote:

Is it really useful to have multiple uplinks?

That's a very good question.

IMHO alternative solutions are:

• A single "uplink" hook, which is almost the same code complexity, while losing the fexibility
• A new API command to alter the flags of each hook individually (which is a mid term change)

I choosed "uplinkX" to provide a simple as well as atomic way to create new hooks into a already busy bridge.

What's your idea?

[afedorov]

I look at this issue from network virtualization point of view.
I have a plan (and patches) for adding native Netgraph support to the Bhyve network backend through ng_socket(4).
For this case, it is very interesting to be able to create ng_bridge(4) with the following options:

• One "uplink" hook with learning turned off, but all unknown MAC's go through it.
• The rest of the hooks have learning enabled, but unknown MAC's are not sent to them.

We use a similar algorithm with vale (4) + bhyve.

I want to create such networks:

[Bhyve vm's or Jail's] --- links[1...N](learnMac = 1, sendUnknown = 0) --- ng_bridge(4) --- link0 (learnMac = 0, sendUnknown = 1) ---- ng_ether(4)

Therefore, I think the second option is better:

• Add flags for each hook: learnMac, sendUnknown (you've already done this).
• Add new messages NGM_BRIDGE_ (GET | SET) _FLAGS.
• If desired, add default flags for each bridge that initialized the new linkX hook and related messages.

[afedorov]

sys/netgraph/ng_bridge.c

94

Maybe better to use unsigned values.

342

To match the style: *prefix

[bcr]

You can check the man page using textproc/igor and "mandoc -Tlint".

share/man/man4/ng_bridge.4

101

You need to make a line break after a sentence stop.

[dab]

share/man/man4/ng_bridge.4

100

s/does no/does not/

101

I don't think it is common usage to start a sentence with "i.e.", nor is it common usage to capitalize such an abbreviation. Such an abbreviation is usually followed by a comma. Suggest:

the internal address table small; i.e., it's desirable to connect

That would also address the newline after full stop issue. :-)

107

s/send out/sent out/

187

s/quering/querying/

[danfe]

share/man/man4/ng_bridge.4

101

Apart from the above (line break, comma after i.e.), contraction should be expanded: it's -> it is.

[donner]

Fix various man page issues.

[donner]

Fix issues in the code, i.e. bitfields are unsigned, spacing style, braces style.

[donner]

Rebase to r366170.

[donner]

Converted local repository from subversion to git

Added:

• Determine the ng_bridge forwarding mode from the order of hooks
• Update manual page for modified default behaviour

I do have massive problems with "unknown" MACs, i.e. packets send to
the uplink hook. The default behaviour of the bridge is to replicate
all those frames to all hooks, which cause cognestion on downlink
interfaces. So the change is obvious.

In order to keep the default behaviour for existing setups, and keep
the API unchanged, the trigger is simple: If the first attached hook
is a new "uplink" hook, the new behaviour is enabled.

Separate API calls to obtain and modify the per hook settings will
come later.

[donner]

Remove the functional extension from this review. It will come as a
separate stacked one to ease reviewing.

This code has been running in production since more than a year.

[afedorov]

sys/netgraph/ng_bridge.c

541

Is this debug output really needed?

[donner]

• Remove debug output

[kp]

Approved by: kp (mentor)

sys/netgraph/ng_bridge.c

383

This check is redundant. malloc(M_WAITOK) never fails. It may wait forever, but will never return NULL.

This was already redundant before this patch, so we can address this later.

[yuripv]

share/man/man4/ng_bridge.4

160–161

just a note as .Dv seems to be misused throughout this entire man page -- .Dv does not seem to be the correct choice for struct ng_bridge_link_stats, use .Vt? and for link, may be .Va?

sys/netgraph/ng_bridge.c

454–455

no space between * and variable names.

455

const?

533

might be more readable with braces around multiline statements

[donner]

Incooperate suggestions.

[donner]

sys/netgraph/ng_bridge.c

383

Adding a link to a busy bridge should not be stall the bridge function, so I'd prefer to fail.

455

good point

533

This would violate style(9) ... at least a bit.

[donner]

share/man/man4/ng_bridge.4

160–161

I'd like to keep it consistent for the moment and fix this issue in a separate review.