Page MenuHomeFreeBSD

netgraph: Fix ng_ether's shutdown handing
ClosedPublic

Authored by markj on Dec 17 2020, 10:48 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Oct 15, 8:29 PM
Unknown Object (File)
Wed, Oct 15, 8:29 PM
Unknown Object (File)
Wed, Oct 15, 10:08 AM
Unknown Object (File)
Fri, Oct 10, 3:11 PM
Unknown Object (File)
Sat, Sep 20, 7:08 PM
Unknown Object (File)
Aug 14 2025, 4:51 AM
Unknown Object (File)
Jul 25 2025, 2:42 PM
Unknown Object (File)
Jul 19 2025, 11:30 AM

Details

Summary

When tearing down a VNET, netgraph sends shutdown messages to all of the
nodes before detaching interfaces (SI_SUB_NETGRAPH comes before
SI_SUB_INIT_IF in teardown order). ng_ether nodes handle this by
destroying themselves without detaching from the parent ifnet. Then,
when ifnets go away they detach their ng_ether nodes again, triggering a
use-after-free.

Handle this by modifying ng_ether_shutdown() to detach from the ifnet.
If the shutdown was triggered by an ifnet being destroyed, we will clear
priv->ifp in the ng_ether detach callback, so priv->ifp may be NULL.

Also get rid of the printf in vnet_netgraph_uninit(). It can be
triggered trivially by ng_ether since ng_ether_shutdown() persists the
node unless NG_REALLY_DIE is set.

PR: 233622
Reported by: KASAN

Test Plan

sys/net/if_vlan:basic triggers a KASAN violation when ng_ether is loaded. This commit fixes that.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 35501
Build 32406: arc lint + arc unit

Event Timeline

markj added a reviewer: kp.

I was confused by your "we will clear priv->ifp in the ng_ether detach callback" sentence.
Clarified for me.

sys/netgraph/ng_ether.c
760

So this is the only effective line in the patch.
It sets the reference from the interface (ipf) to the node (if_l2com) ... to zero.

This revision is now accepted and ready to land.Dec 18 2020, 1:29 AM
In D27662#618351, @lutz_donnerhacke.de wrote:

I was confused by your "we will clear priv->ifp in the ng_ether detach callback" sentence.
Clarified for me.

I was referring to ng_ether_detach(). That callback is invoked when an ethernet interface with an attached ng_ether device is destroyed, and it destroys the ng_ether node.

sys/netgraph/ng_ether.c
760

That's correct.

I don't really like how ng_etner(4) is implemented as a whole. But I think this patch is correct and should be committed.

sys/netgraph/ng_base.c
3172

BTW:

  • I am unsure why we do not just set NGF_REALLY_DIE before the first attempt.
  • I don't know why the code doesn't use NG_NODE_REALLY_DIE().

I don't really like how ng_etner(4) is implemented as a whole. But I think this patch is correct and should be committed.

Could you explain what you would change about it? I'm not familiar enough with ng_ether to know all of its warts.