Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 17 Feb 2021 17:55:41 GMT
From:      Alexander Motin <mav@FreeBSD.org>
To:        src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org
Subject:   git: eefc7f388ab6 - stable/12 - kenv: avoid sleepable alloc for integer tunables
Message-ID:  <202102171755.11HHtfGr044629@gitrepo.freebsd.org>

next in thread | raw e-mail | index | archive | help
The branch stable/12 has been updated by mav:

URL: https://cgit.FreeBSD.org/src/commit/?id=eefc7f388ab66a79f99a5f73236e9fbfc5312d4f

commit eefc7f388ab66a79f99a5f73236e9fbfc5312d4f
Author:     Jason A. Harmening <jah@FreeBSD.org>
AuthorDate: 2020-08-14 21:37:38 +0000
Commit:     Alexander Motin <mav@FreeBSD.org>
CommitDate: 2021-02-17 17:52:24 +0000

    kenv: avoid sleepable alloc for integer tunables
    
    Avoid performing a potentially-blocking malloc for kenv lookups that will only
    perform non-destructive integer conversions on the returned buffer. Instead,
    perform the strtoq() in-place with the kenv lock held.
    
    While here, factor the logic around kenv_lock acquire and release into
    kenv_acquire() and kenv_release(), and use these functions for some light
    cleanup. Collapse getenv_string_buffer() into kern_getenv(), as the former
    no longer has any other callers and the only additional task performed by
    the latter is a WITNESS check that hasn't been useful since r362231.
    
    PR:             248250
    Reported by:    gbe
    Reviewed by:    mjg
    Tested by:      gbe
    Differential Revision:  https://reviews.freebsd.org/D26010
---
 sys/kern/kern_environment.c | 124 ++++++++++++++++++++++++--------------------
 1 file changed, 67 insertions(+), 57 deletions(-)

diff --git a/sys/kern/kern_environment.c b/sys/kern/kern_environment.c
index f2d7dcb1de4d..761113f108ec 100644
--- a/sys/kern/kern_environment.c
+++ b/sys/kern/kern_environment.c
@@ -59,6 +59,9 @@ __FBSDID("$FreeBSD$");
 static char *_getenv_dynamic_locked(const char *name, int *idx);
 static char *_getenv_dynamic(const char *name, int *idx);
 
+static char *kenv_acquire(const char *name);
+static void kenv_release(const char *buf);
+
 static MALLOC_DEFINE(M_KENV, "kenv", "kernel environment");
 
 #define KENV_SIZE	512	/* Maximum number of environment strings */
@@ -88,8 +91,6 @@ bool	dynamic_kenv;
 #define KENV_CHECK	if (!dynamic_kenv) \
 			    panic("%s: called before SI_SUB_KMEM", __func__)
 
-static char	*getenv_string_buffer(const char *);
-
 int
 sys_kenv(td, uap)
 	struct thread *td;
@@ -482,16 +483,24 @@ _getenv_static(const char *name)
 char *
 kern_getenv(const char *name)
 {
-	char *ret;
+	char *cp, *ret;
+	int len;
 
 	if (dynamic_kenv) {
-		ret = getenv_string_buffer(name);
-		if (ret == NULL) {
-			WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, NULL,
-			    "getenv");
+		len = KENV_MNAMELEN + 1 + kenv_mvallen + 1;
+		ret = uma_zalloc(kenv_zone, M_WAITOK | M_ZERO);
+		mtx_lock(&kenv_lock);
+		cp = _getenv_dynamic(name, NULL);
+		if (cp != NULL)
+			strlcpy(ret, cp, len);
+		mtx_unlock(&kenv_lock);
+		if (cp == NULL) {
+			uma_zfree(kenv_zone, ret);
+			ret = NULL;
 		}
 	} else
 		ret = _getenv_static(name);
+
 	return (ret);
 }
 
@@ -503,12 +512,9 @@ testenv(const char *name)
 {
 	char *cp;
 
-	if (dynamic_kenv) {
-		mtx_lock(&kenv_lock);
-		cp = _getenv_dynamic(name, NULL);
-		mtx_unlock(&kenv_lock);
-	} else
-		cp = _getenv_static(name);
+	cp = kenv_acquire(name);
+	kenv_release(cp);
+
 	if (cp != NULL)
 		return (1);
 	return (0);
@@ -616,30 +622,33 @@ kern_unsetenv(const char *name)
 }
 
 /*
- * Return a buffer containing the string value from an environment variable
+ * Return the internal kenv buffer for the variable name, if it exists.
+ * If the dynamic kenv is initialized and the name is present, return
+ * with kenv_lock held.
  */
 static char *
