Message ID | 1630489288-3526-1-git-send-email-wenxu@ucloud.cn |
---|---|
State | Superseded |
Headers | show |
Series | [ovs-dev,v2,1/2] conntrack: restore the origin port for each round with new address | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
ovsrobot/apply-robot | fail | apply and check: fail |
Hello, wenxu@ucloud.cn writes: > From: wenxu <wenxu@ucloud.cn> > > It is better to choose the origin select port as current port > for each port search round with new address. > > Signed-off-by: wenxu <wenxu@ucloud.cn> > --- This should happen normally. It doesn't happen in the case of source port manipulation when the default ephemeral range is used and the packet source port is below 1024. In that case the first IP iteration uses the packet source port, whereas the others don't. if we want to change this behavior, there are some more ways we can consider, e.g.: - Using 1024 as initial curr_sport for source ports < 1024. - picking different default ranges - e.g. the kernel uses the range [1, 511], if the source port is < 512, [600, 1023], if it's >= 512 and < 1024, and [1024,65535] for the remaining ports. What do you think if we do something like the third bullet and squash this change, if needed, in your next patch? In any case, there are a couple of small nits/questions, see inline. > lib/conntrack.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 551c206..2d14205 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -2412,8 +2412,8 @@ nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn, > uint32_t hash = nat_range_hash(conn, ct->hash_basis); > 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_dport, max_dport, curr_dport, orig_dport; > + uint16_t min_sport, max_sport, curr_sport, orig_sport; can we keep the reverse xmas tree style here? > > min_addr = conn->nat_info->min_addr; > max_addr = conn->nat_info->max_addr; > @@ -2425,9 +2425,9 @@ 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(conn->nat_info, &conn->key, hash, &curr_sport, > + set_sport_range(conn->nat_info, &conn->key, hash, &orig_sport, > &min_sport, &max_sport); > - set_dport_range(conn->nat_info, &conn->key, hash, &curr_dport, > + set_dport_range(conn->nat_info, &conn->key, hash, &orig_dport, > &min_dport, &max_dport); > > another_round: > @@ -2443,6 +2443,9 @@ another_round: > goto next_addr; > } > > + curr_sport = orig_sport; > + curr_dport = orig_dport; > + can this happen with dport? > 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) { > -- > 1.8.3.1
From: Paolo Valerio <pvalerio@redhat.com> Date: 2021-09-06 19:00:37 To: wenxu@ucloud.cn,i.maximets@ovn.org,dlu998@gmail.com,aconole@redhat.com Cc: dev@openvswitch.org Subject: Re: [PATCH v2 1/2] conntrack: restore the origin port for each round with new address>Hello, > >wenxu@ucloud.cn writes: > >> From: wenxu <wenxu@ucloud.cn> >> >> It is better to choose the origin select port as current port >> for each port search round with new address. >> >> Signed-off-by: wenxu <wenxu@ucloud.cn> >> --- > >This should happen normally. >It doesn't happen in the case of source port manipulation when the >default ephemeral range is used and the packet source port is below >1024. In that case the first IP iteration uses the packet source port, >whereas the others don't. > >if we want to change this behavior, there are some more ways we can >consider, e.g.: This can be done for avoding keep old_sport for source ports < 1024 case. > >- Using 1024 as initial curr_sport for source ports < 1024. >- picking different default ranges > - e.g. the kernel uses the range [1, 511], if the source port is < 512, > [600, 1023], if it's >= 512 and < 1024, and [1024,65535] for the > remaining ports. > >What do you think if we do something like the third bullet and squash >this change, if needed, in your next patch? > >In any case, there are a couple of small nits/questions, see inline. > >> lib/conntrack.c | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/lib/conntrack.c b/lib/conntrack.c >> index 551c206..2d14205 100644 >> --- a/lib/conntrack.c >> +++ b/lib/conntrack.c >> @@ -2412,8 +2412,8 @@ nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn, >> uint32_t hash = nat_range_hash(conn, ct->hash_basis); >> 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_dport, max_dport, curr_dport, orig_dport; >> + uint16_t min_sport, max_sport, curr_sport, orig_sport; > >can we keep the reverse xmas tree style here? > >> >> min_addr = conn->nat_info->min_addr; >> max_addr = conn->nat_info->max_addr; >> @@ -2425,9 +2425,9 @@ 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(conn->nat_info, &conn->key, hash, &curr_sport, >> + set_sport_range(conn->nat_info, &conn->key, hash, &orig_sport, >> &min_sport, &max_sport); >> - set_dport_range(conn->nat_info, &conn->key, hash, &curr_dport, >> + set_dport_range(conn->nat_info, &conn->key, hash, &orig_dport, >> &min_dport, &max_dport); >> >> another_round: >> @@ -2443,6 +2443,9 @@ another_round: >> goto next_addr; >> } >> >> + curr_sport = orig_sport; >> + curr_dport = orig_dport; >> + > >can this happen with dport? > >> 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) { >> -- >> 1.8.3.1 >
From: Paolo Valerio <pvalerio@redhat.com> Date: 2021-09-06 19:00:37 To: wenxu@ucloud.cn,i.maximets@ovn.org,dlu998@gmail.com,aconole@redhat.com Cc: dev@openvswitch.org Subject: Re: [PATCH v2 1/2] conntrack: restore the origin port for each round with new address>Hello, > >wenxu@ucloud.cn writes: > >> From: wenxu <wenxu@ucloud.cn> >> >> It is better to choose the origin select port as current port >> for each port search round with new address. >> >> Signed-off-by: wenxu <wenxu@ucloud.cn> >> --- > >This should happen normally. >It doesn't happen in the case of source port manipulation when the >default ephemeral range is used and the packet source port is below >1024. In that case the first IP iteration uses the packet source port, >whereas the others don't. > >if we want to change this behavior, there are some more ways we can >consider, e.g.: " In that case the first IP iteration uses the packet source port, whereas the others don't" I think the above rule is not matter with the curr_sport picking different ranges? So you means for source ports < 1024, each IP iteration should using different source port? > >- Using 1024 as initial curr_sport for source ports < 1024. >- picking different default ranges > - e.g. the kernel uses the range [1, 511], if the source port is < 512, > [600, 1023], if it's >= 512 and < 1024, and [1024,65535] for the > remaining ports. > >What do you think if we do something like the third bullet and squash >this change, if needed, in your next patch? > >In any case, there are a couple of small nits/questions, see inline. > >> lib/conntrack.c | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/lib/conntrack.c b/lib/conntrack.c >> index 551c206..2d14205 100644 >> --- a/lib/conntrack.c >> +++ b/lib/conntrack.c >> @@ -2412,8 +2412,8 @@ nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn, >> uint32_t hash = nat_range_hash(conn, ct->hash_basis); >> 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_sp ort, curr_sport; >> + uint16_t min_dport, max_dport, curr_dport, orig_dport; >> + uint16_t min_sport, max_sport, curr_sport, orig_sport; > >can we keep the reverse xmas tree style here? > >> >> min_addr = conn->nat_info->min_addr; >> max_addr = conn->nat_info->max_addr; >> @@ -2425,9 +2425,9 @@ 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(conn->nat_info, &conn->key, hash, &curr_sport, >> + set_sport_range(conn->nat_info, &conn->key, hash, &orig_sport, >> &min_sport, &max_sport); >> - set_dport_range(conn->nat_info, &conn->key, hash, &curr_dport, >> + set_dport_range(conn->nat_info, &conn->key, hash, &orig_dport, >> &min_dport, &max_dport); >> >> another_round: >> @@ -2443,6 +2443,9 @@ another_round: >> goto next_addr; >> } >> >> + curr_sport = orig_sport; >> + curr_dport = orig_dport; >> + > >can this happen with dport? > >> FOR_EACH_PORT_IN_RANGE(curr_dport, min_dport, ma x_dport) { >> nat_conn->rev_key.src.port = htons(curr_dport); >> FOR_EACH_PORT_IN_RANGE(curr_sport, min_sport, max_sport) { >> -- >> 1.8.3.1 >
wenxu <wenxu@ucloud.cn> writes: > From: Paolo Valerio <pvalerio@redhat.com> > Date: 2021-09-06 19:00:37 > To: wenxu@ucloud.cn,i.maximets@ovn.org,dlu998@gmail.com,aconole@redhat.com > Cc: dev@openvswitch.org > Subject: Re: [PATCH v2 1/2] conntrack: restore the origin port for each round with new address>Hello, >> >>wenxu@ucloud.cn writes: >> >>> From: wenxu <wenxu@ucloud.cn> >>> >>> It is better to choose the origin select port as current port >>> for each port search round with new address. >>> >>> Signed-off-by: wenxu <wenxu@ucloud.cn> >>> --- >> >>This should happen normally. >>It doesn't happen in the case of source port manipulation when the >>default ephemeral range is used and the packet source port is below >>1024. In that case the first IP iteration uses the packet source port, >>whereas the others don't. >> >>if we want to change this behavior, there are some more ways we can >>consider, e.g.: > > " In that case the first IP iteration uses the packet source port, whereas the others don't" > > I think the above rule is not matter with the curr_sport picking different ranges? > > So you means for source ports < 1024, each IP iteration should using different source port? > What I meant is that, without your patch, in that case, the first attempt of the first IP iteration is performed using the original source port. The next IP iterations use MIN_NAT_EPHEMERAL_PORT as a starting port. E.g. with ct(nat(commit,src=10.1.1.240-10.1.1.241)), packet src port 500, and all the lookups detect a collision, results in: curr IP: 10.1.1.240, curr source port: 500, 1024, 1025, ..., MAX_NAT_EPHEMERAL_PORT curr IP: 10.1.1.241, curr source port: 1024, 1025, ..., MAX_NAT_EPHEMERAL_PORT
wenxu@ucloud.cn writes: > From: wenxu <wenxu@ucloud.cn> > > It is better to choose the origin select port as current port > for each port search round with new address. > > Signed-off-by: wenxu <wenxu@ucloud.cn> > --- Hi Wenxu, Paolo has done a good job reviewing, so I won't look too much at the code, but I think we might want to include a test or two in the system-traffic.at file that can catch these NAT collision cases. WDYT? > lib/conntrack.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 551c206..2d14205 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -2412,8 +2412,8 @@ nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn, > uint32_t hash = nat_range_hash(conn, ct->hash_basis); > 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_dport, max_dport, curr_dport, orig_dport; > + uint16_t min_sport, max_sport, curr_sport, orig_sport; > > min_addr = conn->nat_info->min_addr; > max_addr = conn->nat_info->max_addr; > @@ -2425,9 +2425,9 @@ 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(conn->nat_info, &conn->key, hash, &curr_sport, > + set_sport_range(conn->nat_info, &conn->key, hash, &orig_sport, > &min_sport, &max_sport); > - set_dport_range(conn->nat_info, &conn->key, hash, &curr_dport, > + set_dport_range(conn->nat_info, &conn->key, hash, &orig_dport, > &min_dport, &max_dport); > > another_round: > @@ -2443,6 +2443,9 @@ another_round: > goto next_addr; > } > > + curr_sport = orig_sport; > + curr_dport = orig_dport; > + > 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) {
From: Aaron Conole <aconole@redhat.com> Date: 2021-09-07 21:46:29 To: wenxu@ucloud.cn Cc: i.maximets@ovn.org,dlu998@gmail.com,pvalerio@redhat.com,dev@openvswitch.org Subject: Re: [PATCH v2 1/2] conntrack: restore the origin port for each round with new address>wenxu@ucloud.cn writes: > >> From: wenxu <wenxu@ucloud.cn> >> >> It is better to choose the origin select port as current port >> for each port search round with new address. >> >> Signed-off-by: wenxu <wenxu@ucloud.cn> >> --- > >Hi Wenxu, > >Paolo has done a good job reviewing, so I won't look too much at the >code, but I think we might want to include a test or two in the >system-traffic.at file that can catch these NAT collision cases. WDYT? This patch does not resolve the collison case. Only optimazation the first src port selection. And I think nat collision case is not easy to setup through some little test. > >> lib/conntrack.c | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/lib/conntrack.c b/lib/conntrack.c >> index 551c206..2d14205 100644 >> --- a/lib/conntrack.c >> +++ b/lib/conntrack.c >> @@ -2412,8 +2412,8 @@ nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn, >> uint32_t hash = nat_range_hash(conn, ct->hash_basis); >> 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_dport, max_dport, curr_dport, orig_dport; >> + uint16_t min_sport, max_sport, curr_sport, orig_sport; >> >> min_addr = conn->nat_info->min_addr; >> max_addr = conn->nat_info->max_addr; >> @@ -2425,9 +2425,9 @@ 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(conn->nat_info, &conn->key, hash, &curr_sport, >> + set_sport_range(conn->nat_info, &conn->key, hash, &orig_sport, >> &min_sport, &max_sport); >> - set_dport_range(conn->nat_info, &conn->key, hash, &curr_dport, >> + set_dport_range(conn->nat_info, &conn->key, hash, &orig_dport, >> &min_dport, &max_dport); >> >> another_round: >> @@ -2443,6 +2443,9 @@ another_round: >> goto next_addr; >> } >> >> + curr_sport = orig_sport; >> + curr_dport = orig_dport; >> + >> 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) { >
Bleep bloop. Greetings wenxu, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. git-am: error: Failed to merge in the changes. hint: Use 'git am --show-current-patch' to see the failed patch Patch failed at 0001 conntrack: restore the origin port for each round with new address When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Patch skipped due to previous failure. Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
diff --git a/lib/conntrack.c b/lib/conntrack.c index 551c206..2d14205 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -2412,8 +2412,8 @@ nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn, uint32_t hash = nat_range_hash(conn, ct->hash_basis); 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_dport, max_dport, curr_dport, orig_dport; + uint16_t min_sport, max_sport, curr_sport, orig_sport; min_addr = conn->nat_info->min_addr; max_addr = conn->nat_info->max_addr; @@ -2425,9 +2425,9 @@ 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(conn->nat_info, &conn->key, hash, &curr_sport, + set_sport_range(conn->nat_info, &conn->key, hash, &orig_sport, &min_sport, &max_sport); - set_dport_range(conn->nat_info, &conn->key, hash, &curr_dport, + set_dport_range(conn->nat_info, &conn->key, hash, &orig_dport, &min_dport, &max_dport); another_round: @@ -2443,6 +2443,9 @@ another_round: goto next_addr; } + curr_sport = orig_sport; + curr_dport = orig_dport; + 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) {