Re: [dpdk-dev] [PATCH v2 2/2] eal: fix signed integers in fbarray
On Fri, Apr 13, 2018 at 05:09:01PM +0100, Burakov, Anatoly wrote:
> On 13-Apr-18 4:56 PM, Adrien Mazarguil wrote:
> > While debugging startup issues encountered with Clang (see "eal: fix
> > undefined behavior in fbarray"), I noticed that fbarray stores indices,
> > sizes and masks on signed integers involved in bitwise operations.
> >
> > Such operations almost invariably cause undefined behavior with values that
> > cannot be represented by the result type, as is often the case with
> > bit-masks and left-shifts.
> >
> > This patch replaces them with unsigned integers as a safety measure and
> > promotes a few internal variables to larger types for consistency.
> >
> > Fixes: c44d09811b40 ("eal: add shared indexed file-backed array")
> > Cc: Anatoly Burakov
> >
> > Signed-off-by: Adrien Mazarguil
> >
> > --
> >
> > v2 changes:
> >
> > Removed unnecessary "(unsigned int)" cast leftovers.
>
> Thanks for figuring this out! In general, i'm OK with the change, however...
>
> > ---
> > lib/librte_eal/common/eal_common_fbarray.c | 97
> > lib/librte_eal/common/include/rte_fbarray.h | 33
> > 2 files changed, 68 insertions(+), 62 deletions(-)
> >
> > diff --git a/lib/librte_eal/common/eal_common_fbarray.c
> > b/lib/librte_eal/common/eal_common_fbarray.c
> > index 11aa3f22a..368290654 100644
> > --- a/lib/librte_eal/common/eal_common_fbarray.c
> > +++ b/lib/librte_eal/common/eal_common_fbarray.c
> > @@ -21,7 +21,7 @@
> > #include "rte_fbarray.h"
> > #define MASK_SHIFT 6ULL
> > -#define MASK_ALIGN (1 << MASK_SHIFT)
> > +#define MASK_ALIGN (1ULL << MASK_SHIFT)
> > #define MASK_LEN_TO_IDX(x) ((x) >> MASK_SHIFT)
> > #define MASK_LEN_TO_MOD(x) ((x) - RTE_ALIGN_FLOOR(x, MASK_ALIGN))
> > #define MASK_GET_IDX(idx, mod) ((idx << MASK_SHIFT) + mod)
> > @@ -32,12 +32,12 @@
> >*/
>
> <...>
>
> > int __rte_experimental
> > -rte_fbarray_find_next_free(struct rte_fbarray *arr, int start)
> > +rte_fbarray_find_next_free(struct rte_fbarray *arr, unsigned int start)
> > {
>
> This leads to inconsistency here. As it is, we can specify len and start
> value up to UINT32_MAX, but this (and others like it) function will only
> return values up to INT32_MAX.
>
> One way to fix this would be to prohibit len being >= INT32_MAX on array
> creation. The place to fix this would probably be fully_validate().
Indeed, also I just received a Coverity report about a bunch of other
details due to these changes (now it's obvious that calc_data_size() doesn't
support negative page sizes), I'll update the patch accordingly and submit
v3. Thanks.
>
> > int ret = -1;
> > - if (arr == NULL || start < 0 || start >= arr->len) {
> > + if (arr == NULL || start >= arr->len) {
> > rte_errno = EINVAL;
> > return -1;
> > }
> > @@ -683,11 +686,11 @@ rte_fbarray_find_next_free(struct rte_fbarray *arr,
> > int start)
>
>
> --
> Thanks,
> Anatoly
--
Adrien Mazarguil
6WIND
Re: [dpdk-dev] [PATCH v2 2/2] eal: fix signed integers in fbarray
On 13-Apr-18 4:56 PM, Adrien Mazarguil wrote:
While debugging startup issues encountered with Clang (see "eal: fix
undefined behavior in fbarray"), I noticed that fbarray stores indices,
sizes and masks on signed integers involved in bitwise operations.
Such operations almost invariably cause undefined behavior with values that
cannot be represented by the result type, as is often the case with
bit-masks and left-shifts.
This patch replaces them with unsigned integers as a safety measure and
promotes a few internal variables to larger types for consistency.
Fixes: c44d09811b40 ("eal: add shared indexed file-backed array")
Cc: Anatoly Burakov
Signed-off-by: Adrien Mazarguil
--
v2 changes:
Removed unnecessary "(unsigned int)" cast leftovers.
Thanks for figuring this out! In general, i'm OK with the change, however...
---
lib/librte_eal/common/eal_common_fbarray.c | 97
lib/librte_eal/common/include/rte_fbarray.h | 33
2 files changed, 68 insertions(+), 62 deletions(-)
diff --git a/lib/librte_eal/common/eal_common_fbarray.c
b/lib/librte_eal/common/eal_common_fbarray.c
index 11aa3f22a..368290654 100644
--- a/lib/librte_eal/common/eal_common_fbarray.c
+++ b/lib/librte_eal/common/eal_common_fbarray.c
@@ -21,7 +21,7 @@
#include "rte_fbarray.h"
#define MASK_SHIFT 6ULL
-#define MASK_ALIGN (1 << MASK_SHIFT)
+#define MASK_ALIGN (1ULL << MASK_SHIFT)
#define MASK_LEN_TO_IDX(x) ((x) >> MASK_SHIFT)
#define MASK_LEN_TO_MOD(x) ((x) - RTE_ALIGN_FLOOR(x, MASK_ALIGN))
#define MASK_GET_IDX(idx, mod) ((idx << MASK_SHIFT) + mod)
@@ -32,12 +32,12 @@
*/
<...>
int __rte_experimental
-rte_fbarray_find_next_free(struct rte_fbarray *arr, int start)
+rte_fbarray_find_next_free(struct rte_fbarray *arr, unsigned int start)
{
This leads to inconsistency here. As it is, we can specify len and start
value up to UINT32_MAX, but this (and others like it) function will only
return values up to INT32_MAX.
One way to fix this would be to prohibit len being >= INT32_MAX on array
creation. The place to fix this would probably be fully_validate().
int ret = -1;
- if (arr == NULL || start < 0 || start >= arr->len) {
+ if (arr == NULL || start >= arr->len) {
rte_errno = EINVAL;
return -1;
}
@@ -683,11 +686,11 @@ rte_fbarray_find_next_free(struct rte_fbarray *arr, int
start)
--
Thanks,
Anatoly
[dpdk-dev] [PATCH v2 2/2] eal: fix signed integers in fbarray
While debugging startup issues encountered with Clang (see "eal: fix
undefined behavior in fbarray"), I noticed that fbarray stores indices,
sizes and masks on signed integers involved in bitwise operations.
Such operations almost invariably cause undefined behavior with values that
cannot be represented by the result type, as is often the case with
bit-masks and left-shifts.
This patch replaces them with unsigned integers as a safety measure and
promotes a few internal variables to larger types for consistency.
Fixes: c44d09811b40 ("eal: add shared indexed file-backed array")
Cc: Anatoly Burakov
Signed-off-by: Adrien Mazarguil
--
v2 changes:
Removed unnecessary "(unsigned int)" cast leftovers.
---
lib/librte_eal/common/eal_common_fbarray.c | 97
lib/librte_eal/common/include/rte_fbarray.h | 33
2 files changed, 68 insertions(+), 62 deletions(-)
diff --git a/lib/librte_eal/common/eal_common_fbarray.c
b/lib/librte_eal/common/eal_common_fbarray.c
index 11aa3f22a..368290654 100644
--- a/lib/librte_eal/common/eal_common_fbarray.c
+++ b/lib/librte_eal/common/eal_common_fbarray.c
@@ -21,7 +21,7 @@
#include "rte_fbarray.h"
#define MASK_SHIFT 6ULL
-#define MASK_ALIGN (1 << MASK_SHIFT)
+#define MASK_ALIGN (1ULL << MASK_SHIFT)
#define MASK_LEN_TO_IDX(x) ((x) >> MASK_SHIFT)
#define MASK_LEN_TO_MOD(x) ((x) - RTE_ALIGN_FLOOR(x, MASK_ALIGN))
#define MASK_GET_IDX(idx, mod) ((idx << MASK_SHIFT) + mod)
@@ -32,12 +32,12 @@
*/
struct used_mask {
- int n_masks;
+ unsigned int n_masks;
uint64_t data[];
};
static size_t
-calc_mask_size(int len)
+calc_mask_size(unsigned int len)
{
/* mask must be multiple of MASK_ALIGN, even though length of array
* itself may not be aligned on that boundary.
@@ -48,7 +48,7 @@ calc_mask_size(int len)
}
static size_t
-calc_data_size(size_t page_sz, int elt_sz, int len)
+calc_data_size(size_t page_sz, unsigned int elt_sz, unsigned int len)
{
size_t data_sz = elt_sz * len;
size_t msk_sz = calc_mask_size(len);
@@ -56,7 +56,7 @@ calc_data_size(size_t page_sz, int elt_sz, int len)
}
static struct used_mask *
-get_used_mask(void *data, int elt_sz, int len)
+get_used_mask(void *data, unsigned int elt_sz, unsigned int len)
{
return (struct used_mask *) RTE_PTR_ADD(data, elt_sz * len);
}
@@ -86,13 +86,14 @@ resize_and_map(int fd, void *addr, size_t len)
}
static int
-find_next_n(const struct rte_fbarray *arr, int start, int n, bool used)
+find_next_n(const struct rte_fbarray *arr, unsigned int start, unsigned int n,
+ bool used)
{
const struct used_mask *msk = get_used_mask(arr->data, arr->elt_sz,
arr->len);
- int msk_idx, lookahead_idx, first, first_mod;
- int last, last_mod, last_msk;
- uint64_t ignore_msk;
+ unsigned int msk_idx, lookahead_idx, first, first_mod;
+ unsigned int last, last_mod;
+ uint64_t last_msk, ignore_msk;
/*
* mask only has granularity of MASK_ALIGN, but start may not be aligned
@@ -108,11 +109,11 @@ find_next_n(const struct rte_fbarray *arr, int start, int
n, bool used)
*/
last = MASK_LEN_TO_IDX(arr->len);
last_mod = MASK_LEN_TO_MOD(arr->len);
- last_msk = ~(-(1ULL) << last_mod);
+ last_msk = ~(-1ULL << last_mod);
for (msk_idx = first; msk_idx < msk->n_masks; msk_idx++) {
uint64_t cur_msk, lookahead_msk;
- int run_start, clz, left;
+ unsigned int run_start, clz, left;
bool found = false;
/*
* The process of getting n consecutive bits for arbitrary n is
@@ -157,7 +158,7 @@ find_next_n(const struct rte_fbarray *arr, int start, int
n, bool used)
/* if n can fit in within a single mask, do a search */
if (n <= MASK_ALIGN) {
uint64_t tmp_msk = cur_msk;
- int s_idx;
+ unsigned int s_idx;
for (s_idx = 0; s_idx < n - 1; s_idx++)
tmp_msk &= tmp_msk >> 1ULL;
/* we found what we were looking for */
@@ -188,7 +189,7 @@ find_next_n(const struct rte_fbarray *arr, int start, int
n, bool used)
for (lookahead_idx = msk_idx + 1; lookahead_idx < msk->n_masks;
lookahead_idx++) {
- int s_idx, need;
+ unsigned int s_idx, need;
lookahead_msk = msk->data[lookahead_idx];
/* if we're looking for free space, invert the mask */
@@ -234,13 +235,13 @@ find_next_n(const struct rte_fbarray *arr, int start, int
n, bool used)
}
static int
-find_next(const struct rte_fbarray *arr, int start, bool used)
+find_next(const struct rte_fbarray *arr, unsigned int start, bool used)
{
const struct used_mask *msk = get_u
