Hi Darrell,
Darrell Ball <dlu...@gmail.com> writes:
> Presently, alg processing is enabled by default to exercise testing.
> This is similar to kernels before 4.7. The recommended default
> behavior in the kernel is to only process algs if a helper is
> supplied in a conntrack rule. The behavior is changed to match the
> later kernels.
>
> Signed-off-by: Darrell Ball <dlu...@gmail.com>
> ---
I like having the alg processing off by default. However, at this point
some folks may be relying (or not care about) on the default behavior of
the ct() action (which is to assume
ALG=whatever-is-appropriate-to-the-port). I think we probably will need
to have a knob to enable / disable this.
More below.
> lib/conntrack.c | 35 +++++++++++++++++++++++++++++------
> 1 file changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 7fbcfba..dea2fed 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -789,13 +789,34 @@ conn_clean(struct conntrack *ct, struct conn *conn,
> }
> }
>
> +static bool
> +ct_verify_helper(const char *helper, enum ct_alg_ctl_type ct_alg_ctl)
> +{
Since we know the algorithm at this point, doesn't it make sense to
just associate the alg with the connection? I say that because if I
read this correctly (and I haven't had coffee yet - maybe I missed
something) non-standard ports won't get their associations properly (so
if I want to run ftp on port 2122 for example, the helper will not get
associated).
If we take the approach I mentioned in 2/4, we can just associate the
function pointer. After all, the user asked for it - we should do what
the user wants.
> + if (ct_alg_ctl == CT_ALG_CTL_NONE) {
> + return true;
> + } else if (helper) {
> + if ((ct_alg_ctl == CT_ALG_CTL_FTP) &&
> + !strncmp(helper, "ftp", strlen("ftp"))) {
> + return true;
> + } else if ((ct_alg_ctl == CT_ALG_CTL_TFTP) &&
> + !strncmp(helper, "tftp", strlen("tftp"))) {
> + return true;
> + } else {
> + return false;
> + }
> + } else {
> + return false;
> + }
> +}
> +
> /* This function is called with the bucket lock held. */
> static struct conn *
> conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
> struct conn_lookup_ctx *ctx, bool commit, long long now,
> const struct nat_action_info_t *nat_action_info,
> struct conn *conn_for_un_nat_copy,
> - const char *helper, const struct alg_exp_node *alg_exp)
> + const char *helper, const struct alg_exp_node *alg_exp,
> + enum ct_alg_ctl_type ct_alg_ctl)
> {
> struct conn *nc = NULL;
>
> @@ -819,15 +840,16 @@ conn_not_found(struct conntrack *ct, struct dp_packet
> *pkt,
> return nc;
> }
>
> + if (!ct_verify_helper(helper, ct_alg_ctl)) {
> + return nc;
> + }
> +
> unsigned bucket = hash_to_bucket(ctx->hash);
> nc = new_conn(&ct->buckets[bucket], pkt, &ctx->key, now);
> ctx->conn = nc;
> nc->rev_key = nc->key;
> conn_key_reverse(&nc->rev_key);
> -
> - if (helper) {
> - nc->alg = xstrdup(helper);
> - }
> + nc->alg = nullable_xstrdup(helper);
>
> if (alg_exp) {
> nc->alg_related = true;
> @@ -1182,7 +1204,8 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
> }
> ct_rwlock_unlock(&ct->resources_lock);
> conn = conn_not_found(ct, pkt, ctx, commit, now, nat_action_info,
> - &conn_for_un_nat_copy, helper, alg_exp);
> + &conn_for_un_nat_copy, helper, alg_exp,
> + ct_alg_ctl);
> }
> write_ct_md(pkt, zone, conn, &ctx->key, alg_exp);
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
