Page MenuHomeFreeBSD

lagg(4): Correctly define some sysctl variables
ClosedPublic

Authored by zlei on Apr 8 2023, 4:21 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, May 2, 9:36 PM
Unknown Object (File)
Thu, May 2, 9:33 PM
Unknown Object (File)
Thu, May 2, 9:32 PM
Unknown Object (File)
Thu, May 2, 8:12 PM
Unknown Object (File)
Jan 14 2024, 6:57 AM
Unknown Object (File)
Dec 20 2023, 3:30 AM
Unknown Object (File)
Nov 8 2023, 7:16 PM
Unknown Object (File)
Nov 2 2023, 7:20 PM
Subscribers

Details

Summary

939a050ad96c virtualized lagg(4), but the corresponding sysctl of some
virtualized global variables are not marked with CTLFLAG_VNET. A try to
operate on those variables via sysctl will effectively go to the 'master'
copies and the virtualized ones are not read or set accordingly. As a
side effect, on updating the 'master' copy, the virtualized global
variables of newly created vnets will have correct values.

PR: 270705
Fixes: 939a050ad96c Virtualize lagg(4) cloner
MFC after: 3 days

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

zlei requested review of this revision.Apr 8 2023, 4:21 PM
zlei added a reviewer: network.
zlei added a reviewer: hrs.

but the corresponding sysctl of some vnet variables are not marked with CTLFLAG_VNET

From the logic of sys/kern/kern_sysctl.c:

static int
sysctl_root(SYSCTL_HANDLER_ARGS)
{
...
#ifdef VIMAGE
	if ((oid->oid_kind & CTLFLAG_VNET) && arg1 != NULL)
		arg1 = (void *)(curvnet->vnet_data_base + (uintptr_t)arg1);
#endif
	error = sysctl_root_handler_locked(oid, arg1, arg2, req, &tracker);
...
}

I think sysctl set net.link.lagg.default_use_flowid, net.link.lagg.default_use_numa or net.link.lagg.default_flowid_shift can cause memory corruption, but I am not quite sure.

Can someone comment on it ?

kp added a subscriber: kp.

I believe that this ends up pointing to the vnet0 (i.e. the base vnet), so it should mean that the sysctls always affect the host system, even when set from a jail.
So, it probably won't crash, but you are correct that it's wrong anyway, and this is the correct fix.

This revision is now accepted and ready to land.Apr 11 2023, 7:24 AM
In D39467#898434, @zlei wrote:

but the corresponding sysctl of some vnet variables are not marked with CTLFLAG_VNET

From the logic of sys/kern/kern_sysctl.c:

static int
sysctl_root(SYSCTL_HANDLER_ARGS)
{
...
#ifdef VIMAGE
	if ((oid->oid_kind & CTLFLAG_VNET) && arg1 != NULL)
		arg1 = (void *)(curvnet->vnet_data_base + (uintptr_t)arg1);
#endif
	error = sysctl_root_handler_locked(oid, arg1, arg2, req, &tracker);
...
}

I think sysctl set net.link.lagg.default_use_flowid, net.link.lagg.default_use_numa or net.link.lagg.default_flowid_shift can cause memory corruption, but I am not quite sure.

There's magic behind VNET. The variables defined with VNET_DEFINE is basically same with normal global variables, and they can still be referenced via &VNET_NAME(var).
On vnet allocation, they are copied to virtualized global variables and left untouched. They are MASTER copies.

If they are modified unexpectedly or intentionally, then the virtualized global variables in vnet are not affected since they're copies, but on subsequent vnet allocation, the new vnet will see these modification and have updated virtualized global variables.

I believe that this ends up pointing to the vnet0 (i.e. the base vnet), so it should mean that the sysctls always affect the host system, even when set from a jail.

The above comment by @kp is not accurate.
vnet0 has the same allocation logic with other vnets, its virtualized global variables are also copied from the MASTER.

So, it probably won't crash, but you are correct that it's wrong anyway, and this is the correct fix.

I can conclude that it won't crash, and there will be no memory corruptions.

Can someone comment on it ?