Re: [PATCH] net: ena: initialize dim_sample

From: Tom Rix
Date: Wed Jan 11 2023 - 09:34:17 EST



On 1/11/23 12:46 AM, Shay Agroskin wrote:

Tom Rix <trix@xxxxxxxxxx> writes:

On 1/10/23 8:58 AM, Shay Agroskin wrote:

Eric Dumazet <edumazet@xxxxxxxxxx> writes:

On Sun, Jan 8, 2023 at 3:38 PM Tom Rix <trix@xxxxxxxxxx> wrote:

clang static analysis reports this problem
drivers/net/ethernet/amazon/ena/ena_netdev.c:1821:2: warning:
Passed-by-value struct
  argument contains uninitialized data (e.g., field: 'comp_ctr')
[core.CallAndMessage]
        net_dim(&ena_napi->dim, dim_sample);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

net_dim can call dim_calc_stats() which uses the comp_ctr element,
so it must be initialized.

This seems to be a dim_update_sample() problem really, when comp_ctr
has been added...

Your patch works, but we could avoid pre-initializing dim_sample in
all callers,
then re-writing all but one field...

diff --git a/include/linux/dim.h b/include/linux/dim.h
index
6c5733981563eadf5f06c59c5dc97df961692b02..4604ced4517268ef8912cd8053ac8f4d2630f977
100644
--- a/include/linux/dim.h
+++ b/include/linux/dim.h
@@ -254,6 +254,7 @@ dim_update_sample(u16 event_ctr, u64 packets, u64
bytes, struct dim_sample *s)
        s->pkt_ctr   = packets;
        s->byte_ctr  = bytes;
        s->event_ctr = event_ctr;
+       s->comp_ctr  = 0;
 }

 /**

Hi,

I'd rather go with Eric's solution to this issue than zero the whole
struct in ENA

Please look at the other callers of dim_update_sample.  The common
pattern is to initialize the struct.

This alternative will work, but the pattern of initializing the struct
the other (~20) callers should be refactored.

Tom


While Eric's patch might be bigger if you also remove the pre-initialization in the other drivers, the Linux code itself would be smaller (granted not significantly) and
it make less room for pitfalls in adding DIM support in other drivers.

Is there a good argument against using Eric's patch other than 'the other patch would be bigger' ?

No, I think it a better approach and if Eric can take it forward that would be great.

However when you start refactoring, it may grow larger than the single fix.

For instance, passing the structure by value could be changed to passing by reference.

Tom


Shay


Thanks,
Shay