From a03ede64936d0aee1a760a19dc6194b2fdc9c692 Mon Sep 17 00:00:00 2001 From: Ondrej Zajicek Date: Tue, 3 Jan 2012 00:42:25 +0100 Subject: [PATCH] Fixes a tricky bug in route filtering. Route attributes was used after rta was freed during copy-on-write in filter code. This causes some random crashes, esp. with multipath routes. --- filter/filter.c | 100 ++++++++++++++++++++++++++++++++++++------------ lib/slab.c | 1 + 2 files changed, 77 insertions(+), 24 deletions(-) diff --git a/filter/filter.c b/filter/filter.c index b3b17dcf..d6d338bf 100644 --- a/filter/filter.c +++ b/filter/filter.c @@ -485,24 +485,42 @@ val_print(struct f_val v) } } -static struct rte **f_rte, *f_rte_old; -static struct linpool *f_pool; +static struct rte **f_rte; +static struct rta *f_old_rta; static struct ea_list **f_tmp_attrs; +static struct linpool *f_pool; static int f_flags; +static inline void f_rte_cow(void) +{ + *f_rte = rte_cow(*f_rte); +} + /* * rta_cow - prepare rta for modification by filter */ static void -rta_cow(void) +f_rta_cow(void) { if ((*f_rte)->attrs->aflags & RTAF_CACHED) { - rta *f_rta_copy = lp_alloc(f_pool, sizeof(rta)); - memcpy(f_rta_copy, (*f_rte)->attrs, sizeof(rta)); - f_rta_copy->aflags = 0; - *f_rte = rte_cow(*f_rte); - rta_free((*f_rte)->attrs); - (*f_rte)->attrs = f_rta_copy; + + /* Prepare to modify rte */ + f_rte_cow(); + + /* Store old rta to free it later */ + f_old_rta = (*f_rte)->attrs; + + /* + * Alloc new rta, do shallow copy and update rte. Fields eattrs + * and nexthops of rta are shared with f_old_rta (they will be + * copied when the cached rta will be obtained at the end of + * f_run()), also the lock of hostentry is inherited (we suppose + * hostentry is not changed by filters). + */ + rta *ra = lp_alloc(f_pool, sizeof(rta)); + memcpy(ra, f_old_rta, sizeof(rta)); + ra->aflags = 0; + (*f_rte)->attrs = ra; } } @@ -819,7 +837,7 @@ interpret(struct f_inst *what) ONEARG; if (what->aux != v1.type) runtime( "Attempt to set static attribute to incompatible type" ); - rta_cow(); + f_rta_cow(); { struct rta *rta = (*f_rte)->attrs; switch (what->aux) { @@ -955,7 +973,7 @@ interpret(struct f_inst *what) } if (!(what->aux & EAF_TEMP) && (!(f_flags & FF_FORCE_TMPATTR))) { - rta_cow(); + f_rta_cow(); l->next = (*f_rte)->attrs->eattrs; (*f_rte)->attrs->eattrs = l; } else { @@ -974,7 +992,7 @@ interpret(struct f_inst *what) runtime( "Can't set preference to non-integer" ); if ((v1.val.i < 0) || (v1.val.i > 0xFFFF)) runtime( "Setting preference value out of bounds" ); - *f_rte = rte_cow(*f_rte); + f_rte_cow(); (*f_rte)->pref = v1.val.i; break; case 'L': /* Get length of */ @@ -1300,29 +1318,64 @@ i_same(struct f_inst *f1, struct f_inst *f2) } /** - * f_run - external entry point to filters - * @filter: pointer to filter to run - * @tmp_attrs: where to store newly generated temporary attributes - * @rte: pointer to pointer to &rte being filtered. When route is modified, this is changed with rte_cow(). + * f_run - run a filter for a route + * @filter: filter to run + * @rte: route being filtered, may be modified + * @tmp_attrs: temporary attributes, prepared by caller or generated by f_run() * @tmp_pool: all filter allocations go from this pool * @flags: flags + * + * If filter needs to modify the route, there are several + * posibilities. @rte might be read-only (with REF_COW flag), in that + * case rw copy is obtained by rte_cow() and @rte is replaced. If + * @rte is originally rw, it may be directly modified (and it is never + * copied). + * + * The returned rte may reuse the (possibly cached, cloned) rta, or + * (if rta was modificied) contains a modified uncached rta, which + * uses parts allocated from @tmp_pool and parts shared from original + * rta. There is one exception - if @rte is rw but contains a cached + * rta and that is modified, rta in returned rte is also cached. + * + * Ownership of cached rtas is consistent with rte, i.e. + * if a new rte is returned, it has its own clone of cached rta + * (and cached rta of read-only source rte is intact), if rte is + * modified in place, old cached rta is possibly freed. */ int f_run(struct filter *filter, struct rte **rte, struct ea_list **tmp_attrs, struct linpool *tmp_pool, int flags) { - struct f_inst *inst; - struct f_val res; + int rte_cow = ((*rte)->flags & REF_COW); DBG( "Running filter `%s'...", filter->name ); - f_flags = flags; - f_tmp_attrs = tmp_attrs; f_rte = rte; - f_rte_old = *rte; + f_old_rta = NULL; + f_tmp_attrs = tmp_attrs; f_pool = tmp_pool; - inst = filter->root; + f_flags = flags; log_reset(); - res = interpret(inst); + struct f_val res = interpret(filter->root); + + if (f_old_rta) { + /* + * Cached rta was modified and f_rte contains now an uncached one, + * sharing some part with the cached one. The cached rta should + * be freed (if rte was originally COW, f_old_rta is a clone + * obtained during rte_cow()). + * + * This also implements the exception mentioned in f_run() + * description. The reason for this is that rta reuses parts of + * f_old_rta, and these may be freed during rta_free(f_old_rta). + * This is not the problem if rte was COW, because original rte + * also holds the same rta. + */ + if (!rte_cow) + (*f_rte)->attrs = rta_lookup((*f_rte)->attrs); + + rta_free(f_old_rta); + } + if (res.type != T_RETURN) { log( L_ERR "Filter %s did not return accept nor reject. Make up your mind", filter->name); @@ -1341,7 +1394,6 @@ f_eval_int(struct f_inst *expr) f_flags = 0; f_tmp_attrs = NULL; f_rte = NULL; - f_rte_old = NULL; f_pool = cfg_mem; log_reset(); diff --git a/lib/slab.c b/lib/slab.c index af6b50b0..e236e26e 100644 --- a/lib/slab.c +++ b/lib/slab.c @@ -60,6 +60,7 @@ static struct resclass sl_class = { sizeof(struct slab), slab_free, slab_dump, + NULL, slab_memsize };