[ovs-dev] [PATCH v5] conntrack: handle SNAT with all-zero IP address

Paolo Valerio pvalerio at redhat.com
Tue Apr 27 19:55:44 UTC 2021


Gaëtan Rivet <grive at u256.net> writes:

> (Adding back the mailing list + original CCes to the thread.)
>

something weird happened, cause patchwork got it:

https://patchwork.ozlabs.org/project/openvswitch/patch/161912956184.60963.6344811329504822258.stgit@fed.void/#2672584

not a big deal, I'll check my side.

> On Mon, Apr 26, 2021, at 19:09, Paolo Valerio wrote:
>> Hello Gaetan,
>> 
>> thanks for the feedback
>> 
>> Gaëtan Rivet <grive at u256.net> writes:
>> 
>> > On Fri, Apr 23, 2021, at 00:28, Paolo Valerio wrote:
>> 
>> [...]
>> 
>> >> +
>> >> +    int i, j, s_attempts, d_attempts;
>> >
>> > Why not use uint16_t here?
>> > {curr,min,max}_{d,s}port are uint16_t and {s,d}_attemps will be set to values derived from them.
>> > i and j will then be compared against {s,d}_attempts, so it seems safer to keep them all aligned.
>> >
>> 
>> ACK
>> 
>> > Additionally, it seems s,d_attempts are unnecessary.
>> > They are only used to know the number of NEXT_PORT_IN_RANGE() that should be attempted.
>> > Their names are slightly misleading (if they are counts of attempts, n_attempts would be clearer),
>> > but also the index could be initialized to the number of attempts remaining, and decrease during the loop.
>> > As the indexes are not useful within the loop, it seems ok.
>> >
>> > Furthermore, if they are not useful, could the indexes be masked completely? Would it be acceptable
>> > to declare them within the for() loop? I know it's should generally be avoided, but I've seen a few places
>> > where in-line declaration were used. In that case I think it's justified if it makes the macro safer to use and simpler
>> > to read.
>> >
>> 
>> Right, the indexes are not useful within the loop, and masking them
>> would make the macro simpler. OTOH, declaring them within the for()
>> and nesting the loops would lead to a warning (-Wshadow).
>> 
>> If I didn't miss anything, and if you are ok with it, I would change it,
>> based on your suggestions, like the following:
>> 
>> uint16_t i, j;
>> FOR_EACH_PORT_IN_RANGE(i, curr_dport, min_dport, max_dport) {
>>     nat_conn->rev_key.src.port = htons(curr_dport);
>>     FOR_EACH_PORT_IN_RANGE(j, curr_sport, min_sport, max_sport) {
>>         [...]
>>     }
>> }
>> 
>> #define FOR_EACH_PORT_IN_RANGE(idx, curr, min, max) \
>>     for (INIT_N_PORT_ATTEMPTS(idx, curr, min, max); \
>>          idx > 0; idx--, NEXT_PORT_IN_RANGE(curr, min, max))
>> 
>> WDYT?
>> 
>
> To nest the loops, you can use the __COUNTER__ macro, like so:
>
> /* Generate a unique name with the __COUNTER__ macro to allow nesting loops. */
> #define OVS_STR_(x,y) x##y
> #define OVS_STR(x, y) OVS_STR_(x,y)
> /* There is one such 'stringify' macro in cmap.h as well, maybe it could be shared in a util.h or similar. */
>

Ok, I'll share the macro in util.h.
I would split the patch in this case.

> #define FOR_EACH_PORT_IN_RANGE__(curr, min, max, INAME) \
>     for (uint16_t INAME = N_PORT_ATTEMPTS(curr, min, max); \
>           INAME > 0; INAME--, NEXT_PORT_IN_RANGE(curr, min, max))
>
> #define FOR_EACH_PORT_IN_RANGE(curr, min, max) \
>     FOR_EACH_PORT_IN_RANGE__(curr, min, max, OVS_STR(idx, __COUNTER__))
>
>> >> +    FOR_EACH_PORT_IN_RANGE(i, d_attempts, curr_dport, min_dport, 
>> >> max_dport) {
>> >> +        nat_conn->rev_key.src.port = htons(curr_dport);
>> >> +        FOR_EACH_PORT_IN_RANGE(j, s_attempts, 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;
>> >>              }
>> >> -            first_port = min_port;
>> >> -            port = first_port;
>> >> -            all_ports_tried = false;
>> >>          }
>> >>      }
>> >> -    return false;
>> >> +
>> >> +    /* Check if next IP is in range and respin. Otherwise, notify
>> >> +     * exhaustion to the caller. */
>> >> +next_addr:
>> >> +    if (next_addr_in_range_guarded(&curr_addr, &min_addr,
>> >> +                                   &max_addr, &guard_addr,
>> >> +                                   conn->key.dl_type == 
>> >> htons(ETH_TYPE_IP))) {
>> >> +        return false;
>> >> +    }
>> >> +
>> >> +    goto another_round;
>> >>  }
>> >>  
>> >>  static enum ct_update_res
>> >> diff --git a/lib/conntrack.h b/lib/conntrack.h
>> >> index 9553b188a..c68a83ccd 100644
>> >> --- a/lib/conntrack.h
>> >> +++ b/lib/conntrack.h
>> >> @@ -77,6 +77,14 @@ enum nat_action_e {
>> >>      NAT_ACTION_DST_PORT = 1 << 3,
>> >>  };
>> >>  
>> >> +#define NAT_ACTION_SNAT_ALL (NAT_ACTION_SRC | NAT_ACTION_SRC_PORT)
>> >> +#define NAT_ACTION_DNAT_ALL (NAT_ACTION_DST | NAT_ACTION_DST_PORT)
>> >> +
>> >> +enum {
>> >> +    MIN_NAT_EPHEMERAL_PORT = 1024,
>> >> +    MAX_NAT_EPHEMERAL_PORT = 65535
>> >> +};
>> >> +
>> >>  struct nat_action_info_t {
>> >>      union ct_addr min_addr;
>> >>      union ct_addr max_addr;
>> >> @@ -85,6 +93,28 @@ struct nat_action_info_t {
>> >>      uint16_t nat_action;
>> >>  };
>> >>  
>> >> +#define IN_RANGE(curr, min, max) \
>> >> +    (curr >= min && curr <= max)
>> >> +
>> >> +#define NEXT_PORT_IN_RANGE(curr, min, max) \
>> >> +    curr = (!IN_RANGE(curr, min, max) || curr == max) ? min : curr + 1
>> >> +
>> >> +/* if the current port is out of range increase the attempts by
>> >> + * one so that in the worst case scenario the current out of
>> >> + * range port plus all the in-range ports get tested.
>> >> + * Note that curr can be an out of range port only in case of
>> >> + * source port (SNAT with port range unspecified or DNAT),
>> >> + * furthermore the source port in the packet has to be less than
>> >> + * MIN_NAT_EPHEMERAL_PORT. */
>> >> +#define INIT_ATT(att, curr, min, max) \
>> >
>> > INIT_ATT is rather unclear about what the macro does.
>> > It is then used only once, it would seem worth it to be more explicit.
>> >
>> > Maybe INIT_N_PORT_ATTEMPTS?
>> >
>> 
>> ACK, it's a better name
>> 
>
> INIT_N_PORT_ATTEMPTS implies that the initialization is done.
> In the suggestion above I changed to N_PORT_ATTEMPTS, if the assignment happens
> outside the macro.

yes, makes sense.

Thanks,
Paolo



More information about the dev mailing list