On 2/4/19, 8:53 AM, "[email protected] on behalf of Aaron
Conole" <[email protected] on behalf of [email protected]> wrote:
Darrell Ball <[email protected]> writes:
> There are a few cases where padding may be undefined according to
> the C standard. Practically, it seems implementations don't have issue,
> but it is better to be safe. The code paths modified are not hot ones.
> Found by inspection.
Better to be safe how? What do implementations not have issue with?
I'm having trouble understanding this commit message as a rationale for
the change.
The conn keys are used in a hash calculation
> Signed-off-by: Darrell Ball <[email protected]>
> ---
>
> v2: Found another one; will double check for others later.
Were there actual problems observed somewhere? From what I can see,
none of these accesses did any kind of bit-wise structure memcmp().
Rather, I see use of named fields when doing comparisons. This tells me
that the pad bits should be irrelevant. Probably I missed something.
OTOH, I guess you might also be interested in fixing:
'struct ct_addr first_addr = ct_addr;' on line 2181 of conntrack.c
Existing structure copy should work
and the 'process_ftp_ctl_v6' function. Those do possibly use memcmp()
with indeterminate pad bits, if I'm reading them correctly (but just
cursory reading).
Existing structure copy and the memset zero should work.
> lib/conntrack.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index e1f4041..e7033a8 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -747,6 +747,7 @@ static struct conn *
> conn_lookup(struct conntrack *ct, const struct conn_key *key, long long
now)
> {
> struct conn_lookup_ctx ctx;
> + memset(&ctx, 0, sizeof ctx);
> ctx.conn = NULL;
> ctx.key = *key;
> ctx.hash = conn_key_hash(key, ct->hash_basis);
How does this help? Looking at conn_key_hash it seems to only use those
defined sections of the struct (ie: it doesn't walk the raw memory, it
pulls the fields it needs by assignment). conn_key_cmp also compares
named fields.
Again, For the hash; conn_key has padding
> @@ -896,6 +897,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet
*pkt,
>
> if (nat_action_info) {
> nc->nat_info = xmemdup(nat_action_info, sizeof
*nc->nat_info);
> + memset(conn_for_un_nat_copy, 0, sizeof
*conn_for_un_nat_copy);
Was there an observable bug related to the pad bits here?
No, it is just adhering to C standard
Implementations are 'very unlikely' to have issue.
I didn't walk this path deep enough, but in conntrack.c I didn't see any
related
memcmp() calls.
Again, for the hash later
> if (alg_exp) {
> if (alg_exp->nat_rpl_dst) {
> @@ -934,8 +936,6 @@ conn_not_found(struct conntrack *ct, struct dp_packet
*pkt,
> ct_rwlock_unlock(&ct->resources_lock);
> }
> conn_for_un_nat_copy->conn_type = CT_CONN_TYPE_UN_NAT;
> - conn_for_un_nat_copy->nat_info = NULL;
> - conn_for_un_nat_copy->alg = NULL;
If this was needed here before, isn't it still needed now? There are a
interrim assignments from 'nc', and I think that won't be changed by
doing a memset() (ie: does this still cause a double free if you drop it?).
Not related to double free
Oops, this is still needed in present code base
> nat_packet(pkt, nc, ctx->icmp_related);
> }
> hmap_insert(&ct->buckets[bucket].connections, &nc->node,
ctx->hash);
> @@ -1262,6 +1262,7 @@ process_one(struct conntrack *ct, struct dp_packet
*pkt,
> ct->hash_basis,
> alg_src_ip_wc(ct_alg_ctl));
> if (alg_exp) {
> + memset(&alg_exp_entry, 0, sizeof alg_exp_entry);
> alg_exp_entry = *alg_exp;
> alg_exp = &alg_exp_entry;
> }
> @@ -2024,7 +2025,9 @@ conn_key_hash(const struct conn_key *key, uint32_t
basis)
> static void
> conn_key_reverse(struct conn_key *key)
> {
> - struct ct_endpoint tmp = key->src;
> + struct ct_endpoint tmp;
> + memset(&tmp, 0, sizeof tmp);
> + tmp = key->src;
I don't understand the reason to clear the pad bits of tmp here - you
only use the named fields and they should be of the same size (and if
not, the compiler should be performing integer widening / truncation
properly). Did I miss something?
This change was previously removed in V3.
> key->src = key->dst;
> key->dst = tmp;
> }
> @@ -2614,7 +2617,9 @@ static struct alg_exp_node *
> expectation_lookup(struct hmap *alg_expectations, const struct conn_key
*key,
> uint32_t basis, bool src_ip_wc)
> {
> - struct conn_key check_key = *key;
> + struct conn_key check_key;
> + memset(&check_key, 0, sizeof check_key);
> + check_key = *key;
> check_key.src.port = ALG_WC_SRC_PORT;
>
> if (src_ip_wc) {
Again, conn_key_hash and conn_key_cmp should be correct, no?
conn_key padding
_______________________________________________
dev mailing list
[email protected]
https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&data=02%7C01%7Cdball%40vmware.com%7Cd0efdcf4f15d43d97e8e08d68ac14444%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636848960020751215&sdata=8dn50kor0In3sPaJuEC7Vs7SE7ba5%2Bol%2FhVqROzEeK8%3D&reserved=0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
