Darrell Ball <db...@vmware.com> writes:
> There is a bug here in that the control connection should still be created
> even though it is likely an unused control connection.
> The following incremental fixes it.
Good catch. While thinking about it, I was wondering if it would be
possible to reject flows that attempt to add insane alg= helpers. Such
a check currently exists, but only when building ofp messages (at least
as far as I can tell). Anyway, that path lies outside the scope of this
patch but it's something to consider.
Please keep my ack if you repost with this incremental.
Thanks, Darrell!
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index fed9f61..18016c1 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -505,7 +505,7 @@ handle_alg_ctl(struct conntrack *ct, const struct
> conn_lookup_ctx *ctx,
> const struct conn *conn_for_expectation)
> {
> /* ALG control packet handling with expectation creation. */
> - if (OVS_UNLIKELY(alg_helpers[ct_alg_ctl] && conn)) {
> + if (OVS_UNLIKELY(alg_helpers[ct_alg_ctl] && conn && conn->alg)) {
> alg_helpers[ct_alg_ctl](ct, ctx, pkt, conn_for_expectation, now,
> CT_FTP_CTL_INTEREST, nat);
> }
> @@ -871,15 +871,14 @@ conn_not_found(struct conntrack *ct, struct dp_packet
> *pkt,
> return nc;
> }
>
> - if (!ct_verify_helper(helper, ct_alg_ctl)) {
> - return nc;
> - }
> -
> nc = new_conn(&ct->buckets[bucket], pkt, &ctx->key, now);
> ctx->conn = nc;
> nc->rev_key = nc->key;
> conn_key_reverse(&nc->rev_key);
> - nc->alg = nullable_xstrdup(helper);
> +
> + if (ct_verify_helper(helper, ct_alg_ctl)) {
> + nc->alg = nullable_xstrdup(helper);
> + }
>
> if (alg_exp) {
> nc->alg_related = true;
>
>
> This deserves a test case and I created one, which I will submit with this
> patch
>
> Darrell
>
>
> On 11/22/17, 12:56 AM, "ovs-dev-boun...@openvswitch.org on behalf of Darrell
> Ball" <ovs-dev-boun...@openvswitch.org on behalf of dlu...@gmail.com> wrote:
>
> Presently, alg processing is enabled by default to better exercise code.
> This is similar to kernels before 4.7 as well. The recommended default
> behavior in the newer kernels 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>
> ---
> lib/conntrack.c | 35 +++++++++++++++++++++++++++++------
> 1 file changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 1364d05..fed9f61 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -819,6 +819,26 @@ conn_clean(struct conntrack *ct, struct conn *conn,
> }
> }
>
> +static bool
> +ct_verify_helper(const char *helper, enum ct_alg_ctl_type ct_alg_ctl)
> +{
> + 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,
> @@ -826,7 +846,8 @@ conn_not_found(struct conntrack *ct, struct dp_packet
> *pkt,
> 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 struct alg_exp_node *alg_exp,
> + enum ct_alg_ctl_type ct_alg_ctl)
> {
> unsigned bucket = hash_to_bucket(ctx->hash);
> struct conn *nc = NULL;
> @@ -850,14 +871,15 @@ conn_not_found(struct conntrack *ct, struct
> dp_packet *pkt,
> return nc;
> }
>
> + if (!ct_verify_helper(helper, ct_alg_ctl)) {
> + return nc;
> + }
> +
> 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;
> @@ -1222,7 +1244,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);
> --
> 1.9.1
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
>
> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=qUmTMHhyVHPtuz5GpCtZ3IYp3s24O7aZrny9l8fX_Mw&s=7SVc_wIlopFlXNnAzI-FXKfbTLYYc74mEltEvCZzjQE&e=
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
