From: Paolo Valerio <[email protected]>
Date: 2021-11-17 23:09:29
To: [email protected],[email protected]
Cc: [email protected]
Subject: Re: [PATCH v6 2/3] conntrack: split the dst and src port range
iterations>Hi wenxu,
>
>[email protected] writes:
>
>> From: wenxu <[email protected]>
>>
>> Splitting the two port range iterations instead of keeping it nested. And
>> the dst port (in case of DNAT) range would have precedence over the src
>> manipulation in the resolution.
>>
>> Signed-off-by: wenxu <[email protected]>
>> ---
>> lib/conntrack.c | 65
>> +++++++++++++++++++++++++++------------------------------
>> 1 file changed, 31 insertions(+), 34 deletions(-)
>>
>> diff --git a/lib/conntrack.c b/lib/conntrack.c
>> index 44f99f3..a6784ba 100644
>> --- a/lib/conntrack.c
>> +++ b/lib/conntrack.c
>> @@ -1036,6 +1036,17 @@ conn_not_found(struct conntrack *ct, struct dp_packet
>> *pkt,
>> nat_action_info);
>>
>> if (!nat_res) {
>> + if (nat_action_info->nat_action & NAT_ACTION_DST) {
>> + struct nat_action_info_t tmp_nat_info;
>> +
>> + memset(&tmp_nat_info, 0, sizeof tmp_nat_info);
>> + tmp_nat_info.nat_action = NAT_ACTION_SRC;
>> + nat_res = nat_get_unique_tuple(ct, nc, nat_conn,
>> + &tmp_nat_info);
>> + if (!nat_res) {
>> + goto nat_res_exhaustion;
>> + }
>> + }
>> goto nat_res_exhaustion;
>> }
>
>is there any particular reason that brought you to call
>nat_get_unique_tuple() twice instead of splitting the FOR_EACH with a
>single call of nat_get_unique_tuple() ?
>
>Something like:
>
>FOR_EACH_PORT_IN_RANGE(curr_dport, min_dport, max_dport) {
> ...
>}
>
>FOR_EACH_PORT_IN_RANGE(curr_sport, min_sport, max_sport) {
> ...
>}
>
>That was my initial understanding. I suspect that approach would require
>fewer changes as well.
And I also think this splitting the FOR_EACH twice method can't handle the both
dst port
and src port nat case. For eample: do dnat case dport range {100..200} and
sport {MIN_NAT_EPHEMERAL_PORT..
MAX_NAT_EPHEMERAL_PORT } . This shuld be chain each FOR-EACH together but not
separate it.
>
>>
>> @@ -2258,12 +2269,12 @@ nat_range_hash(const struct conn *conn, uint32_t
>> basis,
>>
>> /* Ports are stored in host byte order for convenience. */
>> static void
>> -set_sport_range(const struct nat_action_info_t *ni, const struct conn_key
>> *k,
>> - uint32_t hash, uint16_t *curr, uint16_t *min,
>> - uint16_t *max)
>> +set_port_range(const struct nat_action_info_t *ni, const struct conn_key *k,
>> + uint32_t hash, uint16_t *curr, uint16_t *min,
>> + uint16_t *max)
>> {
>> - if (((ni->nat_action & NAT_ACTION_SNAT_ALL) == NAT_ACTION_SRC) ||
>> - ((ni->nat_action & NAT_ACTION_DST))) {
>> + if ((ni->nat_action & NAT_ACTION_SRC) &&
>> + (!(ni->nat_action & NAT_ACTION_SRC_PORT))) {
>> *curr = ntohs(k->src.port);
>> if (*curr < 512) {
>> *min = 1;
>> @@ -2275,6 +2286,10 @@ set_sport_range(const struct nat_action_info_t *ni,
>> const struct conn_key *k,
>> *min = MIN_NAT_EPHEMERAL_PORT;
>> *max = MAX_NAT_EPHEMERAL_PORT;
>> }
>> + } else if ((ni->nat_action & NAT_ACTION_DST) &&
>> + (!(ni->nat_action & NAT_ACTION_DST_PORT))) {
>> + *curr = ntohs(k->dst.port);
>> + *min = *max = *curr;
>> } else {
>> *min = ni->min_port;
>> *max = ni->max_port;
>> @@ -2282,21 +2297,6 @@ set_sport_range(const struct nat_action_info_t *ni,
>> const struct conn_key *k,
>> }
>> }
>>
>> -static void
>> -set_dport_range(const struct nat_action_info_t *ni, const struct conn_key
>> *k,
>> - uint32_t hash, uint16_t *curr, uint16_t *min,
>> - uint16_t *max)
>> -{
>> - if (ni->nat_action & NAT_ACTION_DST_PORT) {
>> - *min = ni->min_port;
>> - *max = ni->max_port;
>> - *curr = *min + (hash % ((*max - *min) + 1));
>> - } else {
>> - *curr = ntohs(k->dst.port);
>> - *min = *max = *curr;
>> - }
>> -}
>> -
>> /* Gets the initial in range address based on the hash.
>> * Addresses are kept in network order. */
>> static void
>> @@ -2426,8 +2426,7 @@ nat_get_unique_tuple(struct conntrack *ct, const
>> struct conn *conn,
>> uint32_t hash = nat_range_hash(conn, ct->hash_basis, nat_info);
>> bool pat_proto = conn->key.nw_proto == IPPROTO_TCP ||
>> conn->key.nw_proto == IPPROTO_UDP;
>> - uint16_t min_dport, max_dport, curr_dport;
>> - uint16_t min_sport, max_sport, curr_sport;
>> + uint16_t min_port, max_port, curr_port;
>>
>> min_addr = nat_info->min_addr;
>> max_addr = nat_info->max_addr;
>> @@ -2439,10 +2438,8 @@ nat_get_unique_tuple(struct conntrack *ct, const
>> struct conn *conn,
>> * we can stop once we reach it. */
>> guard_addr = curr_addr;
>>
>> - set_sport_range(nat_info, &conn->key, hash, &curr_sport,
>> - &min_sport, &max_sport);
>> - set_dport_range(nat_info, &conn->key, hash, &curr_dport,
>> - &min_dport, &max_dport);
>> + set_port_range(nat_info, &conn->key, hash, &curr_port,
>> + &min_port, &max_port);
>>
>> another_round:
>> store_addr_to_key(&curr_addr, &nat_conn->rev_key,
>> @@ -2457,14 +2454,14 @@ another_round:
>> goto next_addr;
>> }
>>
>> - FOR_EACH_PORT_IN_RANGE(curr_dport, min_dport, max_dport) {
>> - nat_conn->rev_key.src.port = htons(curr_dport);
>> - FOR_EACH_PORT_IN_RANGE(curr_sport, min_sport, max_sport) {
>> - nat_conn->rev_key.dst.port = htons(curr_sport);
>> - if (!conn_lookup(ct, &nat_conn->rev_key,
>> - time_msec(), NULL, NULL)) {
>> - return true;
>> - }
>> + FOR_EACH_PORT_IN_RANGE(curr_port, min_port, max_port) {
>> + if (nat_info->nat_action & NAT_ACTION_SRC) {
>> + nat_conn->rev_key.dst.port = htons(curr_port);
>> + } else {
>> + nat_conn->rev_key.src.port = htons(curr_port);
>> + }
>> + if (!conn_lookup(ct, &nat_conn->rev_key, time_msec(), NULL, NULL)) {
>> + return true;
>> }
>> }
>>
>> --
>> 1.8.3.1
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
