Page MenuHomeFreeBSD

ng_ether: Enter NET_EPOCH where required
ClosedPublic

Authored by kp on Aug 29 2020, 8:55 AM.

Details

Summary

We must enter NET_EPOCH before calling ether_output_frame(). Several of
the functions it calls (pfil_run_hooks, if_transmit) expect to be
running in the NET_EPOCH.

While here extend the NET_EPOCH coverage in ng_ether_rcv_upper() to also
cover BRIDGE_INPUT, as that also expects to run in NET_EPOCH.

PR: 248958

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kp requested review of this revision.Aug 29 2020, 8:55 AM
This revision is now accepted and ready to land.Aug 29 2020, 8:56 AM

Address Gleb's feedback in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=248958.

TL;DR: "So, this is ng_pppoe's ng_pppoe_rcvmsg() that needs to enter epoch"

This revision now requires review to proceed.Aug 29 2020, 10:08 PM
This revision is now accepted and ready to land.Aug 30 2020, 8:46 AM
glebius added inline comments.
sys/netgraph/ng_ether.c
735 ↗(On Diff #76358)

This epoch entrance after /* Route packet back in */ was added by myself in r353292. Now that ng_socket and other entrance nodes are fixed, I'm not sure it is still needed. Of course your patch makes it more consistent - covering BRIDGE_INPUT, too.

Have you tested with this NET_EPOCH_ENTER removed but with ng_pppoe_rcvmsg() epoch in place?

sys/netgraph/ng_pppoe.c
777 ↗(On Diff #76358)

There are only three messages that initiate sending data: CONNECT, SEND_HURL and SEND_MOTM. Makes sense to reduce scope of the epoch only to those places? It is more about a self-documenting code rather than about performance. I'm not insisting on that, just suggesting. I'm marking suggested places down below.

989 ↗(On Diff #76358)

#1 This call needs net epoch.

1175 ↗(On Diff #76358)

#2 this needs net epoch

1218 ↗(On Diff #76358)

#3 this needs net epoch

  • Remove NET_EPOCH_ENTER() from ng_ether_rcv_upper() (As yet untested)
  • Only enter NET_EPOCH for the few cases that explicitly require it in ng_pppoe_rcvmsg()
This revision now requires review to proceed.Aug 31 2020, 3:41 PM

Remove code that wasn't supposed to be there.

Reduce churn in the patch

This looks good to me. Thanks a lot! Please commit if it passes tests.

This revision is now accepted and ready to land.Aug 31 2020, 5:20 PM

Tested-by: manu

Thanks a lot.

This revision was automatically updated to reflect the committed changes.