From: Etienne AUJAMES Date: Thu, 22 Aug 2024 15:46:19 +0000 (+0200) Subject: LU-18163 obdclass: fix sysfs_memparse()/string_to_size() X-Git-Tag: 2.16.51~203 X-Git-Url: https://git.whamcloud.com/gitweb?a=commitdiff_plain;h=10547cbf7ad11423729cc7a15ee1352bb4461814;p=fs%2Flustre-release.git LU-18163 obdclass: fix sysfs_memparse()/string_to_size() The patch reworks string_to_size() to avoid string copies and handle default unit directly in __string_to_size(). Also, this would handle more gracefully fractional value with huge unit (like 0.5EiB). This implementation fixes the parsing of the following invalid strings: - "10.badbadMib" - "10MiBbadbad" - "10MBAD" - "10.123badMib" - "1024.KG" The patch change the way to handle decimal fractional part: a maximum of 9 digits are supported. It fixes test_string_to_size_err() to actually return an error if a test failed and then prevents the module to load. It fixes obdclass_init() to avoid a crash if obd_init_checks() failed. Add regression tests in obd_init_checks. Signed-off-by: Etienne AUJAMES Change-Id: Ic20d11368fc7608637e8123d7c6c5a2ab2cf4a4b Reviewed-on: https://review.whamcloud.com/c/fs/lustre-release/+/56135 Tested-by: jenkins Tested-by: Maloo Reviewed-by: Andreas Dilger Reviewed-by: Neil Brown Reviewed-by: James Simmons Reviewed-by: Oleg Drokin --- diff --git a/lustre/obdclass/class_obd.c b/lustre/obdclass/class_obd.c index 5233b14..28cbfda 100644 --- a/lustre/obdclass/class_obd.c +++ b/lustre/obdclass/class_obd.c @@ -553,14 +553,18 @@ static struct miscdevice obd_psdev = { u64 __size; \ int __ret; \ \ - BUILD_BUG_ON(sizeof(value) >= 23); \ __ret = sysfs_memparse(value, sizeof(value) - 1, &__size, def_unit); \ - if (__ret != __rc) \ + if (__ret != __rc) { \ CERROR("string_helper: parsing '%s' expect rc %d != got %d\n", \ value, __rc, __ret); \ - else if (!__ret && (u64)expect != __size) \ + __ret = -EINVAL; \ + } else if (!__ret && (u64)expect != __size) { \ CERROR("string_helper: parsing '%s' expect %llu != got %llu\n",\ value, (u64)expect, __size); \ + __ret = -EINVAL; \ + } else { \ + __ret = 0; \ + } \ __ret; \ }) #define test_string_to_size_one(value, expect, def_unit) \ @@ -632,18 +636,48 @@ static int __init obd_init_checks(void) RETURN(ret); /* invalid string */ - if (!test_string_to_size_err("256B34", 256, "B", -EINVAL)) { + if (test_string_to_size_err("256B34", 256, "B", -EINVAL)) { CERROR("string_helpers: format should be number then units\n"); ret = -EINVAL; } - if (!test_string_to_size_err("132OpQ", 132, "B", -EINVAL)) { + if (test_string_to_size_err("132OpQ", 132, "B", -EINVAL)) { CERROR("string_helpers: invalid units should be rejected\n"); ret = -EINVAL; } - if (!test_string_to_size_err("1.82B", 1, "B", -EINVAL)) { + if (test_string_to_size_err("1.82B", 1, "B", -EINVAL)) { CERROR("string_helpers: 'B' with '.' should be invalid\n"); ret = -EINVAL; } + if (test_string_to_size_err("10.badMib", 1, "B", -EINVAL)) { + CERROR("string_helpers: '10.badMib' should be invalid\n"); + ret = -EINVAL; + } + if (test_string_to_size_err("10MiBbad", 1, "B", -EINVAL)) { + CERROR("string_helpers: '10MiBbad' should be invalid\n"); + ret = -EINVAL; + } + if (test_string_to_size_err("10MBAD", 1, "B", -EINVAL)) { + CERROR("string_helpers: '10MBAD' should be invalid\n"); + ret = -EINVAL; + } + if (test_string_to_size_err("10.123badMib", 1, "B", -EINVAL)) { + CERROR("string_helpers: '10.123badMib' should be invalid\n"); + ret = -EINVAL; + } + if (test_string_to_size_err("1024.KG", 1, "B", -EINVAL)) { + CERROR("string_helpers: '1024.KG' should be invalid\n"); + ret = -EINVAL; + } + if (test_string_to_size_err("102345678910234567891023456789.82M", + 1, "B", -EOVERFLOW)) { + CERROR("string_helpers: too long decimal string should be rejected\n"); + ret = -EINVAL; + } + if (test_string_to_size_err(".1023456789M", + 1, "B", -EOVERFLOW)) { + CERROR("string_helpers: too long string for the fractional part should be rejected\n"); + ret = -EINVAL; + } if (test_string_to_size_one("343\n", 343, "B")) { CERROR("string_helpers: should ignore newline\n"); ret = -EINVAL; @@ -653,42 +687,49 @@ static int __init obd_init_checks(void) /* memparse unit handling */ ret = 0; - ret += test_string_to_size_one("0B", 0, "B"); - ret += test_string_to_size_one("512B", 512, "B"); - ret += test_string_to_size_one("1.067kB", 1067, "B"); - ret += test_string_to_size_one("1.042KiB", 1067, "B"); - ret += test_string_to_size_one("8", 8388608, "M"); - ret += test_string_to_size_one("65536", 65536, "B"); - ret += test_string_to_size_one("128", 131072, "K"); - ret += test_string_to_size_one("1M", 1048576, "B"); - ret += test_string_to_size_one("0.5T", 549755813888ULL, "T"); - ret += test_string_to_size_one("256.5G", 275414777856ULL, "G"); + ret = ret ?: test_string_to_size_one("0B", 0, "B"); + ret = ret ?: test_string_to_size_one("512B", 512, "B"); + ret = ret ?: test_string_to_size_one("1.067kB", 1067, "B"); + ret = ret ?: test_string_to_size_one("1.042KiB", 1067, "B"); + ret = ret ?: test_string_to_size_one("8", 8388608, "M"); + ret = ret ?: test_string_to_size_one("65536", 65536, "B"); + ret = ret ?: test_string_to_size_one("128", 131072, "K"); + ret = ret ?: test_string_to_size_one("1M", 1048576, "B"); + ret = ret ?: test_string_to_size_one("0.5T", 549755813888ULL, "T"); + ret = ret ?: test_string_to_size_one("256.5G", 275414777856ULL, "G"); + ret = ret ?: test_string_to_size_one("0.0625M", 1ULL << 16, "M"); + ret = ret ?: test_string_to_size_one(".03125M", 1ULL << 15, "M"); + ret = ret ?: test_string_to_size_one(".015625M", 1ULL << 14, "M"); if (ret) RETURN(ret); /* string helper values */ - ret += test_string_to_size_one("16", 16777216, "MiB"); - ret += test_string_to_size_one("8.39MB", 8390000, "MiB"); - ret += test_string_to_size_one("8.00MiB", 8388608, "MiB"); - ret += test_string_to_size_one("256GB", 256000000000ULL, "GiB"); - ret += test_string_to_size_one("238.731GiB", 256335459385ULL, "GiB"); + ret = ret ?: test_string_to_size_one("16", 16777216, "MiB"); + ret = ret ?: test_string_to_size_one("8.39MB", 8390000, "MiB"); + ret = ret ?: test_string_to_size_one("8.00MiB", 8388608, "MiB"); + ret = ret ?: test_string_to_size_one("256GB", 256000000000ULL, "GiB"); + ret = ret ?: test_string_to_size_one("238.731GiB", 256335459385ULL, + "GiB"); if (ret) RETURN(ret); /* huge values */ - ret += test_string_to_size_one("0.4TB", 400000000000ULL, "TiB"); - ret += test_string_to_size_one("12.5TiB", 13743895347200ULL, "TiB"); - ret += test_string_to_size_one("2PB", 2000000000000000ULL, "PiB"); - ret += test_string_to_size_one("16PiB", 18014398509481984ULL, "PiB"); + ret = ret ?: test_string_to_size_one("0.4TB", 400000000000ULL, "TiB"); + ret = ret ?: test_string_to_size_one("12.5TiB", 13743895347200ULL, + "TiB"); + ret = ret ?: test_string_to_size_one("2PB", 2000000000000000ULL, "PiB"); + ret = ret ?: test_string_to_size_one("16PiB", 18014398509481984ULL, + "PiB"); + ret = ret ?: test_string_to_size_one("0.5EiB", 1ULL << 59, "EiB"); if (ret) RETURN(ret); /* huge values should overflow */ - if (!test_string_to_size_err("1000EiB", 0, "EiB", -EOVERFLOW)) { + if (test_string_to_size_err("1000EiB", 0, "EiB", -EOVERFLOW)) { CERROR("string_helpers: failed to detect binary overflow\n"); ret = -EINVAL; } - if (!test_string_to_size_err("1000EB", 0, "EiB", -EOVERFLOW)) { + if (test_string_to_size_err("1000EB", 0, "EiB", -EOVERFLOW)) { CERROR("string_helpers: failed to detect decimal overflow\n"); ret = -EINVAL; } @@ -702,8 +743,6 @@ static int __init obdclass_init(void) LCONSOLE_INFO("Lustre: Build Version: "LUSTRE_VERSION_STRING"\n"); - register_oom_notifier(&obdclass_oom); - err = libcfs_setup(); if (err) return err; @@ -719,6 +758,8 @@ static int __init obdclass_init(void) return err; } + register_oom_notifier(&obdclass_oom); + err = libcfs_kkuc_init(); if (err) goto cleanup_obd_memory; diff --git a/lustre/obdclass/lprocfs_status.c b/lustre/obdclass/lprocfs_status.c index aa19c1e..a11e826 100644 --- a/lustre/obdclass/lprocfs_status.c +++ b/lustre/obdclass/lprocfs_status.c @@ -1740,136 +1740,251 @@ __s64 lprocfs_read_helper(struct lprocfs_counter *lc, } EXPORT_SYMBOL(lprocfs_read_helper); -/** - * string_to_size - convert ASCII string representing a numerical - * value with optional units to 64-bit binary value - * - * @size: The numerical value extract out of @buffer - * @buffer: passed in string to parse - * @count: length of the @buffer - * - * This function returns a 64-bit binary value if @buffer contains a valid - * numerical string. The string is parsed to 3 significant figures after - * the decimal point. Support the string containing an optional units at - * the end which can be base 2 or base 10 in value. If no units are given - * the string is assumed to just a numerical value. - * - * Returns: @count if the string is successfully parsed, - * -errno on invalid input strings. Error values: +/* + * Parse a decimal string and decompose it into integer and fractional values. + * The fractionnal part is returned with @frac_d and @frac_div the 10^x + * denominator. The maximum number of digits for the fractional part is 9. * - * - ``-EINVAL``: @buffer is not a proper numerical string - * - ``-EOVERFLOW``: results does not fit into 64 bits. - * - ``-E2BIG ``: @buffer is too large (not a valid number) + * examples of valid inputs: + * - ".01" -> int_d: 0, frac_d: 1, frac_div: 100 + * - "5" -> int_d: 5, frac_d: 0, frac_div: 1 + * - "2.1255" -> int_d: 2, frac_d: 1255, frac_div: 10000 + * - "2.0295" -> int_d: 2, frac_d: 295, frac_div: 10000 + * - "2.99999" -> int_d: 3, frac_d: 99999, frac_div: 100000 */ -int string_to_size(u64 *size, const char *buffer, size_t count) +static int string_to_decimal(u64 *int_d, u64 *frac_d, u32 *frac_div, + const char *buffer, size_t count) +{ + const char *str = buffer; + int len = 0, frac_len = 0; + int i; + int rc; + + *int_d = 0; + *frac_d = 0; + *frac_div = 1; + + if (!count) + return -EINVAL; + + /* parse integer */ + if (*str != '.') { + rc = sscanf(str, "%llu%n", int_d, &len); + if (rc < 0) + return rc; + if (rc < 1 || !len || len > count) + return -EINVAL; + str += len; + } + + /* parse fractional */ + if (*str != '.') + return len ? len : -EINVAL; + + str++; + len++; + rc = sscanf(str, "%llu%n", frac_d, &frac_len); + if (rc < 0) + return rc; + if (rc < 1 || !frac_len) + return (len == 1) ? -EINVAL : len; + + len += frac_len; + if (len > count) + return -EINVAL; + + /* if frac_len >= 10, the frac_div will overflow */ + if (frac_len >= 10) + return -EOVERFLOW; + + for (i = 0; i < frac_len; i++) + *frac_div *= 10; + + return len; +} + +static int string_to_blksize(u64 *blk_size, const char *buffer, size_t count) { /* For string_get_size() it can support values above exabytes, * (ZiB, YiB) due to breaking the return value into a size and * bulk size to avoid 64 bit overflow. We don't break the size * up into block size units so we don't support ZiB or YiB. */ - static const char *const units_10[] = { - "kB", "MB", "GB", "TB", "PB", "EB", - }; + enum string_size_units { + STRING_UNITS_2 = 0, + STRING_UNITS_10, + } unit = STRING_UNITS_2; static const char *const units_2[] = { "K", "M", "G", "T", "P", "E", }; + static const char *const units_10[] = { + "kB", "MB", "GB", "TB", "PB", "EB", + }; static const char *const *const units_str[] = { [STRING_UNITS_2] = units_2, [STRING_UNITS_10] = units_10, }; static const unsigned int coeff[] = { - [STRING_UNITS_10] = 1000, [STRING_UNITS_2] = 1024, + [STRING_UNITS_10] = 1000, }; - enum string_size_units unit = STRING_UNITS_2; - u64 whole, blk_size = 1; - char kernbuf[22], *end; - size_t len = count; - int rc; + size_t len = 0; int i; - if (count >= sizeof(kernbuf)) { - CERROR("count %zd > buffer %zd\n", count, sizeof(kernbuf)); - return -E2BIG; + *blk_size = 1; + if (!count || !*buffer) + return -EINVAL; + + if (*buffer == 'B') { + len = 1; + goto check_end; } - *size = 0; - /* The "iB" suffix is optionally allowed for indicating base-2 numbers. - * If suffix is only "B" and not "iB" then we treat it as base-10. - */ - end = strstr(buffer, "B"); - if (end && *(end - 1) != 'i') + if (count >= 2 && buffer[1] == 'B') unit = STRING_UNITS_10; i = unit == STRING_UNITS_2 ? ARRAY_SIZE(units_2) - 1 : ARRAY_SIZE(units_10) - 1; do { - end = strnstr(buffer, units_str[unit][i], count); - if (end) { + size_t unit_len = min(count, strlen(units_str[unit][i])); + + if (strncmp(buffer, units_str[unit][i], unit_len) == 0) { + len += unit_len; for (; i >= 0; i--) - blk_size *= coeff[unit]; - len = end - buffer; + *blk_size *= coeff[unit]; break; } } while (i--); - /* as 'B' is a substring of all units, we need to handle it - * separately. - */ - if (!end) { - /* 'B' is only acceptable letter at this point */ - end = strnchr(buffer, count, 'B'); - if (end) { - len = end - buffer; - - if (count - len > 2 || - (count - len == 2 && strcmp(end, "B\n") != 0)) { - CDEBUG(D_INFO, "unknown suffix '%s'\n", buffer); - return -EINVAL; - } - } - /* kstrtoull will error out if it has non digits */ - goto numbers_only; + if (*blk_size == 1) { + CDEBUG(D_INFO, "unknown suffix '%s'\n", buffer); + return -EINVAL; } - end = strnchr(buffer, count, '.'); - if (end) { - /* need to limit 3 decimal places */ - char rem[4] = "000"; - u64 frac = 0; - size_t off; + /* handle the optional "iB" suffix */ + if (unit == STRING_UNITS_2 && (count - len) >= 2 && + buffer[len] == 'i' && buffer[len + 1] == 'B') + len += 2; - len = end - buffer; - end++; +check_end: + if (count > len && isalnum(buffer[len])) + return -EINVAL; - /* limit to 3 decimal points */ - off = min_t(size_t, 3, strspn(end, "0123456789")); - /* need to limit frac_d to a u32 */ - memcpy(rem, end, off); - rc = kstrtoull(rem, 10, &frac); - if (rc) - return rc; + return len; +} + +/* + * This comes from scale64_check_overflow() (time/timekeeping.c). + * This is used to prevent u64 overflow for: + * *base = mutl * *base / div + */ +static int scale64_rem(u64 mult, u32 div, u64 *base, u32 *remp) +{ + u64 tmp = *base; + u64 quot; + u32 rem, rem2; - if (fls64(frac) + fls64(blk_size) - 1 > 64) - return -EOVERFLOW; + if (!tmp) + return 0; + if (mult > tmp) + swap(mult, tmp); + + quot = div_u64_rem(tmp, div, &rem); + + if (mult > div && + (fls64(mult) + fls64(quot) >= 8 * sizeof(u64) || + fls64(mult) + fls(rem) >= 8 * sizeof(u64))) + return -EOVERFLOW; + quot *= mult; - frac *= blk_size; - do_div(frac, 1000); - *size += frac; + tmp = div_u64_rem(rem * mult, div, &rem2); + *base = quot + tmp; + if (remp) + *remp = rem2; + + return 0; +} + +static int __string_to_size(u64 *size, const char *buffer, size_t count, + const char *defunit) +{ + u64 whole, frac, blk_size; + u32 frac_div; + const char *ptr; + size_t len, unit_len; + int rc; + + *size = 0; + + rc = string_to_decimal(&whole, &frac, &frac_div, buffer, count); + if (rc < 0) + return rc; + + len = rc; + ptr = buffer + len; + if (len >= count || !*ptr || isspace(*ptr)) { + *size = whole; + if (!defunit) + return len; + + ptr = defunit; + unit_len = strlen(defunit); + } else { + unit_len = count - len; } -numbers_only: - snprintf(kernbuf, sizeof(kernbuf), "%.*s", (int)len, buffer); - rc = kstrtoull(kernbuf, 10, &whole); - if (rc) + + rc = string_to_blksize(&blk_size, ptr, unit_len); + if (rc < 0) return rc; - if (whole != 0 && fls64(whole) + fls64(blk_size) - 1 > 64) + if (ptr != defunit) + len += rc; + + if (blk_size == 1 && frac) + return -EINVAL; + + if (blk_size == 1) { + *size = whole; + return len; + } + + if (fls64(whole) + fls64(blk_size) >= sizeof(u64) * 8) return -EOVERFLOW; - *size += whole * blk_size; + whole *= blk_size; + rc = scale64_rem(blk_size, frac_div, &frac, NULL); + if (rc) + return rc; - return count; + *size = whole + frac; + + return len; +} + +/** + * string_to_size - convert ASCII string representing a numerical + * value with optional units to 64-bit binary value + * + * @size: The numerical value extract out of @buffer + * @buffer: passed in string to parse + * @count: length of the @buffer + * + * This function returns a 64-bit binary value if @buffer contains a valid + * numerical string. The string is parsed to 3 significant figures after + * the decimal point. Support the string containing an optional units at + * the end which can be base 2 or base 10 in value. If no units are given + * the string is assumed to just a numerical value. + * + * Returns: length of characters parsed, + * -errno on invalid input strings. Error values: + * + * - ``-EINVAL``: @buffer is not a proper numerical string + * - ``-EOVERFLOW``: results does not fit into 64 bits. + * - ``-E2BIG ``: @buffer is too large (not a valid number) + */ +int string_to_size(u64 *size, const char *buffer, size_t count) +{ + return __string_to_size(size, buffer, count, NULL); } EXPORT_SYMBOL(string_to_size); @@ -1884,11 +1999,11 @@ EXPORT_SYMBOL(string_to_size); * * Parses a string into a number. The number stored at @buffer is * potentially suffixed with K, M, G, T, P, E. Besides these other - * valid suffix units are shown in the string_to_size() function. + * valid suffix units are shown in the __string_to_size() function. * If the string lacks a suffix then the defunit is used. The defunit * should be given as a binary unit (e.g. MiB) as that is the standard - * for tunables in Lustre. If no unit suffix is given (e.g. 'G'), then - * it is assumed to be in binary units. + * for tunables in Lustre. If no unit suffix is given (e.g. only "G" + * instead of "GB"), then it is assumed to be in binary units ("GiB"). * * Returns: 0 on success or -errno on failure. */ @@ -1896,34 +2011,13 @@ int sysfs_memparse(const char *buffer, size_t count, u64 *val, const char *defunit) { const char *param = buffer; - char tmp_buf[23]; int rc; - if (count > strlen(buffer)) - count = strlen(buffer); - - while (count > 0 && isspace(buffer[count - 1])) - count--; - + count = strnlen(buffer, count); if (!count) RETURN(-EINVAL); - /* If there isn't already a unit on this value, append @defunit. - * Units of 'B' don't affect the value, so don't bother adding. - */ - if (!isalpha(buffer[count - 1]) && defunit[0] != 'B') { - if (count + 3 >= sizeof(tmp_buf)) { - CERROR("count %zd > size %zd\n", count, sizeof(param)); - RETURN(-E2BIG); - } - - scnprintf(tmp_buf, sizeof(tmp_buf), "%.*s%s", (int)count, - buffer, defunit); - param = tmp_buf; - count = strlen(param); - } - - rc = string_to_size(val, param, count); + rc = __string_to_size(val, param, count, defunit); return rc < 0 ? rc : 0; }