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

[Gaëtan Rivet]

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

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. */

#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.

-- 
Gaetan Rivet