On Fri, Mar 8, 2024 at 7:07 PM Florian Westphal <fw@xxxxxxxxx> wrote:
>
> Sriram Rajagopalan <bglsriram@xxxxxxxxx> wrote:
> > diff --git a/src/rule.c b/src/rule.c
> > index 342c43fb..5def185d 100644
> > --- a/src/rule.c
> > +++ b/src/rule.c
> > @@ -2661,8 +2661,13 @@ static void payload_do_merge(struct stmt *sa[],
> > unsigned int n)
> > for (j = 0, i = 1; i < n; i++) {
> > stmt = sa[i];
> > this = stmt->expr;
> > -
> > - if (!payload_can_merge(last->left, this->left) ||
> > + /* We should not be merging two OP_NEQs. For example -
> > + * tcp sport != 22 tcp dport != 23 should not result in
> > + * [ payload load 4b @ transport header + 0 => reg 1 ]
> > + * [ cmp neq reg 1 0x17001600 ]
> > + */
> > + if (this->op == OP_NEQ ||
> > + !payload_can_merge(last->left, this->left) ||
> > !relational_ops_match(last, this)) {
> > last = this;
> > j = i;
> >
> > Please review the patches above and provide your feedback. Please let
> > me know of any questions/clarifications.
>
>
> Probably better to do:
>
> diff --git a/src/rule.c b/src/rule.c
> --- a/src/rule.c
> +++ b/src/rule.c
> @@ -2766,7 +2766,6 @@ static void stmt_reduce(const struct rule *rule)
> switch (stmt->expr->op) {
> case OP_EQ:
> case OP_IMPLICIT:
> - case OP_NEQ:
> break;
> default:
> continue;
>
Sure, it makes sense to prevent this at the caller of
payload_do_merge(), i.e within stmt_reduce() itself.
Thanks,
Sriram
