[ovs-dev] use double postponing to free subtables.

From: Zhantao Fu <fuzhantao@huawei.com>

Now the subtable in classifier/dpcls is postponed by destroy_subtable before move itself out of cls->subtables(pvector).It will raise use-after-free problem.

use double postponing to free subtable

Signed-off-by: Zhantao Fu <fuzhantao@huawei.com>
---
 lib/classifier.c  | 31 +++++++++++++++++++------------
 lib/dpif-netdev.c | 10 ++++++++--
 2 files changed, 27 insertions(+), 14 deletions(-)

On 15.04.2019 13:55, fuzhantao wrote:
> From: Zhantao Fu <fuzhantao@huawei.com>
> 
> Now the subtable in classifier/dpcls is postponed by destroy_subtable before move itself out of cls->subtables(pvector).It will raise use-after-free problem.
> 
> use double postponing to free subtable
> 
> Signed-off-by: Zhantao Fu <fuzhantao@huawei.com>
> ---

Hi. Thanks for the patch.

The code looks good to me. There are few issues with patch formatting
that, probably, could be fixed while applying (no need to send a new
version):

1. The patch subject should start with the capital letter in case the
   patch area is omitted: "Use double postponing to free subtables."
2. It's better to limit the line length in commit message too. The
   commonly used value is 72 characters. Otherwise it doesn't look
   good in 'git log'. (no need to limit tags like 'Fixes:', etc.)
3. No need to duplicate subject in the commit-message body.
4. Don't need to have both 'dev' and 'ovs-dev' in CC list. Patches
   will arrive twice to the list. Use only one of them. 'ovs-dev'
   is preferred.

Anyway, I'd like to suggest a bit different commit-message:

"""
Subtable destruction should be double postponed because readers could
still obtain old values while iterating over pvector implementation
before its new version published.

Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2019-April/357758.html
"""

Beside that, for the code:
Acked-by: Ilya Maximets <i.maximets@samsung.com>

For the future:
While sending the new version of the patch you may add version number
near to "PATCH" like: [PATCH v2] Use double postponing to free subtables.
This will help to visually recognize that this is the new version and
it's somehow different from the previous. You may also add description
of what changed between patch versions here under the '---'.

Best regards, Ilya Maximets.

>  lib/classifier.c  | 31 +++++++++++++++++++------------
>  lib/dpif-netdev.c | 10 ++++++++--
>  2 files changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/classifier.c b/lib/classifier.c
> index edb40c8..0fad953 100644
> --- a/lib/classifier.c
> +++ b/lib/classifier.c
> @@ -1468,6 +1468,24 @@ miniflow_get_map_in_range(const struct miniflow *miniflow, uint8_t start,
>      return map;
>  }
>  
> +static void
> +subtable_destroy_cb(struct cls_subtable *subtable)
> +{
> +    int i;
> +
> +    ovs_assert(ovsrcu_get_protected(struct trie_node *, &subtable->ports_trie)
> +               == NULL);
> +    ovs_assert(cmap_is_empty(&subtable->rules));
> +    ovs_assert(rculist_is_empty(&subtable->rules_list));
> +
> +    for (i = 0; i < subtable->n_indices; i++) {
> +        ccmap_destroy(&subtable->indices[i]);
> +    }
> +    cmap_destroy(&subtable->rules);
> +
> +    ovsrcu_postpone(free, subtable);
> +}
> +
>  /* The new subtable will be visible to the readers only after this. */
>  static struct cls_subtable *
>  insert_subtable(struct classifier *cls, const struct minimask *mask)
> @@ -1534,22 +1552,11 @@ insert_subtable(struct classifier *cls, const struct minimask *mask)
>  static void
>  destroy_subtable(struct classifier *cls, struct cls_subtable *subtable)
>  {
> -    int i;
> -
>      pvector_remove(&cls->subtables, subtable);
>      cmap_remove(&cls->subtables_map, &subtable->cmap_node,
>                  minimask_hash(&subtable->mask, 0));
>  
> -    ovs_assert(ovsrcu_get_protected(struct trie_node *, &subtable->ports_trie)
> -               == NULL);
> -    ovs_assert(cmap_is_empty(&subtable->rules));
> -    ovs_assert(rculist_is_empty(&subtable->rules_list));
> -
> -    for (i = 0; i < subtable->n_indices; i++) {
> -        ccmap_destroy(&subtable->indices[i]);
> -    }
> -    cmap_destroy(&subtable->rules);
> -    ovsrcu_postpone(free, subtable);
> +    ovsrcu_postpone(subtable_destroy_cb, subtable);
>  }
>  
>  static unsigned int be_get_bit_at(const ovs_be32 value[], unsigned int ofs);
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 4d6d0c3..8657a61 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -7592,6 +7592,13 @@ struct dpcls_subtable {
>      /* 'mask' must be the last field, additional space is allocated here. */
>  };
>  
> +static void
> +dpcls_subtable_destroy_cb(struct dpcls_subtable *subtable)
> +{
> +    cmap_destroy(&subtable->rules);
> +    ovsrcu_postpone(free, subtable);
> +}
> +
>  /* Initializes 'cls' as a classifier that initially contains no classification
>   * rules. */
>  static void
> @@ -7608,8 +7615,7 @@ dpcls_destroy_subtable(struct dpcls *cls, struct dpcls_subtable *subtable)
>      pvector_remove(&cls->subtables, subtable);
>      cmap_remove(&cls->subtables_map, &subtable->cmap_node,
>                  subtable->mask.hash);
> -    cmap_destroy(&subtable->rules);
> -    ovsrcu_postpone(free, subtable);
> +    ovsrcu_postpone(dpcls_subtable_destroy_cb, subtable);
>  }
>  
>  /* Destroys 'cls'.  Rules within 'cls', if any, are not freed; this is the
>

On Mon, Apr 15, 2019 at 06:55:22PM +0800, fuzhantao wrote:
> From: Zhantao Fu <fuzhantao@huawei.com>
> 
> Now the subtable in classifier/dpcls is postponed by destroy_subtable before move itself out of cls->subtables(pvector).It will raise use-after-free problem.
> 
> use double postponing to free subtable
> 
> Signed-off-by: Zhantao Fu <fuzhantao@huawei.com>

In addition to Ilya's comments, I see that you've posted this patch a
number of times today.  Maybe that was just a mistake; all of us make
mistakes sometimes.  But maybe these were meant to be different versions
of the patch.  If so, then please include a version number in the patch
header, e.g. [PATCH v2].