-getenv_string_buffer(const char *name)
+kenv_acquire(const char *name)
 {
-	char *cp, *ret;
-	int len;
+	char *value;
 
 	if (dynamic_kenv) {
-		len = KENV_MNAMELEN + 1 + kenv_mvallen + 1;
-		ret = uma_zalloc(kenv_zone, M_WAITOK | M_ZERO);
 		mtx_lock(&kenv_lock);
-		cp = _getenv_dynamic(name, NULL);
-		if (cp != NULL)
-			strlcpy(ret, cp, len);
-		mtx_unlock(&kenv_lock);
-		if (cp == NULL) {
-			uma_zfree(kenv_zone, ret);
-			ret = NULL;
-		}
+		value = _getenv_dynamic(name, NULL);
+		if (value == NULL)
+			mtx_unlock(&kenv_lock);
+		return (value);
 	} else
-		ret = _getenv_static(name);
+		return (_getenv_static(name));
+}
 
-	return (ret);
+/*
+ * Undo a previous kenv_acquire() operation
+ */
+static void
+kenv_release(const char *buf)
+{
+	if ((buf != NULL) && dynamic_kenv)
+		mtx_unlock(&kenv_lock);
 }
 
 /*
@@ -650,17 +659,13 @@ getenv_string(const char *name, char *data, int size)
 {
 	char *cp;
 
-	if (dynamic_kenv) {
-		mtx_lock(&kenv_lock);
-		cp = _getenv_dynamic(name, NULL);
-		if (cp != NULL)
-			strlcpy(data, cp, size);
-		mtx_unlock(&kenv_lock);
-	} else {
-		cp = _getenv_static(name);
-		if (cp != NULL)
-			strlcpy(data, cp, size);
-	}
+	cp = kenv_acquire(name);
+
+	if (cp != NULL)
+		strlcpy(data, cp, size);
+
+	kenv_release(cp);
+
 	return (cp != NULL);
 }
 
@@ -674,16 +679,18 @@ getenv_array(const char *name, void *pdata, int size, int *psize,
 	uint8_t shift;
 	int64_t value;
 	int64_t old;
-	char *buf;
+	const char *buf;
 	char *end;
-	char *ptr;
+	const char *ptr;
 	int n;
 	int rc;
 
-	if ((buf = getenv_string_buffer(name)) == NULL)
-		return (0);
-
 	rc = 0;			  /* assume failure */
+
+	buf = kenv_acquire(name);
+	if (buf == NULL)
+		goto error;
+
 	/* get maximum number of elements */
 	size /= type_size;
 
@@ -798,8 +805,7 @@ getenv_array(const char *name, void *pdata, int size, int *psize,
 	if (n != 0)
 		rc = 1;	/* success */
 error:
-	if (dynamic_kenv)
-		uma_zfree(kenv_zone, buf);
+	kenv_release(buf);
 	return (rc);
 }
 
@@ -899,18 +905,21 @@ getenv_ulong(const char *name, unsigned long *data)
 int
 getenv_quad(const char *name, quad_t *data)
 {
-	char	*value, *vtp;
-	quad_t	iv;
+	const char	*value;
+	char		suffix, *vtp;
+	quad_t		iv;
 
-	value = getenv_string_buffer(name);
-	if (value == NULL)
-		return (0);
+	value = kenv_acquire(name);
+	if (value == NULL) {
+		goto error;
+	}
 	iv = strtoq(value, &vtp, 0);
 	if (vtp == value || (vtp[0] != '\0' && vtp[1] != '\0')) {
-		freeenv(value);
-		return (0);
+		goto error;
 	}
-	switch (vtp[0]) {
+	suffix = vtp[0];
+	kenv_release(value);
+	switch (suffix) {
 	case 't': case 'T':
 		iv *= 1024;
 		/* FALLTHROUGH */
@@ -925,12 +934,13 @@ getenv_quad(const char *name, quad_t *data)
 	case '\0':
 		break;
 	default:
-		freeenv(value);
 		return (0);
 	}
-	freeenv(value);
 	*data = iv;
 	return (1);
+error:
+	kenv_release(value);
+	return (0);
 }
 
 /*



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202102171755.11HHtfGr044629>