Page MenuHomeFreeBSD

sysctl: Do not serialize requests when running as root
AcceptedPublic

Authored by markj on Fri, Nov 29, 5:11 PM.
Tags
None
Referenced Files
F104566667: D47842.diff
Sun, Dec 8, 12:18 AM
Unknown Object (File)
Fri, Dec 6, 7:09 AM
Unknown Object (File)
Fri, Dec 6, 1:27 AM
Unknown Object (File)
Thu, Dec 5, 8:27 PM
Unknown Object (File)
Thu, Dec 5, 3:46 PM
Unknown Object (File)
Tue, Dec 3, 2:54 PM
Unknown Object (File)
Sun, Dec 1, 10:06 AM
Unknown Object (File)
Sun, Dec 1, 10:02 AM
Subscribers

Details

Reviewers
kib
jhb
Summary

Bugs or unexpected behaviour can cause a user thread to block in a
sysctl handler for a long time. "procstat -kka" is the most useful tool
to see why this might happen, but it can block on sysctlmemlock too.

Since the purpose of this lock is merely to ensure userspace can't wire
too much memory, don't require it for requests from privileged threads.

PR: 282994

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 60878
Build 57762: arc lint + arc unit

Event Timeline

markj requested review of this revision.Fri, Nov 29, 5:11 PM

May be a per-uid sysctl memlock limit is better.
BTW, why don't we account sysctl wired memory into the user wire limit?

This revision is now accepted and ready to land.Fri, Nov 29, 5:32 PM
In D47842#1090650, @kib wrote:

May be a per-uid sysctl memlock limit is better.
BTW, why don't we account sysctl wired memory into the user wire limit?

It used to be that all user sysctl requests were serialized by a global lock, so it probably didn't matter very much since the wiring is always transient. Later, the sysctlmemlock was added, and we started serializing only those requests where oldsz > PAGE_SIZE. Perhaps we should modify vslock() to start counting these wirings, but I am not sure it is useful.

In D47842#1090650, @kib wrote:

May be a per-uid sysctl memlock limit is better.
BTW, why don't we account sysctl wired memory into the user wire limit?

It used to be that all user sysctl requests were serialized by a global lock, so it probably didn't matter very much since the wiring is always transient. Later, the sysctlmemlock was added, and we started serializing only those requests where oldsz > PAGE_SIZE. Perhaps we should modify vslock() to start counting these wirings, but I am not sure it is useful.

If we start counting, we can get rid of the global sysctl lock then?

The thing that makes this a bit odd in terms of the memory limit is that userspace isn't the one requesting to wire the memory. The kernel does by calling sysctl_wire_old_buffer in an individual handler. It would be nice to be smarter about this, e.g. don't acquire the lock until sysctl_wire_old_buffer is actually called? OTOH, one might ask why we really need the lock at all? I guess the worry is that userspace can DOS the kernel by making lots of requests with large buffers for nodes that are known to use sysctl_wire_old_buffer? One could implement a per-user limit of sorts by using a mtxpool-like thing but with sx locks and hash the uid to pick the specific sx lock to use if we really cared. Exempting root is probably sufficient in practice.