Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: net_dim() may use uninitialized data
To:     Geert Uytterhoeven <geert@linux-m68k.org>
Cc:     netdev <netdev@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
References: <CAMuHMdWovmqqgrOvtA=cyineJigwOtv-tGwz_PBVRFrgtryumw@mail.gmail.com>
From:   Tal Gilboa <talgi@mellanox.com>
Message-ID: <a90cf5d4-f744-8b4f-4d3c-36ddd5842aaa@mellanox.com>
Date:   Thu, 12 Apr 2018 11:32:41 +0300
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.7.0
MIME-Version: 1.0
In-Reply-To: <CAMuHMdWovmqqgrOvtA=cyineJigwOtv-tGwz_PBVRFrgtryumw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Received-SPF: None (protection.outlook.com: mellanox.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Apr 2018 08:32:52.7831 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 89e477be-e970-4dd7-9bab-08d5a04ffbf5
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: a652971c-7d2e-4d9b-a6a4-d149256f461b
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0501MB2607
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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
>