Page MenuHomeFreeBSD

netgraph: Fix ng_ether's shutdown handing
ClosedPublic

Authored by markj on Dec 17 2020, 10:48 PM.

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 OK
Unit
No Unit 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.