Messages in this thread | | | Subject | Re: net_dim() may use uninitialized data | From | Tal Gilboa <> | Date | Thu, 12 Apr 2018 11:32:41 +0300 |
| |
On 4/5/2018 4:13 PM, Geert Uytterhoeven wrote: > Hi Tal, > > With gcc-4.1.2: > > drivers/net/ethernet/broadcom/bcmsysport.c: In function ‘bcm_sysport_poll’: > include/linux/net_dim.h:354: warning: ‘curr_stats.ppms’ may be > used uninitialized in this function > include/linux/net_dim.h:354: warning: ‘curr_stats.bpms’ may be > used uninitialized in this function > include/linux/net_dim.h:354: warning: ‘curr_stats.epms’ may be > used uninitialized in this function > > Indeed, ... > > | static inline void net_dim_calc_stats(struct net_dim_sample *start, > | struct net_dim_sample *end, > | struct net_dim_stats *curr_stats) > | { > | /* u32 holds up to 71 minutes, should be enough */ > | u32 delta_us = ktime_us_delta(end->time, start->time); > | u32 npkts = BIT_GAP(BITS_PER_TYPE(u32), end->pkt_ctr, start->pkt_ctr); > | u32 nbytes = BIT_GAP(BITS_PER_TYPE(u32), end->byte_ctr, > | start->byte_ctr); > | > | if (!delta_us) > | return; > > ... if delta_us is zero, none of the below will be initialized ... > > | curr_stats->ppms = DIV_ROUND_UP(npkts * USEC_PER_MSEC, delta_us); > | curr_stats->bpms = DIV_ROUND_UP(nbytes * USEC_PER_MSEC, delta_us); > | curr_stats->epms = DIV_ROUND_UP(NET_DIM_NEVENTS * USEC_PER_MSEC, > | delta_us); > | } > | > | static inline void net_dim(struct net_dim *dim, > | struct net_dim_sample end_sample) > | { > | struct net_dim_stats curr_stats; > | u16 nevents; > | > | switch (dim->state) { > | case NET_DIM_MEASURE_IN_PROGRESS: > | nevents = BIT_GAP(BITS_PER_TYPE(u16), > | end_sample.event_ctr, > | dim->start_sample.event_ctr); > | if (nevents < NET_DIM_NEVENTS) > | break; > | net_dim_calc_stats(&dim->start_sample, &end_sample, > | &curr_stats); > > ... in the output parameter curr_stats ... > > | if (net_dim_decision(&curr_stats, dim)) { > > ... and net_dim_decision will make some funky decisions based on > uninitialized data. > > What are the proper values to initialize curr_stats with? > Alternatively, perhaps the call to net_dim_decision() should be made > dependent on delta_us being non-zero?
First, thanks a lot for pointing this out. There are no valid values for initializing curr_stats. If we consider the most straightforward (all 0s) this may result in a (big) negative delta between current and previous stats and a wrong decision. Any other value would make very little sense. The case of !delta_us is an error flow (0 time passed or more probably issues when setting start and/or end times). I suggest adding a return value to net_dim_calc_stats() and abort the net_dim cycle if an error occurs.
> > | dim->state = NET_DIM_APPLY_NEW_PROFILE; > | schedule_work(&dim->work); > | break; > | } > | /* fall through */ > | case NET_DIM_START_MEASURE: > | dim->state = NET_DIM_MEASURE_IN_PROGRESS; > | break; > | case NET_DIM_APPLY_NEW_PROFILE: > | break; > | } > | } > > Gr{oetje,eeting}s, > > Geert >
| |