| Message ID | c993346e790afd96f71d003256f082aceec3fbc2.1716807712.git.echaudro@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,1/7] Coverity: Fix Coverity `Unintentional integer overflow` reports. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 5/27/24 13:01, Eelco Chaudron wrote: > Instead of casting time_t to uint32_t for the 0xffffffff comparison, > define a TIME_T_MAX and use it for both setting and comparison. > > Fixes: c72e245a0e2c ("Add InMon's sFlow Agent library to the build system.") > Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > -- > Note that this checkpatch reports an 'Improper whitespace > around control block' error on this patch. But I did not > want to change the code style in this entire file. > --- > include/openvswitch/types.h | 3 +++ > lib/sflow_receiver.c | 3 ++- > ofproto/ofproto-dpif-sflow.c | 2 +- > 3 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/include/openvswitch/types.h b/include/openvswitch/types.h > index 8c5ec94a6..bfeed752c 100644 > --- a/include/openvswitch/types.h > +++ b/include/openvswitch/types.h > @@ -33,6 +33,9 @@ extern "C" { > #define OVS_FORCE > #endif > > +/* Maximum time_t value. */ > +#define TIME_T_MAX (time_t)((1UL << ((sizeof(time_t) << 3) - 1)) - 1) We should multiply by CHAR_BITS instead of '<< 3'. This also assumes that time_t is an integer type, which is true for POSIX, but isn't generally true for C. And the openvswitch/types.h is probably not a good place for the define. This header is for OVS-specific types and structures that include those specific types. We shouldn't export stuff as generic as TIME_T_MAX. We may probably just replace sFlowRcvrTimeout with a uint32_t value and compare agains UIN32_MAX. It doesn't have to be time_t. It's just a tick counter that should not be too high. And we're not even using it anyway. Best regards, Ilya Maximets. > + > /* The ovs_be<N> types indicate that an object is in big-endian, not > * native-endian, byte order. They are otherwise equivalent to uint<N>_t. */ > typedef uint16_t OVS_BITWISE ovs_be16; > diff --git a/lib/sflow_receiver.c b/lib/sflow_receiver.c > index 4162518e3..b5b02c060 100644 > --- a/lib/sflow_receiver.c > +++ b/lib/sflow_receiver.c > @@ -146,7 +146,8 @@ void sfl_receiver_tick(SFLReceiver *receiver) > // if there are any samples to send, flush them now > if(receiver->sampleCollector.numSamples > 0) sendSample(receiver); > // check the timeout > - if(receiver->sFlowRcvrTimeout && (u_int32_t)receiver->sFlowRcvrTimeout != 0xFFFFFFFF) { > + if(receiver->sFlowRcvrTimeout > + && receiver->sFlowRcvrTimeout != TIME_T_MAX) { > // count down one tick and reset if we reach 0 > if(--receiver->sFlowRcvrTimeout == 0) reset(receiver); > } > diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c > index 4a68e9b94..08c16f6f8 100644 > --- a/ofproto/ofproto-dpif-sflow.c > +++ b/ofproto/ofproto-dpif-sflow.c > @@ -808,7 +808,7 @@ dpif_sflow_set_options(struct dpif_sflow *ds, > > receiver = sfl_agent_addReceiver(ds->sflow_agent); > sfl_receiver_set_sFlowRcvrOwner(receiver, "Open vSwitch sFlow"); > - sfl_receiver_set_sFlowRcvrTimeout(receiver, 0xffffffff); > + sfl_receiver_set_sFlowRcvrTimeout(receiver, TIME_T_MAX); > > /* Set the sampling_rate down in the datapath. */ > ds->probability = MAX(1, UINT32_MAX / ds->options->sampling_rate);
Would it help if I set up a separate github project for this sFlow encoding C code? Then we could make it easier to incorporate in OVS by fixing the whitespace and indentation issues there, and maybe change all the "time_t" variables to uint32_t to save unnecessary headaches like these. (We could also shrink it by removing hooks that were only added to support an SNMP control MIB that OVS does not have or need.) The library has evolved only a little since it was copy-pasted into OVS a while ago, but the latest variant of it is the one that is part of the host-sflow project here: https://github.com/sflow/host-sflow/tree/master/src/sflow This version has a more efficient XDR encoding scheme, and it supports the new packet-drop-notification extension to sFlow. It would certainly benefit from having the scrutiny of the OVS project brought to bear, so let me know if you think a separate github project would be usable. I don't even know if OVS now uses a "subproject" mechanism or not, so maybe the idea is moot. Regards, Neil ------ Neil McKee InMon Corp.
On 27 May 2024, at 16:42, Ilya Maximets wrote: > On 5/27/24 13:01, Eelco Chaudron wrote: >> Instead of casting time_t to uint32_t for the 0xffffffff comparison, >> define a TIME_T_MAX and use it for both setting and comparison. >> >> Fixes: c72e245a0e2c ("Add InMon's sFlow Agent library to the build system.") >> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >> -- >> Note that this checkpatch reports an 'Improper whitespace >> around control block' error on this patch. But I did not >> want to change the code style in this entire file. >> --- >> include/openvswitch/types.h | 3 +++ >> lib/sflow_receiver.c | 3 ++- >> ofproto/ofproto-dpif-sflow.c | 2 +- >> 3 files changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/include/openvswitch/types.h b/include/openvswitch/types.h >> index 8c5ec94a6..bfeed752c 100644 >> --- a/include/openvswitch/types.h >> +++ b/include/openvswitch/types.h >> @@ -33,6 +33,9 @@ extern "C" { >> #define OVS_FORCE >> #endif >> >> +/* Maximum time_t value. */ >> +#define TIME_T_MAX (time_t)((1UL << ((sizeof(time_t) << 3) - 1)) - 1) > > We should multiply by CHAR_BITS instead of '<< 3'. > > This also assumes that time_t is an integer type, which is true for POSIX, > but isn't generally true for C. > > And the openvswitch/types.h is probably not a good place for the define. > This header is for OVS-specific types and structures that include those > specific types. We shouldn't export stuff as generic as TIME_T_MAX. > > > We may probably just replace sFlowRcvrTimeout with a uint32_t value and > compare agains UIN32_MAX. It doesn't have to be time_t. It's just a tick > counter that should not be too high. And we're not even using it anyway. I did this at first, but the TIME_T_MAX was a smaller change. But with all your feedback above, I’ll do the uint32_t change instead for the v2. > Best regards, Ilya Maximets. > >> + >> /* The ovs_be<N> types indicate that an object is in big-endian, not >> * native-endian, byte order. They are otherwise equivalent to uint<N>_t. */ >> typedef uint16_t OVS_BITWISE ovs_be16; >> diff --git a/lib/sflow_receiver.c b/lib/sflow_receiver.c >> index 4162518e3..b5b02c060 100644 >> --- a/lib/sflow_receiver.c >> +++ b/lib/sflow_receiver.c >> @@ -146,7 +146,8 @@ void sfl_receiver_tick(SFLReceiver *receiver) >> // if there are any samples to send, flush them now >> if(receiver->sampleCollector.numSamples > 0) sendSample(receiver); >> // check the timeout >> - if(receiver->sFlowRcvrTimeout && (u_int32_t)receiver->sFlowRcvrTimeout != 0xFFFFFFFF) { >> + if(receiver->sFlowRcvrTimeout >> + && receiver->sFlowRcvrTimeout != TIME_T_MAX) { >> // count down one tick and reset if we reach 0 >> if(--receiver->sFlowRcvrTimeout == 0) reset(receiver); >> } >> diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c >> index 4a68e9b94..08c16f6f8 100644 >> --- a/ofproto/ofproto-dpif-sflow.c >> +++ b/ofproto/ofproto-dpif-sflow.c >> @@ -808,7 +808,7 @@ dpif_sflow_set_options(struct dpif_sflow *ds, >> >> receiver = sfl_agent_addReceiver(ds->sflow_agent); >> sfl_receiver_set_sFlowRcvrOwner(receiver, "Open vSwitch sFlow"); >> - sfl_receiver_set_sFlowRcvrTimeout(receiver, 0xffffffff); >> + sfl_receiver_set_sFlowRcvrTimeout(receiver, TIME_T_MAX); >> >> /* Set the sampling_rate down in the datapath. */ >> ds->probability = MAX(1, UINT32_MAX / ds->options->sampling_rate);
On 28 May 2024, at 1:04, Neil McKee wrote: > Would it help if I set up a separate github project for this sFlow encoding > C code? Then we could make it easier to incorporate in OVS by fixing the > whitespace and indentation issues there, and maybe change all the "time_t" > variables to uint32_t to save unnecessary headaches like these. (We could > also shrink it by removing hooks that were only added to support an SNMP > control MIB that OVS does not have or need.) > > The library has evolved only a little since it was copy-pasted into OVS a > while ago, but the latest variant of it is the one that is part of the > host-sflow project here: > https://github.com/sflow/host-sflow/tree/master/src/sflow > This version has a more efficient XDR encoding scheme, and it supports the > new packet-drop-notification extension to sFlow. It would certainly > benefit from having the scrutiny of the OVS project brought to bear, so > let me know if you think a separate github project would be usable. > > I don't even know if OVS now uses a "subproject" mechanism or not, so > maybe the idea is moot. Hi Neil, Thanks for your feedback. Currently, we do not have any git submodules in OVS, and I’m not sure if making a fork of https://github.com/sflow/host-sflow/, and extracting the parts we need, keeping it in sync is the cleanest way forward. I assume the cleanest way would be if we could occasionally sync the files from https://github.com/sflow/host-sflow/ without any modifications, but not sure if that is doable within the host-flows project. Ilya, thoughts? //Eelco
On 5/28/24 13:27, Eelco Chaudron wrote: > > > On 28 May 2024, at 1:04, Neil McKee wrote: > >> Would it help if I set up a separate github project for this sFlow encoding >> C code? Then we could make it easier to incorporate in OVS by fixing the >> whitespace and indentation issues there, and maybe change all the "time_t" >> variables to uint32_t to save unnecessary headaches like these. (We could >> also shrink it by removing hooks that were only added to support an SNMP >> control MIB that OVS does not have or need.) >> >> The library has evolved only a little since it was copy-pasted into OVS a >> while ago, but the latest variant of it is the one that is part of the >> host-sflow project here: >> https://github.com/sflow/host-sflow/tree/master/src/sflow >> This version has a more efficient XDR encoding scheme, and it supports the >> new packet-drop-notification extension to sFlow. It would certainly >> benefit from having the scrutiny of the OVS project brought to bear, so >> let me know if you think a separate github project would be usable. >> >> I don't even know if OVS now uses a "subproject" mechanism or not, so >> maybe the idea is moot. > > Hi Neil, > > Thanks for your feedback. Currently, we do not have any git submodules in OVS, > and I’m not sure if making a fork of https://github.com/sflow/host-sflow/, and > extracting the parts we need, keeping it in sync is the cleanest way forward. > > I assume the cleanest way would be if we could occasionally sync the files from > https://github.com/sflow/host-sflow/ without any modifications, but not sure if > that is doable within the host-flows project. > > Ilya, thoughts? > > //Eelco Thanks for the offer, Neil! I think, from packaging perspective, having sFlow code just be within OVS is much more simple option than consuming an external library. And, as you said, the original code didn't evolve too much so it's not a big burden to port changes. On the other hand, we didn't actually port any changes for a while and OVS developers are generally not an sFlow experts, so consuming an external library would be easier from that perspective. Since it's not a huge amount of code and OVS will still need a decent amount of glue to pair with the library as well as maintain OVS-specific counters, I think, it's better to keep things as-is for now and try to periodically sync with the original code from host-sflow. Sub-projects/modules/trees are generally not easy to work with, so I'd like to avoid them, if possible. We do not have any at the moment. The new XDR encoding looks very nice though, we should definitely port that! Best regards, Ilya Maximets.
The only other suggestion I have is that it might be worth using the sFlow-RT "sflow-test" app from time to time as an external test that will only pass if the sFlow agent in OVS is behaving correctly for both counters and packet samples. In addition to the browser UI it has an API that can be queried with curl, so it would be quite possible to script a test that configured OVS, launched a series of iperfs with gaps between, and then read back the results. It would take several minutes (can't use the cunning time-compression trick that other OVS tests use) but it would cover a lot of the things that can go wrong. There is a docker image called "sflow/sflow-test" that you could use. Here is a blog post about it: https://blog.sflow.com/2015/11/sflow-test.html ------ Neil McKee InMon Corp. On Tue, May 28, 2024 at 5:13 AM Ilya Maximets <i.maximets@ovn.org> wrote: > On 5/28/24 13:27, Eelco Chaudron wrote: > > > > > > On 28 May 2024, at 1:04, Neil McKee wrote: > > > >> Would it help if I set up a separate github project for this sFlow > encoding > >> C code? Then we could make it easier to incorporate in OVS by fixing > the > >> whitespace and indentation issues there, and maybe change all the > "time_t" > >> variables to uint32_t to save unnecessary headaches like these. (We > could > >> also shrink it by removing hooks that were only added to support an SNMP > >> control MIB that OVS does not have or need.) > >> > >> The library has evolved only a little since it was copy-pasted into OVS > a > >> while ago, but the latest variant of it is the one that is part of the > >> host-sflow project here: > >> https://github.com/sflow/host-sflow/tree/master/src/sflow > >> This version has a more efficient XDR encoding scheme, and it supports > the > >> new packet-drop-notification extension to sFlow. It would certainly > >> benefit from having the scrutiny of the OVS project brought to bear, so > >> let me know if you think a separate github project would be usable. > >> > >> I don't even know if OVS now uses a "subproject" mechanism or not, so > >> maybe the idea is moot. > > > > Hi Neil, > > > > Thanks for your feedback. Currently, we do not have any git submodules > in OVS, > > and I’m not sure if making a fork of > https://github.com/sflow/host-sflow/, and > > extracting the parts we need, keeping it in sync is the cleanest way > forward. > > > > I assume the cleanest way would be if we could occasionally sync the > files from > > https://github.com/sflow/host-sflow/ without any modifications, but not > sure if > > that is doable within the host-flows project. > > > > Ilya, thoughts? > > > > //Eelco > > Thanks for the offer, Neil! > > I think, from packaging perspective, having sFlow code just be within OVS > is much > more simple option than consuming an external library. And, as you said, > the > original code didn't evolve too much so it's not a big burden to port > changes. > On the other hand, we didn't actually port any changes for a while and OVS > developers > are generally not an sFlow experts, so consuming an external library would > be easier > from that perspective. > > Since it's not a huge amount of code and OVS will still need a decent > amount of > glue to pair with the library as well as maintain OVS-specific counters, I > think, > it's better to keep things as-is for now and try to periodically sync with > the > original code from host-sflow. Sub-projects/modules/trees are generally > not easy > to work with, so I'd like to avoid them, if possible. We do not have any > at the > moment. > > The new XDR encoding looks very nice though, we should definitely port > that! > > Best regards, Ilya Maximets. >
diff --git a/include/openvswitch/types.h b/include/openvswitch/types.h index 8c5ec94a6..bfeed752c 100644 --- a/include/openvswitch/types.h +++ b/include/openvswitch/types.h @@ -33,6 +33,9 @@ extern "C" { #define OVS_FORCE #endif +/* Maximum time_t value. */ +#define TIME_T_MAX (time_t)((1UL << ((sizeof(time_t) << 3) - 1)) - 1) + /* The ovs_be<N> types indicate that an object is in big-endian, not * native-endian, byte order. They are otherwise equivalent to uint<N>_t. */ typedef uint16_t OVS_BITWISE ovs_be16; diff --git a/lib/sflow_receiver.c b/lib/sflow_receiver.c index 4162518e3..b5b02c060 100644 --- a/lib/sflow_receiver.c +++ b/lib/sflow_receiver.c @@ -146,7 +146,8 @@ void sfl_receiver_tick(SFLReceiver *receiver) // if there are any samples to send, flush them now if(receiver->sampleCollector.numSamples > 0) sendSample(receiver); // check the timeout - if(receiver->sFlowRcvrTimeout && (u_int32_t)receiver->sFlowRcvrTimeout != 0xFFFFFFFF) { + if(receiver->sFlowRcvrTimeout + && receiver->sFlowRcvrTimeout != TIME_T_MAX) { // count down one tick and reset if we reach 0 if(--receiver->sFlowRcvrTimeout == 0) reset(receiver); } diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c index 4a68e9b94..08c16f6f8 100644 --- a/ofproto/ofproto-dpif-sflow.c +++ b/ofproto/ofproto-dpif-sflow.c @@ -808,7 +808,7 @@ dpif_sflow_set_options(struct dpif_sflow *ds, receiver = sfl_agent_addReceiver(ds->sflow_agent); sfl_receiver_set_sFlowRcvrOwner(receiver, "Open vSwitch sFlow"); - sfl_receiver_set_sFlowRcvrTimeout(receiver, 0xffffffff); + sfl_receiver_set_sFlowRcvrTimeout(receiver, TIME_T_MAX); /* Set the sampling_rate down in the datapath. */ ds->probability = MAX(1, UINT32_MAX / ds->options->sampling_rate);
Instead of casting time_t to uint32_t for the 0xffffffff comparison, define a TIME_T_MAX and use it for both setting and comparison. Fixes: c72e245a0e2c ("Add InMon's sFlow Agent library to the build system.") Signed-off-by: Eelco Chaudron <echaudro@redhat.com> -- Note that this checkpatch reports an 'Improper whitespace around control block' error on this patch. But I did not want to change the code style in this entire file. --- include/openvswitch/types.h | 3 +++ lib/sflow_receiver.c | 3 ++- ofproto/ofproto-dpif-sflow.c | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-)