[2/7] tunables: Add support for tunables of uint64_t type

  Recognize the uint64_t type in addition to the current int32_t and
size_t.  This allows addition of tunables of uint64_t types.  In
addition to adding the uint64_t type, this patch also consolidates
validation and reading of integer types in tunables.

One notable change is that of overflow computation in
tunables_strtoul.  The function was lifted from __internal_strtoul,
but it does not need the boundary condition check (i.e. result ==
ULONG_MAX) since it does not need to set errno.  As a result the check
can be simplified, which I have now done.

	* elf/dl-tunable-types.h (tunable_type_code_t): New type
	TUNABLE_TYPE_UINT_64.
	* elf/dl-tunables.c (tunables_strtoul): Return uint64_t.
	Simplify computation of overflow.
	(tunable_set_val_if_valid_range_signed,
	tunable_set_val_if_valid_range_unsigned): Remove and replace
	with this...
	(TUNABLE_SET_VAL_IF_VALID_RANGE): ... New macro.
	(tunable_initialize): Adjust.  Add uint64_t support.
	(__tunable_set_val): Add uint64_t support.
---
 elf/dl-tunable-types.h |  1 +
 elf/dl-tunables.c      | 94 +++++++++++++++++++++-----------------------------
 2 files changed, 41 insertions(+), 54 deletions(-)
  

On 11/05/2017 11:51, Siddhesh Poyarekar wrote:
> Recognize the uint64_t type in addition to the current int32_t and
> size_t.  This allows addition of tunables of uint64_t types.  In
> addition to adding the uint64_t type, this patch also consolidates
> validation and reading of integer types in tunables.
> 
> One notable change is that of overflow computation in
> tunables_strtoul.  The function was lifted from __internal_strtoul,
> but it does not need the boundary condition check (i.e. result ==
> ULONG_MAX) since it does not need to set errno.  As a result the check
> can be simplified, which I have now done.
> 
> 	* elf/dl-tunable-types.h (tunable_type_code_t): New type
> 	TUNABLE_TYPE_UINT_64.
> 	* elf/dl-tunables.c (tunables_strtoul): Return uint64_t.
> 	Simplify computation of overflow.
> 	(tunable_set_val_if_valid_range_signed,
> 	tunable_set_val_if_valid_range_unsigned): Remove and replace
> 	with this...
> 	(TUNABLE_SET_VAL_IF_VALID_RANGE): ... New macro.
> 	(tunable_initialize): Adjust.  Add uint64_t support.
> 	(__tunable_set_val): Add uint64_t support.

LGTM with some remarks.

