mbuf: simplify m_free

[gallatin]

Summary

The check for  (M_PKTHDR|M_NOFREE) was added by np@ in
"91d1b03d3617 Merge r254736 from user/np/cxl_tuning."
in order to support a clever rx mode optimization used
by cxgbe at the time.  That optimization was removed in
46e1e307edb6 several years ago.  This check is otherwise
unneeded and adds an extraneous test to every mbuf free,
so let's remove it.

[glebius]

Looks like pfil(9) in compatibility mode remains the only user of M_NOFREE. And this change hints that pfil_fake_mbuf() actually needs to call m_tag_delete_chain.

[np]

cxgbe(4) does not use M_NOFREE any more and it's okay to remove this.

[gallatin]

In D60686#1391250, @glebius wrote:

Looks like pfil(9) in compatibility mode remains the only user of M_NOFREE. And this change hints that pfil_fake_mbuf() actually needs to call m_tag_delete_chain.

I think the only way it can get a tag is via m_pkthdr_init() calling mac_mbuf_init(). But I don't think that mac tags can be used in the pfil fake mbuf path.
I was going to suggest just skipping the call to mac_mbuf_init() for M_NOFREE mbufs

Or are there other ways a tag can get attached there?

[gallatin]

In D60686#1391281, @gallatin wrote:

In D60686#1391250, @glebius wrote:

Looks like pfil(9) in compatibility mode remains the only user of M_NOFREE. And this change hints that pfil_fake_mbuf() actually needs to call m_tag_delete_chain.

I think the only way it can get a tag is via m_pkthdr_init() calling mac_mbuf_init(). But I don't think that mac tags can be used in the pfil fake mbuf path.
I was going to suggest just skipping the call to mac_mbuf_init() for M_NOFREE mbufs

Or are there other ways a tag can get attached there?

Or skipping the tag alloc.. probably cheaper since mac_labled is probably 0 most of the time:

@@ -175,6 +175,8 @@ mac_mbuf_init(struct mbuf *m, int flag)
        M_ASSERTPKTHDR(m);
 
        if (mac_labeled & MPC_OBJECT_MBUF) {
+               if ((m->m_flags & M_NOFREE) != 0)
+                       return (0);
                tag = m_tag_get(PACKET_TAG_MACLABEL, sizeof(struct label),
                    flag);
                if (tag == NULL)

[glebius]

In D60686#1391290, @gallatin wrote:

I think the only way it can get a tag is via m_pkthdr_init() calling mac_mbuf_init(). But I don't think that mac tags can be used in the pfil fake mbuf path.
I was going to suggest just skipping the call to mac_mbuf_init() for M_NOFREE mbufs

Or are there other ways a tag can get attached there?

Firewalls may attach tags. I think we found a bug: one can craft a ruleset with tags for pf(4) and then attach it to NIC level pfil. That would leak memory.

[gallatin]

In D60686#1391306, @glebius wrote:

In D60686#1391290, @gallatin wrote:

I think the only way it can get a tag is via m_pkthdr_init() calling mac_mbuf_init(). But I don't think that mac tags can be used in the pfil fake mbuf path.
I was going to suggest just skipping the call to mac_mbuf_init() for M_NOFREE mbufs

Or are there other ways a tag can get attached there?

Firewalls may attach tags. I think we found a bug: one can craft a ruleset with tags for pf(4) and then attach it to NIC level pfil. That would leak memory.

ick

[glebius]

Wait please... I think actually this revision would expose the bug I described above.

[glebius]

Okay, indeed this revision would open up the leak described. Normally m_tag_delete_chain is called from UMA destructor that is called via uma_zfree(zone_mbuf, m) and for M_NOFREE mbufs it is called here. AFAIR, those cxgbe's M_NOFREE mbufs were some kind of a pool. The pfil fake mbuf is on stack. So, if this change is combined with adding m_tag_delete_chain to the end of pfil_fake_mbuf, everything should be alright.

pfil_fake_mbuf(...) {
...
...
...
	if (!SLIST_EMPTY(&m.m_pkthdr.tags))
		m_tag_delete_chain(&m, NULL);
}

[gallatin]

In D60686#1391325, @glebius wrote:

Okay, indeed this revision would open up the leak described. Normally m_tag_delete_chain is called from UMA destructor that is called via uma_zfree(zone_mbuf, m) and for M_NOFREE mbufs it is called here. AFAIR, those cxgbe's M_NOFREE mbufs were some kind of a pool. The pfil fake mbuf is on stack. So, if this change is combined with adding m_tag_delete_chain to the end of pfil_fake_mbuf, everything should be alright.

pfil_fake_mbuf(...) {
...
...
...
	if (!SLIST_EMPTY(&m.m_pkthdr.tags))
		m_tag_delete_chain(&m, NULL);
}

This is a pre-existing bug, since m_free() was not called in pfil_fake_mbuf(). Eg, this tag leak is not introduced by this change.. its just that this change made us notice it, correct? Or did func() or one of its callees call m_free?

[glebius]

If a packet filter attached a tag to this mbuf and then it (or an other packet filter) called m_free(), then it would not leak before this change. With this change would leak. However, it a packet filter allocated a tag and then nobody called m_free() -- yes, this bug was pre-existing. Thanks for D60690.