ng_iface(4): Set the current VNET before calling netisr_dispatch().

[markj]

Summary

When a ng_tty node receives a byte or packet from the device, it sends
the data up through its hook. Once it makes its way up to an ng_iface
node, it gets dispatched through netisr, so the thread must have a VNET
context set.

By default ng_tty will use direct dispatch, i.e., the thread calling the
TTY hook will pass packets all the way up to netisr. However, it does
not set a VNET context, so in this case we crash.

Work around the problem by setting the current VNET before calling
netisr_dispatch().

Test Plan

See PR 242406, the patch fixes a problem report in opnsense's bug tracker.

[donner]

ng_tty is the wrong place for deciding this question. Almost any netgraph node is able to send data over a hook, most of them are VNET agnostic. The correct vnet depends on the context of the ng_iface node.

So - in order to handle this problem - the ng_iface node need to validate the incoming data messages and

• supply the missing VNET information
• overwrite(?) a wrong VNET information

[markj]

In D25788#571211, @lutz_donnerhacke.de wrote:

ng_tty is the wrong place for deciding this question.

Well, yes, so the patch "fixes" the problem by forcing an ngthread to decide the question. I don't know how to correctly modify ng_iface to do the right thing yet.

Almost any netgraph node is able to send data over a hook, most of them are VNET agnostic. The correct vnet depends on the context of the ng_iface node.

So - in order to handle this problem - the ng_iface node need to validate the incoming data messages and

• supply the missing VNET information
• overwrite(?) a wrong VNET information

Why is it not sufficient for ng_iface to simply set the current vnet in ng_iface_rcvdata()? Or, why does ng_send_item() not set the current VNET to that of the receiving hook's node? How can VNET information be wrong?

[markj]

This comment was removed by markj.

[donner]

In D25788#571336, @markj wrote:

In D25788#571211, @lutz_donnerhacke.de wrote:

So - in order to handle this problem - the ng_iface node need to validate the incoming data messages and

• supply the missing VNET information
• overwrite(?) a wrong VNET information

Why is it not sufficient for ng_iface to simply set the current vnet in ng_iface_rcvdata()?

Currently I tend to believe, that overriding the VNET setting is the correct solution.

Or, why does ng_send_item() not set the current VNET to that of the receiving hook's node? How can VNET information be wrong?

Assume two ng_iface nodes in different VNETs. Which VNET is correct in which situation?

[markj]

In D25788#571344, @lutz_donnerhacke.de wrote:

In D25788#571336, @markj wrote:

In D25788#571211, @lutz_donnerhacke.de wrote:

So - in order to handle this problem - the ng_iface node need to validate the incoming data messages and

• supply the missing VNET information
• overwrite(?) a wrong VNET information

Why is it not sufficient for ng_iface to simply set the current vnet in ng_iface_rcvdata()?

Currently I tend to believe, that overriding the VNET setting is the correct solution.

Or, why does ng_send_item() not set the current VNET to that of the receiving hook's node? How can VNET information be wrong?

Assume two ng_iface nodes in different VNETs. Which VNET is correct in which situation?

Each netgraph node has an associated VNET, the VNET in which the node was created. When sending a message to a hook, the current VNET should be set to that of the receiving node. When receiving a message from a hook, the current VNET should be set to that of the node if the message is going to be injected into the network stack. If netgraph always takes care of setting the current VNET, then individual node functions do not need to do anything.

ngthread() implements this by simply setting the current VNET before passing any message through a hook. The same logic is not implemented for the direct dispatch case for some reason, but I suspect that doing the following in ng_snd_item() would be correct and sufficient:

CURVNET_SET(node->nd_vnet);
error = ng_apply_item(node, item, rw);
CURVNET_RESTORE();

Alternately, ng_iface could set the current VNET to that of the associated ifp before calling netisr_dispatch(). ng_eiface effectively does this because received packets are passed to ether_input(), which sets the current VNET.

In fact, for ng_iface it is possible for the node and ifnet VNETs to become out of sync: if I create two ng_iface interfaces and pass one into a jail, the node's VNET is not updated but the ifnet's VNET is updated. I'm not sure if this is intentional or not, but it suggests to me that ng_iface should simply set the current VNET to that of the ifnet before calling netisr_dispatch().

[donner]

In D25788#571362, @markj wrote:

In fact, for ng_iface it is possible for the node and ifnet VNETs to become out of sync: if I create two ng_iface interfaces and pass one into a jail, the node's VNET is not updated but the ifnet's VNET is updated.

Sounds like a bug to me.

I'm not sure if this is intentional or not, but it suggests to me that ng_iface should simply set the current VNET to that of the ifnet before calling netisr_dispatch().

Yes, please.
Are you able to provide such a patch?

[markj]

In D25788#571387, @lutz_donnerhacke.de wrote:

In D25788#571362, @markj wrote:

In fact, for ng_iface it is possible for the node and ifnet VNETs to become out of sync: if I create two ng_iface interfaces and pass one into a jail, the node's VNET is not updated but the ifnet's VNET is updated.

Sounds like a bug to me.

Yeah, and I expect it affects ng_eiface and possibly ng_sppp, though I'm totally unfamiliar with the latter.

I'm not sure if this is intentional or not, but it suggests to me that ng_iface should simply set the current VNET to that of the ifnet before calling netisr_dispatch().

Yes, please.
Are you able to provide such a patch?

I will work on it, but probably not before next week. If you are interested in doing it please go ahead.

[markj]

Modify ng_iface(4) instead.

[donner]

Thanks.