lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <010f0cef-0f6e-1d4e-47ec-29a3c667b97a@redhat.com>
Date:   Wed, 11 Jan 2023 06:29:53 -0800
From:   Tom Rix <trix@...hat.com>
To:     Shay Agroskin <shayagr@...zon.com>
Cc:     Eric Dumazet <edumazet@...gle.com>, akiyano@...zon.com,
        darinzon@...zon.com, ndagan@...zon.com, saeedb@...zon.com,
        davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
        nathan@...nel.org, ndesaulniers@...gle.com, khalasa@...p.pl,
        wsa+renesas@...g-engineering.com, yuancan@...wei.com,
        tglx@...utronix.de, 42.hyeyoo@...il.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, llvm@...ts.linux.dev
Subject: Re: [PATCH] net: ena: initialize dim_sample


On 1/11/23 12:46 AM, Shay Agroskin wrote:
>
> Tom Rix <trix@...hat.com> writes:
>
>> On 1/10/23 8:58 AM, Shay Agroskin wrote:
>>>
>>> Eric Dumazet <edumazet@...gle.com> writes:
>>>
>>>> On Sun, Jan 8, 2023 at 3:38 PM Tom Rix <trix@...hat.com> 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
>>>
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