ng_one2many: Don't duplicate packets with m_dup() when receiving and re-transmitting

[nc]

Summary

ng_one2many: Don't duplicate packets with m_dup() when receiving and re-transmitting in a one-to-many setup, copy them instead with m_copypacket(). This should improve performance by reducing unnecessary copying.

Submitted by: Neel Chauhan <neel AT neelc DOT org>

[donner]

According to the man page "m_copypacket" makes a read-only version of the packet (by virtually setting some pointers to the same area of memory.
On contrary "m_dup" does copy also the content, so each version can be modified differently afterwards.

How do the code ensure, that the copied packet memory is not modified afterwards in the netgraph network?
Let me think about a one2many node connecting to different vlan nodes, which individually add different vlan headers to the packets. How will this work?

[nc]

You make a good point.

I decided to call m_dup() one time and then call m_copypacket() on the copy made my m_dup(), so if the original packet gets modified, the copies made by m_copypacket() aren't affected.

I'm not sure if the NG_FREE_M(mcpy) is the right thing to do, or if it will cause problems with the copy. I didn't want to cause a memory leak, but don't want to remove the data and cause a null dereference in the copies either.

[donner]

In D23721#521154, @neel_neelc.org wrote:

You make a good point.

I decided to call m_dup() one time and then call m_copypacket() on the copy made my m_dup(), so if the original packet gets modified, the copies made by m_copypacket() aren't affected.

I'm not sure if the NG_FREE_M(mcpy) is the right thing to do, or if it will cause problems with the copy. I didn't want to cause a memory leak, but don't want to remove the data and cause a null dereference in the copies either.

Let's take an example of a dynamic remote sniffing tool (used by many operators)

ether
    |
 lower
    |
    |
  one
    |
one2many 
    +-many0--lower- ether (in/out normal traffic)
    +-many1--upstream- vlan (tagged for remote monitoring by operator A)
    +-many2--upstream- vlan (tagged for remote monitoring by operator B)
    `-many3--upstream- vlan (tagged for remote monitoring by operator C)

In this case every packet replicated to manyX (X > 0) will be modified in a different way. The original packet needs to be transmitted unmodified to the original destination. For each replicated packet a new, complete copy is needed (aka m_dup).

[nc]

I'll give up on this.

[glebius]

This can make sense in certain setups. However, since originally node provided writable copies to each of "many" hooks, we can't change that. This can be configured as a node option, if sysadmin is sure that nodes downstream of "many" hooks are fine with read only mbufs.

[donner]

In D23721#526022, @glebius wrote:

This can make sense in certain setups. However, since originally node provided writable copies to each of "many" hooks, we can't change that. This can be configured as a node option, if sysadmin is sure that nodes downstream of "many" hooks are fine with read only mbufs.

You need a deep understanding of the mbuf framework and it's interaction with various network device drivers to use such a flag correctly. I believe, that it will create much more harm, than effect. The only reasonable solution is to rewrite the mbuf framework to intrinsic copy on write.