## TodoWrite Checklist - [x] git sha of the commit - [x] Author: line from the commit - [x] One line subject of the commit - [x] A brief summary of the commit - [x] A unified diff of the commit, quoted as though it's in an email reply - [x] Place questions about regressions alongside code in diff - [x] Snip portions of quoted content unrelated to review - [x] Check every hunk for relevance to review comments
## TodoWrite for diff hunks: - [x] ofproto-provider.h pkt_stats_add function - relevant (introduces potential overflow) - [x] ofproto-provider.h pkt_stats_addmul function - relevant (overflow vulnerability) - [x] ofproto-provider.h struct ofproto changes - relevant (new field) - [x] ofproto-provider.h struct rule_actions changes - relevant (new field) - [x] ofproto-provider.h function signature change - relevant (API change) - [x] ofproto.c rule_actions_create function - relevant (logic issues) - [x] ofproto.c rules_mark_for_removal function - relevant (uses vulnerable function) - [x] ofproto.c ofproto_get_pkt_stats function - relevant (uses vulnerable function) git sha: Not available from patch format Author: Gaetan Rivet <[email protected]> ofproto: Add ofproto output packet stats This commit adds counters to track emitted packets to each ofproto and marks rules that have output actions that would result in packets being emitted outside OVS with new counters specific for those rules. > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h > index 3ca87748bf..c4feb1b01b 100644 > --- a/ofproto/ofproto-provider.h > +++ b/ofproto/ofproto-provider.h > @@ -91,6 +91,16 @@ pkt_stats_add(struct pkt_stats *dst, struct pkt_stats src) > src.n_offload_bytes); > } > > +static inline void > +pkt_stats_addmul(struct pkt_stats *dst, struct pkt_stats src, uint64_t n) > +{ > + src.n_packets = ovs_u64_safemul(src.n_packets, n); > + src.n_bytes = ovs_u64_safemul(src.n_bytes, n); > + src.n_offload_packets = ovs_u64_safemul(src.n_offload_packets, n); > + src.n_offload_bytes = ovs_u64_safemul(src.n_offload_bytes, n); > + pkt_stats_add(dst, src); > +} Does pkt_stats_addmul handle overflow correctly when ovs_u64_safemul returns UINT64_MAX? The multiplication results are passed directly to pkt_stats_add without checking if overflow occurred, which could lead to incorrect statistics accumulation. > [ ... ] > @@ -185,7 +185,8 @@ void ofproto_init_max_ports(struct ofproto *, uint16_t > max_ports); > /* Read the current packet stats of this ofproto. > * It sums each currently existing rule packet stats as well > * as past, removed rules. */ > -enum ofperr ofproto_get_pkt_stats(struct ofproto *, struct pkt_stats *) > +enum ofperr ofproto_get_pkt_stats(struct ofproto *, struct pkt_stats *, > + struct pkt_stats *) > OVS_EXCLUDED(ofproto_mutex); This API change breaks backward compatibility. Are all callers of ofproto_get_pkt_stats being updated to handle the new parameter? > [ ... ] > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > index 7b2f9a3fc0..f9a6b13d94 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -3274,6 +3276,31 @@ rule_actions_create(const struct ofpact *ofpacts, > size_t ofpacts_len) > actions->has_learn_with_delete = (next_learn_with_delete(actions, NULL) > != NULL); > > + actions->n_output = 0; > + OFPACT_FOR_EACH_FLATTENED (a, ofpacts, ofpacts_len) { > + if (a->type == OFPACT_OUTPUT > + || a->type == OFPACT_OUTPUT_TRUNC > + || a->type == OFPACT_OUTPUT_REG) { > + actions->n_output += 1; > + } > + if (a->type == OFPACT_OUTPUT > + || a->type == OFPACT_OUTPUT_TRUNC > + || a->type == OFPACT_OUTPUT_REG > + || a->type == OFPACT_ENQUEUE > + || a->type == OFPACT_CONTROLLER) { > + has_output = true; > + } > + } > + if (has_output && actions->n_output == 0) { > + actions->n_output = 1; > + } > + /* Rules within a group are a rule set, not a rule list. > + * It means if there are more than one 'output' action, only > + * one of them will be executed. */ > + if (actions->has_groups && actions->n_output > 1) { > + actions->n_output = 1; > + } Can n_output overflow since it's declared as uint8_t? With a large number of output actions in the ofpacts, the increment could wrap around from 255 back to 0, leading to incorrect output counting. > @@ -4602,6 +4629,10 @@ rules_mark_for_removal(struct ofproto *ofproto, struct > rule_collection *rules, > > ofproto->ofproto_class->rule_get_stats(rule, &stats, &used); > pkt_stats_add(&ofproto->removed_stats, stats); > + if (rule->actions->n_output) { > + pkt_stats_addmul(&ofproto->removed_output_stats, stats, > + rule->actions->n_output); > + } This uses the potentially vulnerable pkt_stats_addmul function. Could the multiplication overflow propagate incorrect statistics into removed_output_stats? > @@ -6411,12 +6446,20 @@ ofproto_get_pkt_stats(struct ofproto *ofproto, struct > pkt_stats *stats) > > *stats = removed_stats; > > + if (output_stats != NULL) { > + *output_stats = removed_output_stats; > + } > + > RULE_COLLECTION_FOR_EACH (rule, &rules) { > struct pkt_stats rule_stats; > long long int used; > > ofproto->ofproto_class->rule_get_stats(rule, &rule_stats, &used); > pkt_stats_add(stats, rule_stats); > + if (output_stats != NULL && rule->actions->n_output) { > + pkt_stats_addmul(output_stats, rule_stats, > + rule->actions->n_output); > + } > } Same concern here - does this multiplication overflow handling propagate to the output_stats accumulation? _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
