On Fri, Jan 11, 2019 at 8:05 PM Darrell Ball <dlu...@gmail.com> wrote:
> On Thu, Jan 10, 2019 at 12:58 PM Darrell Ball <dlu...@gmail.com> wrote:
>
>> On Thu, Jan 10, 2019 at 1:03 AM David Marchand <david.march...@redhat.com>
>> wrote:
>>
>>> Hello,
>>>
>>> Just wondering, can't we rely integer promotion then truncation on
>>> 32bits ?
>>> My test program tends to show it works.
>>>
>>
>> I believe a compiler is free to convert the operands to int64 and AFAIK
>> overflow/underflow is undefined for signed variables.
>>
>
> nevermind; it appears we can trust the compiler in this case and simplify.
>
Ok, I had missed the int64_t for seq_skew.
More than trusting the compiler, I understand we only want to deal with
32bits integers, since, at the end of the game, we want any offset bigger
than this to wrap on the 32bits range.
If we change it to int32_t, we are back with only 32bits integers: signed
for the seq_skew variable, and unsigned for the seq/ack values => addition
and substraction gives a uint32_t thanks to the "Usual arithmetic
conversions" ?
diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
index a344801..c261f80 100644
--- a/lib/conntrack-private.h
+++ b/lib/conntrack-private.h
@@ -99,7 +99,7 @@ struct conn {
/* XXX: consider flattening. */
struct nat_action_info_t *nat_info;
char *alg;
- int seq_skew;
+ int32_t seq_skew;
uint32_t mark;
uint8_t conn_type;
/* TCP sequence skew due to NATTing of FTP control messages. */
diff --git a/lib/conntrack.c b/lib/conntrack.c
index 6f6021a..206f5f1 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -756,7 +756,7 @@ conn_lookup(struct conntrack *ct, const struct conn_key
*key, long long now)
static void
conn_seq_skew_set(struct conntrack *ct, const struct conn_key *key,
- long long now, int seq_skew, bool seq_skew_dir)
+ long long now, int32_t seq_skew, bool seq_skew_dir)
{
unsigned bucket = hash_to_bucket(conn_key_hash(key, ct->hash_basis));
ct_lock_lock(&ct->buckets[bucket].lock);
@@ -3192,7 +3192,7 @@ handle_ftp_ctl(struct conntrack *ct, const struct
conn_lookup_ctx *ctx,
}
struct ovs_16aligned_ip6_hdr *nh6 = dp_packet_l3(pkt);
- int64_t seq_skew = 0;
+ int32_t seq_skew = 0;
if (ftp_ctl == CT_FTP_CTL_OTHER) {
seq_skew = conn_for_expectation->seq_skew;
@@ -3251,31 +3251,13 @@ handle_ftp_ctl(struct conntrack *ct, const struct
conn_lookup_ctx *ctx,
if (do_seq_skew_adj && seq_skew != 0) {
if (ctx->reply != conn_for_expectation->seq_skew_dir) {
-
uint32_t tcp_ack = ntohl(get_16aligned_be32(&th->tcp_ack));
- if ((seq_skew > 0) && (tcp_ack < seq_skew)) {
- /* Should not be possible; will be marked invalid. */
- tcp_ack = 0;
- } else if ((seq_skew < 0) && (UINT32_MAX - tcp_ack <
-seq_skew)) {
- tcp_ack = (-seq_skew) - (UINT32_MAX - tcp_ack);
- } else {
- tcp_ack -= seq_skew;
- }
- ovs_be32 new_tcp_ack = htonl(tcp_ack);
- put_16aligned_be32(&th->tcp_ack, new_tcp_ack);
+ put_16aligned_be32(&th->tcp_ack, htonl(tcp_ack - seq_skew));
} else {
uint32_t tcp_seq = ntohl(get_16aligned_be32(&th->tcp_seq));
- if ((seq_skew > 0) && (UINT32_MAX - tcp_seq < seq_skew)) {
- tcp_seq = seq_skew - (UINT32_MAX - tcp_seq);
- } else if ((seq_skew < 0) && (tcp_seq < -seq_skew)) {
- /* Should not be possible; will be marked invalid. */
- tcp_seq = 0;
- } else {
- tcp_seq += seq_skew;
- }
- ovs_be32 new_tcp_seq = htonl(tcp_seq);
- put_16aligned_be32(&th->tcp_seq, new_tcp_seq);
+
+ put_16aligned_be32(&th->tcp_seq, htonl(tcp_seq + seq_skew));
}
}
What do you think ?
Do you want me to send a patch ? (I could take it as part of my other fixes)
--
David Marchand
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
