net/if: Fix ifa memory leak during vnet removal

[pouria]

Summary

Clean up memory allocations after removal of a vnet jail.

MFC after: 2 weeks

Without patch:
Before running sys/net kyua tests:

[root@ftsr1] [~] # vmstat -m | grep ifaddr
ifaddr   38 17392   38 16,32,128,256,384,1024,2048,4096

After running sys/net kyua tests:

[root@ftsr1] [~] # vmstat -m | grep ifaddr
ifaddr 1418 383152 19838 16,32,128,256,384,1024,2048,4096

With patch:
Before running sys/net kyua tests:

[root@ftsr1] [~] # vmstat -m | grep ifaddr
ifaddr   39 17776   39 16,32,128,256,384,1024,2048,4096

After running sys/net kyua tests:

[root@ftsr1] [~] # vmstat -m | grep ifaddr
ifaddr  140 55984 19010 16,32,128,256,384,1024,2048,4096

Test Plan

vmstat -m | grep ifaddr
jail -c name=leak persist
jail -r leak
vmstat -m | grep ifaddr

To increase leak size:

ifconfig -j leak lo create

[markj]

But where is the leak? Shouldn't each interface be responsible for removing its addresses when it's destroyed?

[pouria]

In D59387#1362564, @markj wrote:

But where is the leak? Shouldn't each interface be responsible for removing its addresses when it's destroyed?

Hi!, See the comments.

sys/net/if.c

877–880

4. We allocate an ifa for loopback interfaces here, but we don't free it after jail removal.

960

2.if we do 1, we can't call in6_purge_proxy_ndp, due to its LLTABLE lock.
even if we make it conditional by using !VNET_IS_SHUTTING_DOWN. Then (see third comment)

967

3. we skip AF_LINKs here anyway, and we can't include AF_LINK in this function. (too soon to free it when !shutdown)

1137

1. We can't call this function under if (shutdown), but even if we do it. (see second comment.)

[glebius]

You definitely found the problem, but the fix doesn't seem correct. There is asymmetry in allocation/freeing. It could close the leak at some configurations without regressions, of course. But IMHO not at all configurations.

First, the stack allocates single address, but frees the entire list. Can this be made symmetrical and only ifp->if_addr is freed by the stack?
Second, allocation happens always, but freeing is under #ifdef VIMAGE.

[glebius]

P.S. Can't resist to put my standard sidenote here. This all is so complicated due to if_vmove() :)

[pouria]

In D59387#1362617, @glebius wrote:

You definitely found the problem, but the fix doesn't seem correct. There is asymmetry in allocation/freeing. It could close the leak at some configurations without regressions, of course. But IMHO not at all configurations.

First, the stack allocates single address, but frees the entire list. Can this be made symmetrical and only ifp->if_addr is freed by the stack?
Second, allocation happens always, but freeing is under #ifdef VIMAGE.

IMHO, this is what we already do under the !vmove condition, and my change also looks similar to !vmove.
I understand the problem statement, but I think it needs more work than simply resolving the leak here, therefore I'm not sure this revision is the right place to fix the overall asymmetry issue. (see comment)

In D59387#1362621, @glebius wrote:

P.S. Can't resist to put my standard sidenote here. This all is so complicated due to if_vmove() :)

was waiting for this one :D

sys/net/if.c

1144–1162

Normally, without the shutdown condition, this code runs and removes any remaining ifa, especially AF_LINK, since if_purgeaddrs() above it frees the rest.
We already do that. I could create a separate dedicated function for both !vmove and shutdown condition, but I'd rather not, because the code under the shutdown doesn't actually require removing each ifa from ifp->if_addrhead, so it won't need its lock.

[glebius]

I vaguely remember that December 2025 when I was refactoring the attach/detach of domains/interfaces 0d469d23715d690b863787ebfa51529e1f6a9092, I found out that there are at least 2 places where we free addresses and multicast addresses. And apparently we mishandle that as there is a leak left. And I left it for later. Maybe good time for another take on that mess.

[zlei]

sys/net/if.c

1104

I do NOT like this special logic for VNET. To be precise that is the introduction of VNET_IS_SHUTTING_DOWN.

So why VNET_IS_SHUTTING_DOWN was introduced ? I think that is a kind of optimization. Typically there are two cases to detach an interface,

# ifconfig -j a_live_jail gif0 destroy

and

# jail -R jail_to_be_destroyed

For the second case, all loaned interfaces are returned to their home vnet jail, and all cloned interfaces are to be destroyed. So it might be an optimization to free all the addresses at once.

Let us re-consider that optimization. Is that really required ? That depends. I think typically on a production server a vnet jail lives for a long time, we do not create and destroy vnet jails frequently except for the testing purpose ( for example running FreeBSD test cases ).

So I'd prefer a consistent logic of the two cases, to make the maintenance easier.

What do you think ?

[pouria]

sys/net/if.c

1104

You and @glebius are much more expert than I in this area.
But, if this code only used for optimization, I think we should remove it. I prefer correctness over optimization.

IMHO, at the end we should re-implement this with Gleb's proposed dev_detach design.
But since that redesign is intended for FreeBSD 17, I prefer to fix this leak now.

[glebius]

I'm afraid this is not an optimization. I don't remember anybody working on performance of vnet attach & detach. We were only working on stability :) It could actually be that after several refactors there is a piece left that can be removed now. But I doubt it ever was an optimization.