> ---
>  elf/dl-tunable-types.h |  1 +
>  elf/dl-tunables.c      | 94 +++++++++++++++++++++-----------------------------
>  2 files changed, 41 insertions(+), 54 deletions(-)
> 
> diff --git a/elf/dl-tunable-types.h b/elf/dl-tunable-types.h
> index 37a4e80..1d516df 100644
> --- a/elf/dl-tunable-types.h
> +++ b/elf/dl-tunable-types.h
> @@ -24,6 +24,7 @@
>  typedef enum
>  {
>    TUNABLE_TYPE_INT_32,
> +  TUNABLE_TYPE_UINT_64,
>    TUNABLE_TYPE_SIZE_T,
>    TUNABLE_TYPE_STRING

As for previous patch we should update README.tunables with this new allowed
type.  Also, I think we should add that both hexadecimal and octal are also
supported.

>  } tunable_type_code_t;
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index ebf48bb..8d72e26 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -105,10 +105,10 @@ get_next_env (char **envp, char **name, size_t *namelen, char **val,
>  /* A stripped down strtoul-like implementation for very early use.  It does not
>     set errno if the result is outside bounds because it gets called before
>     errno may have been set up.  */
> -static unsigned long int
> +static uint64_t
>  tunables_strtoul (const char *nptr)
>  {
> -  unsigned long int result = 0;
> +  uint64_t result = 0;
>    long int sign = 1;
>    unsigned max_digit;
>  
> @@ -144,7 +144,7 @@ tunables_strtoul (const char *nptr)
>  
>    while (1)
>      {
> -      unsigned long int digval;
> +      int digval;
>        if (*nptr >= '0' && *nptr <= '0' + max_digit)
>          digval = *nptr - '0';
>        else if (base == 16)
> @@ -159,9 +159,8 @@ tunables_strtoul (const char *nptr)
>        else
>          break;
>  
> -      if (result > ULONG_MAX / base
> -	  || (result == ULONG_MAX / base && digval > ULONG_MAX % base))
> -	return ULONG_MAX;
> +      if (result >= (UINT64_MAX - digval) / base)
> +	return UINT64_MAX;
>        result *= base;
>        result += digval;

I think this does not really handle overflows correctly and I would suggest
to actually use the new check_mul_overflow_size_t macro from reallocarray
patch to actually check it and result UINT64_MAX for the case.

Also, do we have any generic value range input test for tunable interface
(to check for validation, overflow, underflow, etc.)?


>        ++nptr;
> @@ -170,68 +169,50 @@ tunables_strtoul (const char *nptr)
>    return result * sign;
>  }
>  
> -/* Initialize the internal type if the value validates either using the
> -   explicit constraints of the tunable or with the implicit constraints of its
> -   type.  */
> -static void
> -tunable_set_val_if_valid_range_signed (tunable_t *cur, const char *strval,
> -				int64_t default_min, int64_t default_max)
> -{
> -  int64_t val = (int64_t) tunables_strtoul (strval);
> -
> -  int64_t min = cur->type.min;
> -  int64_t max = cur->type.max;
> -
> -  if (min == max)
> -    {
> -      min = default_min;
> -      max = default_max;
> -    }
> -
> -  if (val >= min && val <= max)
> -    {
> -      cur->val.numval = val;
> -      cur->strval = strval;
> -    }
> -}
> -
> -static void
> -tunable_set_val_if_valid_range_unsigned (tunable_t *cur, const char *strval,
> -					 uint64_t default_min, uint64_t default_max)
> -{
> -  uint64_t val = (uint64_t) tunables_strtoul (strval);
> -
> -  uint64_t min = cur->type.min;
> -  uint64_t max = cur->type.max;
> -
> -  if (min == max)
> -    {
> -      min = default_min;
> -      max = default_max;
> -    }
> -
> -  if (val >= min && val <= max)
> -    {
> -      cur->val.numval = val;
> -      cur->strval = strval;
> -    }
> -}
> +#define TUNABLE_SET_VAL_IF_VALID_RANGE(__cur, __val, __type, __default_min, \
> +				       __default_max)			      \
> +({									      \
> +  __type min = (__cur)->type.min;					      \
> +  __type max = (__cur)->type.max;					      \
> +									      \
> +  if (min == max)							      \
> +    {									      \
> +      min = __default_min;						      \
> +      max = __default_max;						      \
> +    }									      \
> +									      \
> +  if ((__type) (__val) >= min && (__type) (val) <= max)			      \
> +    {									      \
> +      (__cur)->val.numval = val;					      \
> +      (__cur)->strval = strval;						      \
> +    }									      \
> +})
>  
>  /* Validate range of the input value and initialize the tunable CUR if it looks
>     good.  */
>  static void
>  tunable_initialize (tunable_t *cur, const char *strval)
>  {
> +  uint64_t val;
> +
> +  if (cur->type.type_code != TUNABLE_TYPE_STRING)
> +    val = tunables_strtoul (strval);
> +
>    switch (cur->type.type_code)
>      {
>      case TUNABLE_TYPE_INT_32:
>  	{
> -	  tunable_set_val_if_valid_range_signed (cur, strval, INT32_MIN, INT32_MAX);
> +	  TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, int64_t, INT32_MIN, INT32_MAX);
> +	  break;
> +	}
> +    case TUNABLE_TYPE_UINT_64:
> +	{
> +	  TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, uint64_t, 0, UINT64_MAX);
>  	  break;
>  	}
>      case TUNABLE_TYPE_SIZE_T:
>  	{
> -	  tunable_set_val_if_valid_range_unsigned (cur, strval, 0, SIZE_MAX);
> +	  TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, uint64_t, 0, SIZE_MAX);
>  	  break;
>  	}
>      case TUNABLE_TYPE_STRING:
> @@ -461,6 +442,11 @@ __tunable_set_val (tunable_id_t id, void *valp, tunable_callback_t callback)
>  
>    switch (cur->type.type_code)
>      {
> +    case TUNABLE_TYPE_UINT_64:
> +	{
> +	  *((uint64_t *) valp) = (uint64_t) cur->val.numval;
> +	  break;
> +	}
>      case TUNABLE_TYPE_INT_32:
>  	{
>  	  *((int32_t *) valp) = (int32_t) cur->val.numval;
>
  

On Tuesday 16 May 2017 03:39 AM, Adhemerval Zanella wrote:
> As for previous patch we should update README.tunables with this new allowed
> type.  Also, I think we should add that both hexadecimal and octal are also
> supported.

I've added this.

> I think this does not really handle overflows correctly and I would suggest
> to actually use the new check_mul_overflow_size_t macro from reallocarray
> patch to actually check it and result UINT64_MAX for the case.

I don't understand, can you please elaborate?  I am specifically trying
to ensure that the computation does not overflow at all and saturating
the result at UINT64_MAX if the result is too large.  The subsequent
TUNABLE_SET_VAL_IF_VALID_RANGE check should then do a range check before
setting the tunable value and refuse to set it if it is beyond the
bounds of the tunable type.

> Also, do we have any generic value range input test for tunable interface
> (to check for validation, overflow, underflow, etc.)?

TUNABLE_SET_VAL_IF_VALID_RANGE does the range check and avoids setting
the tunable if the value is not within the range of its type or the set
range, whichever is smaller.

Siddhesh
  

On 17/05/2017 04:07, Siddhesh Poyarekar wrote:
> On Tuesday 16 May 2017 03:39 AM, Adhemerval Zanella wrote:
>> As for previous patch we should update README.tunables with this new allowed
>> type.  Also, I think we should add that both hexadecimal and octal are also
>> supported.
> 
> I've added this.
> 
>> I think this does not really handle overflows correctly and I would suggest
>> to actually use the new check_mul_overflow_size_t macro from reallocarray
>> patch to actually check it and result UINT64_MAX for the case.
> 
> I don't understand, can you please elaborate?  I am specifically trying
> to ensure that the computation does not overflow at all and saturating
> the result at UINT64_MAX if the result is too large.  The subsequent
> TUNABLE_SET_VAL_IF_VALID_RANGE check should then do a range check before
> setting the tunable value and refuse to set it if it is beyond the
> bounds of the tunable type.
> 

My mistake here, I noticed after fire up the email that it is indeed
checking correctly for overflow.

>> Also, do we have any generic value range input test for tunable interface
>> (to check for validation, overflow, underflow, etc.)?
> 
> TUNABLE_SET_VAL_IF_VALID_RANGE does the range check and avoids setting
> the tunable if the value is not within the range of its type or the set
> range, whichever is smaller.
> 
> Siddhesh

Sorry, I meant an external testcase to stress out such cases.