Commited d7cf2e714e29a52264fa4bc67e7d727948003afb

Rebase over ng_pppoe fix.

Was detected while running an experimental version of net/frr10 full regression test (topotest).

The NET_EPOCH_WAIT() is actually a crutch. I would agree with this change if we were very close to stable/16. For main, I'd like to make it right. The table shall be freed with deferred free. Do you have a good repro for this panic? I can create this change.

The change is good as is, of course.

There are many places where memcpy() can be used. I don't want to mix that into this commit.

I'd suggest to make the variable and the sysctl unsigned.

• Rebase over fixed bug in pfsync and over caddr_t cleanup D60738

mtodo() is not used widely, I don't yet want to proliferate its use. At least not with this change.

Use void * where char * isn't needed.

Note that this conflicts with bugfix from @rrs that we are discussing with @tuexen in email and this revision will be updated.

Change m_append, m_copyback, m_copydata prototypes to use void *.

Sorry, dropped unfinished diff. Will update.

Note: commit message singles out the datagram sockets, but the change applies to all sockets. How does it work on Linux? We should emulate their bug completely.

Thanks! LGTM Let's get ack from @kib and give some time to emulation people to review.

Thanks a lot for this project! It is definitely very valuable. Adding more reviewers.

In D60659#1391791, @kib wrote:

No, we prefer to have flags in sv_sysent to qualify modifications to the common syscall code paths, See the suggestion in D60701 and flags like SV_SIG_DISCIGN

Usually in the kernel when we do Linux compatibility checks we check SV_PROC_ABI(p) == SV_ABI_LINUX. The sockets may outlive the process, so for a worst case we would put the flag on the socket. However in a connect(2) syscall the process is always present. Other important moment is that sockets may also be shared between processes. Weird to share one between a Linux process and FreeBSD process, but possible.

No objection.

These were a legend already when I first touched FreeBSD :)

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.

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);
}

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

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?

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.

Here is another idea. I'm sorry that I didn't suggest that before and you already produced a version.

I don't like that we may return 0 from MOD_QUIESCE and then return EBUSY from MOD_UNLOAD. I would suggest to keep a global 'static bool carp_unloading` and set it at MOD_QUIESCE when we return 0. Then carp_nl_set shall check this boolean. It should be protected by the carp_sx and carp_modevent should also use this lock.

In D60602#1389594, @gallatin wrote:

My intention is to just get prefetching working for arm64 in iflib. His kern_prefetch uses a different level assembly instruction (nt0 in iflib, vs nt1 in rrs code).

This is correct fix, thanks! The commit message should be like this:

Can you please share the trace? I wonder how did we come with ng_ppp not working in the epoch in the first place.

Thank for the fix! It is good as is. But feel free to rethink the buffer allocation in general. Netlink initially used mbufs(9), and then I changed that. However, when doing that I didn't invest enough time into thinking on how to properly do that. There could be some bits of logic left that were relevant to mbufs. I mean that if this code was written from scratch and not as a refactor of the very first implementation of netlink it could have been different (better).

I wonder if you just use THREAD_CAN_SLEEP()?

Pretty sure you meant me, so reviewers updated.

I guess you definitely considering to put list of family specific softcs right on nlpcb. Why did you choose to have list of known socket softcs on a family instead?

Here is an idea: we can create an unloading mechanism that will queue unload requests to the same taskqueue thread that processes the netlink messages. This will guarantee there are no messages being processed. The actual unload syscall will sleep on the unload request, and the taskqueue will perform the unload, then wake up the syscall.

The problem is real and patch of course is correct. However, we are adding two atomics per-message to protect against a race that happens only at kldunload(8). This degrades performance for the sake of a problem that only developers and fuzzers hit.

Looks correct, but let's check with @rscheff. Thanks!

Okay, I'm not a test guy, but throwing in my green light to threaten the rest of the reviewers to actually review the test. So that Igor has more greens and the revision will look closer to being pushed.

In D60423#1386712, @igoro wrote:

Having that patch applied, the udp_bindings:connect_bound_socket_from_2address_jail test case could be wrapped into a jail with pre-configuration like this:

Paulo, do you have a public git repo (e.g. github) with your branch? I'd like to take your commit as is (with your name and your commit message) for pushing.

Please include in the commit log references to the original phk's commit hashes and maybe our discussion in D60281.

Wait... Maybe we should understand why userland_sysctl() does this and is it needed at all today? It originates from 7a69d9230f834f562d2c5c36c837e67eacdf5ab3. @phk maybe you can comment on this one?

So what happens here is that the KPI of TCP stack switching is being changed under pressure of one of its consumers that has a special handling of EAGAIN. And we are substituting this EAGAIN to factually incorrect EINVAL. No, the argument is valid, the connection exists it is just its state that doesn't allow the switch (supposedly temporarily).

No objection. When writing this test I relied on @igoro and other test team members to deliver a framework where you would describe a jail for a test to be delivered real soon.

The change is correct, but as Pouria noted all the stuff around here is fragile. There could be bugs fixing other bugs :) Let's proceed with this change and see if there is any fallout that needs to be dealt with.

Thanks!

I'm not a voting srcmgr member, so not putting my approval here.

There was a period of time when Mr. Macy was committing a lot of his draft ideas right into svn HEAD. Most of them were never finished or continued. This is one of them.

Yeah, that's true. I can also imagine a program that would modify a string as a member of struct jail before passing to jail(2). :(

Per discussion with Michael, will do that differently.