netdev - Re: [PATCH] aquantia: Fix "ethtool -S" crash when adapter down.

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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 4 May 2017 21:37:04 +0300
From:   Pavel Belous <pavel.belous@...antia.com>
To:     David Arcari <darcari@...hat.com>,
        David Miller <davem@...emloft.net>, LinoSanfilippo@....de
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH] aquantia: Fix "ethtool -S" crash when adapter down.



On 04.05.2017 21:27, David Arcari wrote:
> On 05/04/2017 01:09 PM, Pavel Belous wrote:
>>
>>
>> On 04.05.2017 19:51, David Miller wrote:
>>> From: Lino Sanfilippo <LinoSanfilippo@....de>
>>> Date: Thu, 4 May 2017 18:48:12 +0200
>>>
>>>> Hi Pavel,
>>>>
>>>> On 04.05.2017 18:33, Pavel Belous wrote:
>>>>> From: Pavel Belous <pavel.belous@...antia.com>
>>>>>
>>>>> This patch fixes the crash that happens when driver tries to collect statistics
>>>>> from already released "aq_vec" object.
>>>>>
>>>>> Fixes: 97bde5c4f909 ("net: ethernet: aquantia: Support for NIC-specific code")
>>>>> Signed-off-by: Pavel Belous <pavel.belous@...antia.com>
>>>>> ---
>>>>>  drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 3 ++-
>>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
>>>>> b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
>>>>> index cdb0299..3a32573 100644
>>>>> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
>>>>> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
>>>>> @@ -755,7 +755,7 @@ void aq_nic_get_stats(struct aq_nic_s *self, u64 *data)
>>>>>      count = 0U;
>>>>>
>>>>>      for (i = 0U, aq_vec = self->aq_vec[0];
>>>>> -        self->aq_vecs > i; ++i, aq_vec = self->aq_vec[i]) {
>>>>> +        aq_vec && self->aq_vecs > i; ++i, aq_vec = self->aq_vec[i]) {
>>>>>          data += count;
>>>>>          aq_vec_get_sw_stats(aq_vec, data, &count);
>>>>>      }
>>>>> @@ -961,6 +961,7 @@ void aq_nic_free_hot_resources(struct aq_nic_s *self)
>>>>>      for (i = AQ_DIMOF(self->aq_vec); i--;) {
>>>>>          if (self->aq_vec[i])
>>>>>              aq_vec_free(self->aq_vec[i]);
>>>>> +            self->aq_vec[i] = NULL;
>>>>>      }
>>>>>
>>>>>  err_exit:;
>>>>>
>>>>
>>>> if the driver does not support statistics when the interface is down, would
>>>> not it be clearer
>>>> to check if netif_running() in get_stats() instead?
>>>
>>> Yes, much cleaner.
>>>
>>> Much better would be to have a cached software copy so that statistics
>>> can be reported regardless of whether the device is down or not.
>>>
>>
>> Thank you.
>> I will think about how to do it better.
>
> It appears that the adapter is still reporting the cumulative hardware stats
> even while its down.  The user is just losing the per queue stats.
>
> Although the loss of the per queue stats is not ideal, this patch still fixes a
> crash.
>
> It might be worthwhile to refactor this patch as a short term solution and then
> subsequently produce a version that contains cached statistics.  Assuming that
> is amenable to everyone of course.
>
> -DA
>

Yes, even adapter is in the down state user can still see statistics 
from the HW.
For example (adapter is down):

$ ethtool -S enp2s0
NIC statistics:
      InPackets: 3237727
      InUCast: 3237214
      InMCast: 391
      InBCast: 122
      InErrors: 0
      OutPackets: 14157898
      OutUCast: 14157089
      OutMCast: 304
      OutBCast: 505
      InUCastOctects: 226714406
      OutUCastOctects: 10463156
      InMCastOctects: 58046
      OutMCastOctects: 44817
      InBCastOctects: 12857
      OutBCastOctects: 41626
      InOctects: 226785309
      OutOctects: 10549599
      InPacketsDma: 0
      OutPacketsDma: 16
      InOctetsDma: 0
      OutOctetsDma: 2396
      InDroppedDma: 0
      Queue[0] InPackets: 0
      Queue[0] OutPackets: 0
      Queue[0] InJumboPackets: 0
      Queue[0] InLroPackets: 0
      Queue[0] InErrors: 0
      Queue[1] InPackets: 0
      Queue[1] OutPackets: 0
      Queue[1] InJumboPackets: 0
      Queue[1] InLroPackets: 0
      Queue[1] InErrors: 0
      Queue[2] InPackets: 0
      Queue[2] OutPackets: 0
      Queue[2] InJumboPackets: 0
      Queue[2] InLroPackets: 0
      Queue[2] InErrors: 0
      Queue[3] InPackets: 0
      Queue[3] OutPackets: 0
      Queue[3] InJumboPackets: 0
      Queue[3] InLroPackets: 0
      Queue[3] InErrors: 0

Lino, David what do you think?
If you agree I can re-submit the patch (with fixed braces).

Regards,
Pavel