Re: svn commit: r357614 - in head/sys: kern sys
On 2/6/20 4:45 AM, Pawel Biernacki wrote: > Author: kaktus > Date: Thu Feb 6 12:45:58 2020 > New Revision: 357614 > URL: https://svnweb.freebsd.org/changeset/base/357614 > > Log: > sysctl(9): add CTLFLAG_NEEDGIANT flag > > Add CTLFLAG_NEEDGIANT flag (modelled after D_NEEDGIANT) that will be used to > mark sysctls that still require locking Giant. > > Rewrite sysctl_handle_string() to use internal locking instead of locking > Giant. This broke CTLFLAG_RDTUN strings such as hw.cxgbe.config_file. This is supposed to be writable by setting the associated environment variable via loader.conf or kenv: >From sys/dev/cxgbe/t4_main.c: /* * Configuration file. All the _CF names here are special. */ #define DEFAULT_CF "default" #define BUILTIN_CF "built-in" #define FLASH_CF"flash" #define UWIRE_CF"uwire" #define FPGA_CF "fpga" static char t4_cfg_file[32] = DEFAULT_CF; SYSCTL_STRING(_hw_cxgbe, OID_AUTO, config_file, CTLFLAG_RDTUN, t4_cfg_file, sizeof(t4_cfg_file), "Firmware configuration file"); However, CTLFLAG_RDTUN does not include CTLFLAG_WR, so when the kernel attempts to "write" to this node (the "TUN" part) to apply the associated kenv variable when loading the kernel module, your changes here now treat it as a ro_string and set 'arg2' to the length of the actual string. In my case I was setting the value to a longer string that still fit in the allocated space, and the value now fails to set giving me the following error on the console: Setting sysctl hw.cxgbe.config_file failed: 22 You can reproduce by doing the following: # kenv hw.cxgbe.config_file="kern_tls" # kldload if_cxgbe That should give you the error, and then checking the hw.cxgbe.config_file sysctl afterwards will show "default" instead of "kern_tls". Note that you don't need any cxgbe hardware to reproduce this as a fix would result in the value of the sysctl after the kldload being "kern_tls". -- John Baldwin ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r357614 - in head/sys: kern sys
On Thu, Feb 06, 2020 at 05:41:52PM +0100, Hans Petter Selasky wrote: > On 2020-02-06 13:45, Pawel Biernacki wrote: > > +#ifdef notyet > > +#defineSYSCTL_ENFORCE_FLAGS(x) > > \ > > +_Static_assert(((CTLFLAG_MPSAFE ^ CTLFLAG_NEEDGIANT) & (x)), \ > > +"Has to be either CTLFLAG_MPSAFE or CTLFLAG_NEEDGIANT") > > +#else > > +#defineSYSCTL_ENFORCE_FLAGS(x) > > +#endif > > Like cem@ pointed out, either you expand the XOR via OR or you can also do > it like this: > > (((x) & CTLFLAG_MPSAFE) != 0) ^ (((x) & CTLFLAG_NEEDGIANT) != 0) The intent was to do exactly this, xor the bools. > > which avoids having to define another macro. > > --HPS ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r357614 - in head/sys: kern sys
Thanks, will be fixed before enabling it. > On 6 Feb 2020, at 17:41, Hans Petter Selasky wrote: > > On 2020-02-06 13:45, Pawel Biernacki wrote: >> +#ifdef notyet >> +#define SYSCTL_ENFORCE_FLAGS(x) >> \ >> +_Static_assert(((CTLFLAG_MPSAFE ^ CTLFLAG_NEEDGIANT) & (x)),\ >> +"Has to be either CTLFLAG_MPSAFE or CTLFLAG_NEEDGIANT") >> +#else >> +#define SYSCTL_ENFORCE_FLAGS(x) >> +#endif > > Like cem@ pointed out, either you expand the XOR via OR or you can also do it > like this: > > (((x) & CTLFLAG_MPSAFE) != 0) ^ (((x) & CTLFLAG_NEEDGIANT) != 0) > > which avoids having to define another macro. > > --HPS ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r357614 - in head/sys: kern sys
On 2020-02-06 13:45, Pawel Biernacki wrote: +#ifdef notyet +#defineSYSCTL_ENFORCE_FLAGS(x) \ +_Static_assert(((CTLFLAG_MPSAFE ^ CTLFLAG_NEEDGIANT) & (x)), \ +"Has to be either CTLFLAG_MPSAFE or CTLFLAG_NEEDGIANT") +#else +#defineSYSCTL_ENFORCE_FLAGS(x) +#endif Like cem@ pointed out, either you expand the XOR via OR or you can also do it like this: (((x) & CTLFLAG_MPSAFE) != 0) ^ (((x) & CTLFLAG_NEEDGIANT) != 0) which avoids having to define another macro. --HPS ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r357614 - in head/sys: kern sys
Hi Pawel,
I don't think the (notyet) static assertion is quite right.
On Thu, Feb 6, 2020 at 4:46 AM Pawel Biernacki wrote:
>
> Author: kaktus
> Date: Thu Feb 6 12:45:58 2020
> New Revision: 357614
> URL: https://svnweb.freebsd.org/changeset/base/357614
>
> Log:
> sysctl(9): add CTLFLAG_NEEDGIANT flag
> ...
> Modified: head/sys/sys/sysctl.h
> ==
> --- head/sys/sys/sysctl.h Thu Feb 6 10:11:41 2020(r357613)
> +++ head/sys/sys/sysctl.h Thu Feb 6 12:45:58 2020(r357614)
> @@ -105,6 +105,13 @@ struct ctlname {
> ...
> + * One, and only one of CTLFLAG_MPSAFE or CTLFLAG_NEEDGIANT is required
> + * for SYSCTL_PROC and SYSCTL_NODE.
> ...
> @@ -263,6 +270,14 @@ TAILQ_HEAD(sysctl_ctx_list, sysctl_ctx_entry);
> #define__DESCR(d) ""
> #endif
>
> +#ifdef notyet
> +#defineSYSCTL_ENFORCE_FLAGS(x)
> \
> +_Static_assert(((CTLFLAG_MPSAFE ^ CTLFLAG_NEEDGIANT) & (x)), \
> +"Has to be either CTLFLAG_MPSAFE or CTLFLAG_NEEDGIANT")
The current (notyet) assertion checks for one or both flags being set,
but you want to disallow both being set. The XOR operator here is
meaningless; it is the same as OR for different bit flags.
That would be something like:
#define _CTLFLAG_MUTUALLY_EXCLUSIVE(CTLFLAG_MPSAFE | CTLFLAG_NEEDGIANT);
#define SYSCTL_ENFORCE_FLAGS(x) do { \
_Static_assert(((x) & _CTLFLAG_MUTUALLY_EXCLUSIVE) != 0 && \
((x) & _CTLFLAG_MUTUALLY_EXCLUSIVE) !=
_CTLFLAG_MUTUALLY_EXCLUSIVE, \
"Must set exactly one of CTLFLAG_MPSAFE, CTLFLAG_NEEDGIANT"); \
} while (0)
Best,
Conrad
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
svn commit: r357614 - in head/sys: kern sys
Author: kaktus
Date: Thu Feb 6 12:45:58 2020
New Revision: 357614
URL: https://svnweb.freebsd.org/changeset/base/357614
Log:
sysctl(9): add CTLFLAG_NEEDGIANT flag
Add CTLFLAG_NEEDGIANT flag (modelled after D_NEEDGIANT) that will be used to
mark sysctls that still require locking Giant.
Rewrite sysctl_handle_string() to use internal locking instead of locking
Giant.
Mark SYSCTL_STRING, SYSCTL_OPAQUE and their variants as MPSAFE.
Add infrastructure support for enforcing proper use of CTLFLAG_NEEDGIANT
and CTLFLAG_MPSAFE flags with SYSCTL_PROC and SYSCTL_NODE, not enabled yet.
Reviewed by: kib (mentor)
Approved by: kib (mentor)
Differential Revision:https://reviews.freebsd.org/D23378
Modified:
head/sys/kern/kern_sysctl.c
head/sys/sys/sysctl.h
Modified: head/sys/kern/kern_sysctl.c
==
--- head/sys/kern/kern_sysctl.c Thu Feb 6 10:11:41 2020(r357613)
+++ head/sys/kern/kern_sysctl.c Thu Feb 6 12:45:58 2020(r357614)
@@ -96,9 +96,13 @@ static MALLOC_DEFINE(M_SYSCTLTMP, "sysctltmp", "sysctl
* The sysctlmemlock is used to limit the amount of user memory wired for
* sysctl requests. This is implemented by serializing any userland
* sysctl requests larger than a single page via an exclusive lock.
+ *
+ * The sysctlstringlock is used to protect concurrent access to writable
+ * string nodes in sysctl_handle_string().
*/
static struct rmlock sysctllock;
static struct sx __exclusive_cache_line sysctlmemlock;
+static struct sx sysctlstringlock;
#defineSYSCTL_WLOCK() rm_wlock(&sysctllock)
#defineSYSCTL_WUNLOCK()rm_wunlock(&sysctllock)
@@ -170,10 +174,16 @@ sysctl_root_handler_locked(struct sysctl_oid *oid, voi
else
SYSCTL_WUNLOCK();
- if (!(oid->oid_kind & CTLFLAG_MPSAFE))
+ /*
+* Treat set CTLFLAG_NEEDGIANT and unset CTLFLAG_MPSAFE flags the same,
+* untill we're ready to remove all traces of Giant from sysctl(9).
+*/
+ if ((oid->oid_kind & CTLFLAG_NEEDGIANT) ||
+ (!(oid->oid_kind & CTLFLAG_MPSAFE)))
mtx_lock(&Giant);
error = oid->oid_handler(oid, arg1, arg2, req);
- if (!(oid->oid_kind & CTLFLAG_MPSAFE))
+ if ((oid->oid_kind & CTLFLAG_NEEDGIANT) ||
+ (!(oid->oid_kind & CTLFLAG_MPSAFE)))
mtx_unlock(&Giant);
KFAIL_POINT_ERROR(_debug_fail_point, sysctl_running, error);
@@ -917,6 +927,7 @@ sysctl_register_all(void *arg)
struct sysctl_oid **oidp;
sx_init(&sysctlmemlock, "sysctl mem");
+ sx_init(&sysctlstringlock, "sysctl string handler");
SYSCTL_INIT();
SYSCTL_WLOCK();
SET_FOREACH(oidp, sysctl_set)
@@ -1632,48 +1643,69 @@ sysctl_handle_64(SYSCTL_HANDLER_ARGS)
int
sysctl_handle_string(SYSCTL_HANDLER_ARGS)
{
+ char *tmparg;
size_t outlen;
int error = 0, ro_string = 0;
/*
+* If the sysctl isn't writable, microoptimise and treat it as a
+* const string.
* A zero-length buffer indicates a fixed size read-only
* string. In ddb, don't worry about trying to make a malloced
* snapshot.
*/
- if (arg2 == 0 || kdb_active) {
+ if (!(oidp->oid_kind & CTLFLAG_WR) || arg2 == 0 || kdb_active) {
arg2 = strlen((char *)arg1) + 1;
ro_string = 1;
}
if (req->oldptr != NULL) {
- char *tmparg;
-
if (ro_string) {
tmparg = arg1;
+ outlen = strlen(tmparg) + 1;
} else {
- /* try to make a coherent snapshot of the string */
tmparg = malloc(arg2, M_SYSCTLTMP, M_WAITOK);
+ sx_slock(&sysctlstringlock);
memcpy(tmparg, arg1, arg2);
+ sx_sunlock(&sysctlstringlock);
+ outlen = strlen(tmparg) + 1;
}
- outlen = strnlen(tmparg, arg2 - 1) + 1;
error = SYSCTL_OUT(req, tmparg, outlen);
if (!ro_string)
free(tmparg, M_SYSCTLTMP);
} else {
- outlen = strnlen((char *)arg1, arg2 - 1) + 1;
+ if (!ro_string)
+ sx_slock(&sysctlstringlock);
+ outlen = strlen((char *)arg1) + 1;
+ if (!ro_string)
+ sx_sunlock(&sysctlstringlock);
error = SYSCTL_OUT(req, NULL, outlen);
}
if (error || !req->newptr)
return (error);
- if ((req->newlen - req->newidx) >= arg2) {
+ if (req->newlen - req->newidx >= arg2 ||
+ req->newlen - req->newidx <= 0) {
error = EINVAL;
} else {
- arg2 = (req->newlen - req->newidx);
- error = SY
