| Message ID | 20220412144711.93354-8-pablo@netfilter.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nft,v4,1/7] src: add EXPR_F_KERNEL to identify expression in the kernel | expand |
Hi Pablo, On Tue, Apr 12, 2022 at 04:47:11PM +0200, Pablo Neira Ayuso wrote: [...] > diff --git a/src/intervals.c b/src/intervals.c > index 16debf9cd4be..65e0531765a6 100644 > --- a/src/intervals.c > +++ b/src/intervals.c > @@ -255,6 +255,262 @@ int set_automerge(struct list_head *msgs, struct cmd *cmd, struct set *set, > return 0; > } > > +static void remove_elem(struct expr *prev, struct set *set, struct expr *purge) > +{ > + struct expr *clone; > + > + if (!(prev->flags & EXPR_F_REMOVE)) { Does prev->flags ever contain EXPR_F_REMOVE? (See below.) > + if (prev->flags & EXPR_F_KERNEL) { > + clone = expr_clone(prev); > + list_move_tail(&clone->list, &purge->expressions); > + } else { > + list_del(&prev->list); > + expr_free(prev); > + } > + } > +} > + > +static void __adjust_elem_left(struct set *set, struct expr *prev, struct expr *i, > + struct expr *init) > +{ > + prev->flags &= EXPR_F_KERNEL; This looks odd. You're intentionally stripping all flags other than EXPR_F_KERNEL (if set)? IIUC, you're just dropping EXPR_F_REMOVE if set. If so, explicit 'prev->flags &= ~EXPR_F_REMOVE' is more clear, no? Maybe it's also irrelevant after all WRT above question. > + expr_free(prev->key->left); > + prev->key->left = expr_get(i->key->right); > + mpz_add_ui(prev->key->left->value, prev->key->left->value, 1); > + list_move(&prev->list, &init->expressions); > +} [...] > +static int setelem_delete(struct list_head *msgs, struct set *set, > + struct expr *init, struct expr *purge, > + struct expr *elems, unsigned int debug_mask) > +{ > + struct expr *i, *next, *prev = NULL; > + struct range range, prev_range; > + int err = 0; > + mpz_t rop; > + > + mpz_init(prev_range.low); > + mpz_init(prev_range.high); > + mpz_init(range.low); > + mpz_init(range.high); > + mpz_init(rop); > + > + list_for_each_entry_safe(i, next, &elems->expressions, list) { > + if (i->key->etype == EXPR_SET_ELEM_CATCHALL) > + continue; > + > + range_expr_value_low(range.low, i); > + range_expr_value_high(range.high, i); > + > + if (!prev && i->flags & EXPR_F_REMOVE) { > + expr_error(msgs, i, "element does not exist"); > + err = -1; > + goto err; > + } > + > + if (!(i->flags & EXPR_F_REMOVE)) { > + prev = i; > + mpz_set(prev_range.low, range.low); > + mpz_set(prev_range.high, range.high); > + continue; > + } The loop assigns to 'prev' only if EXPR_F_REMOVE is not set. > + > + if (mpz_cmp(prev_range.low, range.low) == 0 && > + mpz_cmp(prev_range.high, range.high) == 0) { > + if (!(prev->flags & EXPR_F_REMOVE) && > + i->flags & EXPR_F_REMOVE) { > + list_move_tail(&prev->list, &purge->expressions); > + list_del(&i->list); > + expr_free(i); > + } > + } else if (set->automerge && > + setelem_adjust(set, init, purge, &prev_range, &range, prev, i) < 0) { > + expr_error(msgs, i, "element does not exist"); > + err = -1; > + goto err; > + } > + prev = NULL; The code above may set EXPR_F_REMOVE in 'prev', but AFAICT 'prev' is not revisited within and cleared before next iteration. Cheers, Phil
On Wed, Apr 13, 2022 at 02:54:34PM +0200, Phil Sutter wrote: > Hi Pablo, > > On Tue, Apr 12, 2022 at 04:47:11PM +0200, Pablo Neira Ayuso wrote: > [...] > > diff --git a/src/intervals.c b/src/intervals.c > > index 16debf9cd4be..65e0531765a6 100644 > > --- a/src/intervals.c > > +++ b/src/intervals.c > > @@ -255,6 +255,262 @@ int set_automerge(struct list_head *msgs, struct cmd *cmd, struct set *set, > > return 0; > > } > > > > +static void remove_elem(struct expr *prev, struct set *set, struct expr *purge) > > +{ > > + struct expr *clone; > > + > > + if (!(prev->flags & EXPR_F_REMOVE)) { > > Does prev->flags ever contain EXPR_F_REMOVE? (See below.) EXPR_F_REMOVE flag is used to specify that this element represents a deletion. The existing list of elements in the kernel is mixed with the list of elements to be deleted, this list is merge-sorted, then we look for EXPR_F_REMOVE elements that are asking for removal of elements in the kernel. The flag allows me to track objects, whether they are in the kernel (EXPR_F_KERNEL), they are new (no flag) or they represent an element that need to be removed from the kernel (EXPR_F_REMOVE). > > + if (prev->flags & EXPR_F_KERNEL) { > > + clone = expr_clone(prev); > > + list_move_tail(&clone->list, &purge->expressions); > > + } else { > > + list_del(&prev->list); > > + expr_free(prev); > > + } > > + } > > +} > > + > > +static void __adjust_elem_left(struct set *set, struct expr *prev, struct expr *i, > > + struct expr *init) > > +{ > > + prev->flags &= EXPR_F_KERNEL; > > This looks odd. You're intentionally stripping all flags other than > EXPR_F_KERNEL (if set)? > IIUC, you're just dropping EXPR_F_REMOVE if set. If so, explicit > 'prev->flags &= ~EXPR_F_REMOVE' is more clear, no? > Maybe it's also irrelevant after all WRT above question. Yes, this should be prev->flags &= ~EXPR_F_KERNEL, I'll fix it. This element is moved to the list of elements to be added. This flag is irrelevant though at this stage, but in case you look at the list of elements to be added, you should not see EXPR_F_KERNEL there. > > + expr_free(prev->key->left); > > + prev->key->left = expr_get(i->key->right); > > + mpz_add_ui(prev->key->left->value, prev->key->left->value, 1); > > + list_move(&prev->list, &init->expressions); > > +} > [...] > > +static int setelem_delete(struct list_head *msgs, struct set *set, > > + struct expr *init, struct expr *purge, > > + struct expr *elems, unsigned int debug_mask) > > +{ > > + struct expr *i, *next, *prev = NULL; > > + struct range range, prev_range; > > + int err = 0; > > + mpz_t rop; > > + > > + mpz_init(prev_range.low); > > + mpz_init(prev_range.high); > > + mpz_init(range.low); > > + mpz_init(range.high); > > + mpz_init(rop); > > + > > + list_for_each_entry_safe(i, next, &elems->expressions, list) { > > + if (i->key->etype == EXPR_SET_ELEM_CATCHALL) > > + continue; > > + > > + range_expr_value_low(range.low, i); > > + range_expr_value_high(range.high, i); > > + > > + if (!prev && i->flags & EXPR_F_REMOVE) { > > + expr_error(msgs, i, "element does not exist"); > > + err = -1; > > + goto err; > > + } > > + > > + if (!(i->flags & EXPR_F_REMOVE)) { > > + prev = i; > > + mpz_set(prev_range.low, range.low); > > + mpz_set(prev_range.high, range.high); > > + continue; > > + } > > The loop assigns to 'prev' only if EXPR_F_REMOVE is not set. Yes, this annotates is a element candidate to be removed. The list of elements is merged-sorted, coming the element with EXPR_F_REMOVE before the element that needs to be removed. > > + if (mpz_cmp(prev_range.low, range.low) == 0 && > > + mpz_cmp(prev_range.high, range.high) == 0) { > > + if (!(prev->flags & EXPR_F_REMOVE) && > > + i->flags & EXPR_F_REMOVE) { > > + list_move_tail(&prev->list, &purge->expressions); > > + list_del(&i->list); > > + expr_free(i); > > + } > > + } else if (set->automerge && > > + setelem_adjust(set, init, purge, &prev_range, &range, prev, i) < 0) { > > + expr_error(msgs, i, "element does not exist"); > > + err = -1; > > + goto err; > > + } > > + prev = NULL; > > The code above may set EXPR_F_REMOVE in 'prev', but AFAICT 'prev' is not > revisited within and cleared before next iteration. Yes, it is intentional. First this annotates the element to be delete, next iteration should find a similar element with either EXPR_F_KERNEL (if already in the kernel) or no flag if it was freshly added in this batch.
On Wed, Apr 13, 2022 at 03:13:30PM +0200, Pablo Neira Ayuso wrote: > On Wed, Apr 13, 2022 at 02:54:34PM +0200, Phil Sutter wrote: [...] > > > +static void __adjust_elem_left(struct set *set, struct expr *prev, struct expr *i, > > > + struct expr *init) > > > +{ > > > + prev->flags &= EXPR_F_KERNEL; > > > > This looks odd. You're intentionally stripping all flags other than > > EXPR_F_KERNEL (if set)? > > IIUC, you're just dropping EXPR_F_REMOVE if set. If so, explicit > > 'prev->flags &= ~EXPR_F_REMOVE' is more clear, no? > > Maybe it's also irrelevant after all WRT above question. > > Yes, this should be prev->flags &= ~EXPR_F_KERNEL, I'll fix it. Ah, OK! > This element is moved to the list of elements to be added. This flag > is irrelevant though at this stage, but in case you look at the list > of elements to be added, you should not see EXPR_F_KERNEL there. I guess none of the flags are relevant at this point anymore since your code cleared them all and apparently passed testing? Or none of the relevant ones were set, which is my suspicion with EXPR_F_REMOVE. [...] > > > + list_for_each_entry_safe(i, next, &elems->expressions, list) { > > > + if (i->key->etype == EXPR_SET_ELEM_CATCHALL) > > > + continue; > > > + > > > + range_expr_value_low(range.low, i); > > > + range_expr_value_high(range.high, i); > > > + > > > + if (!prev && i->flags & EXPR_F_REMOVE) { > > > + expr_error(msgs, i, "element does not exist"); > > > + err = -1; > > > + goto err; > > > + } > > > + > > > + if (!(i->flags & EXPR_F_REMOVE)) { > > > + prev = i; > > > + mpz_set(prev_range.low, range.low); > > > + mpz_set(prev_range.high, range.high); > > > + continue; > > > + } > > > > The loop assigns to 'prev' only if EXPR_F_REMOVE is not set. > > Yes, this annotates is a element candidate to be removed. > > The list of elements is merged-sorted, coming the element with > EXPR_F_REMOVE before the element that needs to be removed. The one with EXPR_F_REMOVE comes *after* the one to be removed, right? My question again: Is it possible for 'prev' to have EXPR_F_REMOVE set? Maybe I miss something, but to me it looks like not although the code expects it. Cheers, Phil
On Wed, Apr 13, 2022 at 04:02:45PM +0200, Phil Sutter wrote: > On Wed, Apr 13, 2022 at 03:13:30PM +0200, Pablo Neira Ayuso wrote: > > On Wed, Apr 13, 2022 at 02:54:34PM +0200, Phil Sutter wrote: > [...] > > > > +static void __adjust_elem_left(struct set *set, struct expr *prev, struct expr *i, > > > > + struct expr *init) > > > > +{ > > > > + prev->flags &= EXPR_F_KERNEL; > > > > > > This looks odd. You're intentionally stripping all flags other than > > > EXPR_F_KERNEL (if set)? > > > IIUC, you're just dropping EXPR_F_REMOVE if set. If so, explicit > > > 'prev->flags &= ~EXPR_F_REMOVE' is more clear, no? > > > Maybe it's also irrelevant after all WRT above question. > > > > Yes, this should be prev->flags &= ~EXPR_F_KERNEL, I'll fix it. > > Ah, OK! > > > This element is moved to the list of elements to be added. This flag > > is irrelevant though at this stage, but in case you look at the list > > of elements to be added, you should not see EXPR_F_KERNEL there. > > I guess none of the flags are relevant at this point anymore since your > code cleared them all and apparently passed testing? Or none of the > relevant ones were set, which is my suspicion with EXPR_F_REMOVE. > > [...] > > > > + list_for_each_entry_safe(i, next, &elems->expressions, list) { > > > > + if (i->key->etype == EXPR_SET_ELEM_CATCHALL) > > > > + continue; > > > > + > > > > + range_expr_value_low(range.low, i); > > > > + range_expr_value_high(range.high, i); > > > > + > > > > + if (!prev && i->flags & EXPR_F_REMOVE) { > > > > + expr_error(msgs, i, "element does not exist"); > > > > + err = -1; > > > > + goto err; > > > > + } > > > > + > > > > + if (!(i->flags & EXPR_F_REMOVE)) { > > > > + prev = i; > > > > + mpz_set(prev_range.low, range.low); > > > > + mpz_set(prev_range.high, range.high); > > > > + continue; > > > > + } > > > > > > The loop assigns to 'prev' only if EXPR_F_REMOVE is not set. > > > > Yes, this annotates is a element candidate to be removed. > > > > The list of elements is merged-sorted, coming the element with > > EXPR_F_REMOVE before the element that needs to be removed. > > The one with EXPR_F_REMOVE comes *after* the one to be removed, right? Right, the other way around actually. > My question again: Is it possible for 'prev' to have EXPR_F_REMOVE set? > Maybe I miss something, but to me it looks like not although the code > expects it. prev never has EXPR_F_REMOVE, so it points to an existing element.
On Wed, Apr 13, 2022 at 04:27:07PM +0200, Pablo Neira Ayuso wrote: [...] > > The one with EXPR_F_REMOVE comes *after* the one to be removed, right? > > Right, the other way around actually. > > > My question again: Is it possible for 'prev' to have EXPR_F_REMOVE set? > > Maybe I miss something, but to me it looks like not although the code > > expects it. > > prev never has EXPR_F_REMOVE, so it points to an existing element. So below change should be fine? diff --git a/src/intervals.c b/src/intervals.c index 451bc4dd4dd45..c0077c06880ff 100644 --- a/src/intervals.c +++ b/src/intervals.c @@ -265,14 +265,12 @@ static void remove_elem(struct expr *prev, struct set *set, struct expr *purge) { struct expr *clone; - if (!(prev->flags & EXPR_F_REMOVE)) { - if (prev->flags & EXPR_F_KERNEL) { - clone = expr_clone(prev); - list_move_tail(&clone->list, &purge->expressions); - } else { - list_del(&prev->list); - expr_free(prev); - } + if (prev->flags & EXPR_F_KERNEL) { + clone = expr_clone(prev); + list_move_tail(&clone->list, &purge->expressions); + } else { + list_del(&prev->list); + expr_free(prev); } } @@ -360,18 +358,15 @@ static int setelem_adjust(struct set *set, struct expr *add, struct expr *purge, { if (mpz_cmp(prev_range->low, range->low) == 0 && mpz_cmp(prev_range->high, range->high) > 0) { - if (!(prev->flags & EXPR_F_REMOVE) && - i->flags & EXPR_F_REMOVE) + if (i->flags & EXPR_F_REMOVE) adjust_elem_left(set, prev, i, add, purge); } else if (mpz_cmp(prev_range->low, range->low) < 0 && mpz_cmp(prev_range->high, range->high) == 0) { - if (!(prev->flags & EXPR_F_REMOVE) && - i->flags & EXPR_F_REMOVE) + if (i->flags & EXPR_F_REMOVE) adjust_elem_right(set, prev, i, add, purge); } else if (mpz_cmp(prev_range->low, range->low) < 0 && mpz_cmp(prev_range->high, range->high) > 0) { - if (!(prev->flags & EXPR_F_REMOVE) && - i->flags & EXPR_F_REMOVE) + if (i->flags & EXPR_F_REMOVE) split_range(set, prev, i, add, purge); } else { return -1; @@ -417,8 +412,7 @@ static int setelem_delete(struct list_head *msgs, struct set *set, if (mpz_cmp(prev_range.low, range.low) == 0 && mpz_cmp(prev_range.high, range.high) == 0) { - if (!(prev->flags & EXPR_F_REMOVE) && - i->flags & EXPR_F_REMOVE) { + if (i->flags & EXPR_F_REMOVE) { list_move_tail(&prev->list, &purge->expressions); list_del(&i->list); expr_free(i);
On Wed, Apr 13, 2022 at 04:38:02PM +0200, Phil Sutter wrote: > On Wed, Apr 13, 2022 at 04:27:07PM +0200, Pablo Neira Ayuso wrote: > [...] > > > The one with EXPR_F_REMOVE comes *after* the one to be removed, right? > > > > Right, the other way around actually. > > > > > My question again: Is it possible for 'prev' to have EXPR_F_REMOVE set? > > > Maybe I miss something, but to me it looks like not although the code > > > expects it. > > > > prev never has EXPR_F_REMOVE, so it points to an existing element. > > So below change should be fine? Yes, setelem_adjust() always checks for if (!(prev->flags & EXPR_F_REMOVE)) Your patch looks correct to me. > diff --git a/src/intervals.c b/src/intervals.c > index 451bc4dd4dd45..c0077c06880ff 100644 > --- a/src/intervals.c > +++ b/src/intervals.c > @@ -265,14 +265,12 @@ static void remove_elem(struct expr *prev, struct set *set, struct expr *purge) > { > struct expr *clone; > > - if (!(prev->flags & EXPR_F_REMOVE)) { > - if (prev->flags & EXPR_F_KERNEL) { > - clone = expr_clone(prev); > - list_move_tail(&clone->list, &purge->expressions); > - } else { > - list_del(&prev->list); > - expr_free(prev); > - } > + if (prev->flags & EXPR_F_KERNEL) { > + clone = expr_clone(prev); > + list_move_tail(&clone->list, &purge->expressions); > + } else { > + list_del(&prev->list); > + expr_free(prev); > } > } > > @@ -360,18 +358,15 @@ static int setelem_adjust(struct set *set, struct expr *add, struct expr *purge, > { > if (mpz_cmp(prev_range->low, range->low) == 0 && > mpz_cmp(prev_range->high, range->high) > 0) { > - if (!(prev->flags & EXPR_F_REMOVE) && > - i->flags & EXPR_F_REMOVE) > + if (i->flags & EXPR_F_REMOVE) > adjust_elem_left(set, prev, i, add, purge); > } else if (mpz_cmp(prev_range->low, range->low) < 0 && > mpz_cmp(prev_range->high, range->high) == 0) { > - if (!(prev->flags & EXPR_F_REMOVE) && > - i->flags & EXPR_F_REMOVE) > + if (i->flags & EXPR_F_REMOVE) > adjust_elem_right(set, prev, i, add, purge); > } else if (mpz_cmp(prev_range->low, range->low) < 0 && > mpz_cmp(prev_range->high, range->high) > 0) { > - if (!(prev->flags & EXPR_F_REMOVE) && > - i->flags & EXPR_F_REMOVE) > + if (i->flags & EXPR_F_REMOVE) > split_range(set, prev, i, add, purge); > } else { > return -1; > @@ -417,8 +412,7 @@ static int setelem_delete(struct list_head *msgs, struct set *set, > > if (mpz_cmp(prev_range.low, range.low) == 0 && > mpz_cmp(prev_range.high, range.high) == 0) { > - if (!(prev->flags & EXPR_F_REMOVE) && > - i->flags & EXPR_F_REMOVE) { > + if (i->flags & EXPR_F_REMOVE) { > list_move_tail(&prev->list, &purge->expressions); > list_del(&i->list); > expr_free(i);
On Wed, Apr 13, 2022 at 04:38:02PM +0200, Phil Sutter wrote: > On Wed, Apr 13, 2022 at 04:27:07PM +0200, Pablo Neira Ayuso wrote: > [...] > > > The one with EXPR_F_REMOVE comes *after* the one to be removed, right? > > > > Right, the other way around actually. > > > > > My question again: Is it possible for 'prev' to have EXPR_F_REMOVE set? > > > Maybe I miss something, but to me it looks like not although the code > > > expects it. > > > > prev never has EXPR_F_REMOVE, so it points to an existing element. > > So below change should be fine? Wait. > diff --git a/src/intervals.c b/src/intervals.c > index 451bc4dd4dd45..c0077c06880ff 100644 > --- a/src/intervals.c > +++ b/src/intervals.c [...] > @@ -360,18 +358,15 @@ static int setelem_adjust(struct set *set, struct expr *add, struct expr *purge, > { > if (mpz_cmp(prev_range->low, range->low) == 0 && > mpz_cmp(prev_range->high, range->high) > 0) { > - if (!(prev->flags & EXPR_F_REMOVE) && > - i->flags & EXPR_F_REMOVE) > + if (i->flags & EXPR_F_REMOVE) This chunk is not correct. User might ask to delete an element which does not exist. Then, you might find two consecutive EXPR_F_REMOVE. Only the initial chunk in this patch is fine.
On Wed, Apr 13, 2022 at 04:47:07PM +0200, Pablo Neira Ayuso wrote: > On Wed, Apr 13, 2022 at 04:38:02PM +0200, Phil Sutter wrote: > > On Wed, Apr 13, 2022 at 04:27:07PM +0200, Pablo Neira Ayuso wrote: > > [...] > > > > The one with EXPR_F_REMOVE comes *after* the one to be removed, right? > > > > > > Right, the other way around actually. > > > > > > > My question again: Is it possible for 'prev' to have EXPR_F_REMOVE set? > > > > Maybe I miss something, but to me it looks like not although the code > > > > expects it. > > > > > > prev never has EXPR_F_REMOVE, so it points to an existing element. > > > > So below change should be fine? > > Wait. > > > diff --git a/src/intervals.c b/src/intervals.c > > index 451bc4dd4dd45..c0077c06880ff 100644 > > --- a/src/intervals.c > > +++ b/src/intervals.c > [...] > > @@ -360,18 +358,15 @@ static int setelem_adjust(struct set *set, struct expr *add, struct expr *purge, > > { > > if (mpz_cmp(prev_range->low, range->low) == 0 && > > mpz_cmp(prev_range->high, range->high) > 0) { > > - if (!(prev->flags & EXPR_F_REMOVE) && > > - i->flags & EXPR_F_REMOVE) > > + if (i->flags & EXPR_F_REMOVE) > > This chunk is not correct. > > User might ask to delete an element which does not exist. > > Then, you might find two consecutive EXPR_F_REMOVE. > > Only the initial chunk in this patch is fine. Wait, you mean prev is always !EXPR_F_REMOVE, then this should be fine.
diff --git a/include/expression.h b/include/expression.h index c2d67d4c88af..2c3818e89b79 100644 --- a/include/expression.h +++ b/include/expression.h @@ -202,6 +202,7 @@ enum expr_flags { EXPR_F_BOOLEAN = 0x10, EXPR_F_INTERVAL = 0x20, EXPR_F_KERNEL = 0x40, + EXPR_F_REMOVE = 0x80, }; #include <payload.h> diff --git a/include/intervals.h b/include/intervals.h index 797129fc93a5..964804b19dda 100644 --- a/include/intervals.h +++ b/include/intervals.h @@ -4,6 +4,8 @@ void set_to_range(struct expr *init); int set_automerge(struct list_head *msgs, struct cmd *cmd, struct set *set, struct expr *init, unsigned int debug_mask); +int set_delete(struct list_head *msgs, struct cmd *cmd, struct set *set, + struct expr *init, unsigned int debug_mask); int set_overlap(struct list_head *msgs, struct set *set, struct expr *init); int set_to_intervals(const struct set *set, struct expr *init, bool add); diff --git a/src/evaluate.c b/src/evaluate.c index c79f3429eb2a..812f309e431a 100644 --- a/src/evaluate.c +++ b/src/evaluate.c @@ -1474,7 +1474,8 @@ static int interval_set_eval(struct eval_ctx *ctx, struct set *set, } break; case CMD_DELETE: - set_to_range(init); + ret = set_delete(ctx->msgs, ctx->cmd, set, init, + ctx->nft->debug_mask); break; case CMD_GET: break; diff --git a/src/intervals.c b/src/intervals.c index 16debf9cd4be..65e0531765a6 100644 --- a/src/intervals.c +++ b/src/intervals.c @@ -255,6 +255,262 @@ int set_automerge(struct list_head *msgs, struct cmd *cmd, struct set *set, return 0; } +static void remove_elem(struct expr *prev, struct set *set, struct expr *purge) +{ + struct expr *clone; + + if (!(prev->flags & EXPR_F_REMOVE)) { + if (prev->flags & EXPR_F_KERNEL) { + clone = expr_clone(prev); + list_move_tail(&clone->list, &purge->expressions); + } else { + list_del(&prev->list); + expr_free(prev); + } + } +} + +static void __adjust_elem_left(struct set *set, struct expr *prev, struct expr *i, + struct expr *init) +{ + prev->flags &= EXPR_F_KERNEL; + expr_free(prev->key->left); + prev->key->left = expr_get(i->key->right); + mpz_add_ui(prev->key->left->value, prev->key->left->value, 1); + list_move(&prev->list, &init->expressions); +} + +static void adjust_elem_left(struct set *set, struct expr *prev, struct expr *i, + struct expr *init, struct expr *purge) +{ + struct expr *clone; + + clone = expr_clone(prev); + list_move_tail(&clone->list, &purge->expressions); + + remove_elem(prev, set, purge); + __adjust_elem_left(set, prev, i, init); + + list_del(&i->list); + expr_free(i); + + clone = expr_clone(prev); + list_add_tail(&clone->list, &set->existing_set->init->expressions); +} + +static void __adjust_elem_right(struct set *set, struct expr *prev, struct expr *i, + struct expr *init) +{ + prev->flags &= EXPR_F_KERNEL; + expr_free(prev->key->right); + prev->key->right = expr_get(i->key->left); + mpz_sub_ui(prev->key->right->value, prev->key->right->value, 1); + list_move(&prev->list, &init->expressions); +} + +static void adjust_elem_right(struct set *set, struct expr *prev, struct expr *i, + struct expr *init, struct expr *purge) +{ + struct expr *clone; + + clone = expr_clone(prev); + list_move_tail(&clone->list, &purge->expressions); + + remove_elem(prev, set, purge); + __adjust_elem_right(set, prev, i, init); + + list_del(&i->list); + expr_free(i); + + clone = expr_clone(prev); + list_add_tail(&clone->list, &set->existing_set->init->expressions); +} + +static void split_range(struct set *set, struct expr *prev, struct expr *i, + struct expr *init, struct expr *purge) +{ + struct expr *clone; + + clone = expr_clone(prev); + list_move_tail(&clone->list, &purge->expressions); + + prev->flags &= EXPR_F_KERNEL; + clone = expr_clone(prev); + expr_free(clone->key->left); + clone->key->left = expr_get(i->key->right); + mpz_add_ui(clone->key->left->value, i->key->right->value, 1); + list_move(&clone->list, &init->expressions); + clone = expr_clone(clone); + list_add_tail(&clone->list, &set->existing_set->init->expressions); + + expr_free(prev->key->right); + prev->key->right = expr_get(i->key->left); + mpz_sub_ui(prev->key->right->value, i->key->left->value, 1); + list_move(&prev->list, &init->expressions); + clone = expr_clone(prev); + list_add_tail(&clone->list, &set->existing_set->init->expressions); + + list_del(&i->list); + expr_free(i); +} + +static int setelem_adjust(struct set *set, struct expr *init, struct expr *purge, + struct range *prev_range, struct range *range, + struct expr *prev, struct expr *i) +{ + if (mpz_cmp(prev_range->low, range->low) == 0 && + mpz_cmp(prev_range->high, range->high) > 0) { + if (!(prev->flags & EXPR_F_REMOVE) && + i->flags & EXPR_F_REMOVE) + adjust_elem_left(set, prev, i, init, purge); + } else if (mpz_cmp(prev_range->low, range->low) < 0 && + mpz_cmp(prev_range->high, range->high) == 0) { + if (!(prev->flags & EXPR_F_REMOVE) && + i->flags & EXPR_F_REMOVE) + adjust_elem_right(set, prev, i, init, purge); + } else if (mpz_cmp(prev_range->low, range->low) < 0 && + mpz_cmp(prev_range->high, range->high) > 0) { + if (!(prev->flags & EXPR_F_REMOVE) && + i->flags & EXPR_F_REMOVE) + split_range(set, prev, i, init, purge); + } else { + return -1; + } + + return 0; +} + +static int setelem_delete(struct list_head *msgs, struct set *set, + struct expr *init, struct expr *purge, + struct expr *elems, unsigned int debug_mask) +{ + struct expr *i, *next, *prev = NULL; + struct range range, prev_range; + int err = 0; + mpz_t rop; + + mpz_init(prev_range.low); + mpz_init(prev_range.high); + mpz_init(range.low); + mpz_init(range.high); + mpz_init(rop); + + list_for_each_entry_safe(i, next, &elems->expressions, list) { + if (i->key->etype == EXPR_SET_ELEM_CATCHALL) + continue; + + range_expr_value_low(range.low, i); + range_expr_value_high(range.high, i); + + if (!prev && i->flags & EXPR_F_REMOVE) { + expr_error(msgs, i, "element does not exist"); + err = -1; + goto err; + } + + if (!(i->flags & EXPR_F_REMOVE)) { + prev = i; + mpz_set(prev_range.low, range.low); + mpz_set(prev_range.high, range.high); + continue; + } + + if (mpz_cmp(prev_range.low, range.low) == 0 && + mpz_cmp(prev_range.high, range.high) == 0) { + if (!(prev->flags & EXPR_F_REMOVE) && + i->flags & EXPR_F_REMOVE) { + list_move_tail(&prev->list, &purge->expressions); + list_del(&i->list); + expr_free(i); + } + } else if (set->automerge && + setelem_adjust(set, init, purge, &prev_range, &range, prev, i) < 0) { + expr_error(msgs, i, "element does not exist"); + err = -1; + goto err; + } + prev = NULL; + } +err: + mpz_clear(prev_range.low); + mpz_clear(prev_range.high); + mpz_clear(range.low); + mpz_clear(range.high); + mpz_clear(rop); + + return err; +} + +static void automerge_delete(struct list_head *msgs, struct set *set, + struct expr *init, unsigned int debug_mask) +{ + struct set_automerge_ctx ctx = { + .set = set, + .init = init, + .debug_mask = debug_mask, + }; + + ctx.purge = set_expr_alloc(&internal_location, set); + list_expr_sort(&init->expressions); + setelem_automerge(&ctx); + expr_free(ctx.purge); +} + +/* detection for unexisting intervals already exists in Linux kernels >= 5.7. */ +int set_delete(struct list_head *msgs, struct cmd *cmd, struct set *set, + struct expr *init, unsigned int debug_mask) +{ + struct set *existing_set = set->existing_set; + struct expr *i, *add; + struct handle h = {}; + struct cmd *add_cmd; + int err; + + set_to_range(init); + if (set->automerge) + automerge_delete(msgs, set, init, debug_mask); + + list_for_each_entry(i, &init->expressions, list) + i->flags |= EXPR_F_REMOVE; + + set_to_range(existing_set->init); + list_splice_init(&init->expressions, &existing_set->init->expressions); + + list_expr_sort(&existing_set->init->expressions); + + add = set_expr_alloc(&internal_location, set); + + err = setelem_delete(msgs, set, add, init, existing_set->init, debug_mask); + if (err < 0) { + expr_free(add); + return err; + } + + if (debug_mask & NFT_DEBUG_SEGTREE) { + list_for_each_entry(i, &init->expressions, list) + gmp_printf("remove: [%Zx-%Zx]\n", + i->key->left->value, i->key->right->value); + list_for_each_entry(i, &add->expressions, list) + gmp_printf("add: [%Zx-%Zx]\n", + i->key->left->value, i->key->right->value); + list_for_each_entry(i, &existing_set->init->expressions, list) + gmp_printf("existing: [%Zx-%Zx]\n", + i->key->left->value, i->key->right->value); + } + + if (list_empty(&add->expressions)) { + expr_free(add); + return 0; + } + + handle_merge(&h, &cmd->handle); + add_cmd = cmd_alloc(CMD_ADD, CMD_OBJ_ELEMENTS, &h, &cmd->location, add); + add_cmd->elem.set = set_get(set); + list_add(&add_cmd->list, &cmd->list); + + return 0; +} + static int setelem_overlap(struct list_head *msgs, struct set *set, struct expr *init) { diff --git a/src/libnftables.c b/src/libnftables.c index dc0932bdbdd0..6a22ea093952 100644 --- a/src/libnftables.c +++ b/src/libnftables.c @@ -500,8 +500,8 @@ static int nft_evaluate(struct nft_ctx *nft, struct list_head *msgs, struct list_head *cmds) { struct nft_cache_filter *filter; + struct cmd *cmd, *next; unsigned int flags; - struct cmd *cmd; filter = nft_cache_filter_init(); flags = nft_cache_evaluate(nft, cmds, filter); @@ -512,7 +512,7 @@ static int nft_evaluate(struct nft_ctx *nft, struct list_head *msgs, nft_cache_filter_fini(filter); - list_for_each_entry(cmd, cmds, list) { + list_for_each_entry_safe(cmd, next, cmds, list) { struct eval_ctx ectx = { .nft = nft, .msgs = msgs,
Splice the existing set element cache with the elements to be deleted and merge sort it. The elements to be deleted are identified by the EXPR_F_REMOVE flag. The set elements to be deleted is automerged in first place if the automerge flag is set on. There are four possible deletion scenarios: - Exact match, eg. delete [a-b] and there is a [a-b] range in the kernel set. - Adjust left side of range, eg. delete [a-b] from range [a-x] where x > b. - Adjust right side of range, eg. delete [a-b] from range [x-b] where x < a. - Split range, eg. delete [a-b] from range [x-y] where x < a and b < y. Update nft_evaluate() to use the safe list variant since new commands are dynamically registered to the list to update ranges. This patch also restores the set element existence check for Linux kernels <= 5.7. Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> --- include/expression.h | 1 + include/intervals.h | 2 + src/evaluate.c | 3 +- src/intervals.c | 256 +++++++++++++++++++++++++++++++++++++++++++ src/libnftables.c | 4 +- 5 files changed, 263 insertions(+), 3 deletions(-)