[ovs-dev] [PATCH v2] miniflow: Remove unused values_inline branch from miniflow_move()

Jarno Rajahalme jrajahalme at nicira.com
Tue Aug 26 19:02:48 UTC 2014


On Aug 26, 2014, at 3:01 AM, Thomas Graf <tgraf at noironetworks.com> wrote:

> The branch is unused as size < sizeof dst->inline_values must
> always be true for inlined values. Hitting the branch would lead
> to corruption as inline_values is accessed out of bounds.
> 

Miniflows can also be dynamically allocated to have more inline space than the default amount. classifier.c makes use of that, but it never calls miniflow_move (even indirectly) so you are not getting an assert fail. The motivation for right-sized miniflows in the classifier is to reduce levels of indirection while not wasting memory.

I should (have) extend(ed) the comment above struct miniflow definition in lib/flow.h to describe the above.

size is computed from the actual source miniflow instance, not from the generic miniflow struct. If size really is larger than the default inline space, then obviously no out of bounds access would occur.

I’m OK changing this function to not work for dynamically allocated miniflows, but that should be reflected in the comment above the function, which currently reads:

/* Initializes 'dst' with the data in 'src', destroying 'src'.
 * The caller must eventually free 'dst' with miniflow_destroy().
 * 'dst' must be regularly sized miniflow, but 'src' can have
 * larger than default inline values. */

However, it might be better to revert this patch to keep the larger-than-default miniflows not stand out too much.

Regards,

  Jarno

> Remove branch and add assertion.
> 
> Cc: Jarno Rajahalme <jrajahalme at nicira.com>
> Signed-off-by: Thomas Graf <tgraf at noironetworks.com>
> ---
> v2: fixed assertion
> 
> lib/flow.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/flow.c b/lib/flow.c
> index 29b331e..bef2f27 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -1726,12 +1726,9 @@ miniflow_move(struct miniflow *dst, struct miniflow *src)
>         dst->values_inline = true;
>         memcpy(dst->inline_values, miniflow_get_values(src), size);
>         miniflow_destroy(src);
> -    } else if (src->values_inline) {
> -        dst->values_inline = false;
> -        COVERAGE_INC(miniflow_malloc);
> -        dst->offline_values = xmalloc(size);
> -        memcpy(dst->offline_values, src->inline_values, size);
>     } else {
> +        ovs_assert(!src->values_inline);
> +
>         dst->values_inline = false;
>         dst->offline_values = src->offline_values;
>     }
> -- 
> 1.9.3
> 




More information about the dev mailing list