unix/dgram: dispose mbufs on uxdg_mb queue
AbandonedPublic
Actions

Authored by khng on Jun 1 2024, 6:36 PM.

Details

Reviewers

glebius
markj

Summary

Since the new implementation of unix/dgram is introduced, the old
one-size-fits-all solution is not longer in use. Do our own cleanup by
walking mbufs through m_nextpkt/m_stailqpkt (alias of m_nextpkt) so that
nothing is leaked if unp_disconnect/unp_dispose found buffers to be
freed.

Sponsored by: Juniper Networks, Inc.
MFC after: 3 days

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 57996
Build 54884: arc lint + arc unit

Event Timeline

khng created this revision.Jun 1 2024, 6:36 PM

Herald added subscribers: olce, imp. · View Herald TranscriptJun 1 2024, 6:36 PM

khng requested review of this revision.Jun 1 2024, 6:36 PM

Harbormaster completed remote builds in B57995: Diff 139310.Jun 1 2024, 6:36 PM

Oops.

Harbormaster completed remote builds in B57996: Diff 139311.Jun 1 2024, 6:44 PM

m_stailqpkt is an alias to m_nextpkt, so the code:

m0 = is_dgram ? STAILQ_NEXT(m0, m_stailqpkt) : m0->m_nextpkt;

doesn't change anything with is_dgram switching between true/false.

Do you really see a leak fixed with this patch? Is this the leak reported in bug 279467?

Just seen reply there from Dmitry. It is the same leak.

In D45443#1037070, @glebius wrote:
m_stailqpkt is an alias to m_nextpkt, so the code:
m0 = is_dgram ? STAILQ_NEXT(m0, m_stailqpkt) : m0->m_nextpkt;
doesn't change anything with is_dgram switching between true/false.

Do you really see a leak fixed with this patch? Is this the leak reported in bug 279467?

ok, then the change to unp_gc can be reverted, We just need the uxdg queue m_freem.

khng retitled this revision from unix/dgram: dispose mbufs through m_stailqpkt chain to unix/dgram: dispose mbufs on uxdg_mb queue.Jun 3 2024, 6:44 PM

khng edited the summary of this revision. (Show Details)

m_nextpkt and m_stailqnxt are the same.

Harbormaster completed remote builds in B58034: Diff 139380.Jun 3 2024, 6:45 PM

Maybe I'm wrong and some of my assumptions don't work. Please give me a bit more time to look into that. Thanks a lot for working on that!

Ah, I see the bug. It is just because m_freem() follows only m_next linkage and doesn't follow m_nextpkt linkage. I always assumed it does both.

markj added inline comments.Jun 3 2024, 7:41 PM

sys/kern/uipc_usrreq.c
3221	You don't need to remove it. The usual idioms are: while ((m = STAILQ_FIRST(...)) != NULL) { STAILQ_REMOVE(...); m_freem(m); } or STAILQ_FOREACH_SAFE(...) { m_freem(m); }
3307–3311	I think the first parameter needs to be removed?

In D45443#1037231, @glebius wrote:

Ah, I see the bug. It is just because m_freem() follows only m_next linkage and doesn't follow m_nextpkt linkage. I always assumed it does both.

Yep, the old cold did it with sbflush instead. So in this case I make the fix into implementation specific part than to let sbflush do the job.

markj@: code should be idiomatic

Harbormaster completed remote builds in B58042: Diff 139406.Jun 3 2024, 7:45 PM

khng marked an inline comment as done.Jun 3 2024, 7:45 PM

khng marked an inline comment as done.Jun 3 2024, 7:53 PM

https://reviews.freebsd.org/D45477
https://reviews.freebsd.org/D45478