| 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
| ||
|
Message-ID: <eb3b0964-662e-2c5f-02c6-6141c4c5bf92@intel.com>
Date: Wed, 16 Mar 2022 10:25:28 +0800
From: "Zhu, Lingshan" <lingshan.zhu@...el.com>
To: "Michael S. Tsirkin" <mst@...hat.com>, Tom Rix <trix@...hat.com>
Cc: jasowang@...hat.com, nathan@...nel.org, ndesaulniers@...gle.com,
sgarzare@...hat.com, xieyongji@...edance.com,
virtualization@...ts.linux-foundation.org,
linux-kernel@...r.kernel.org, llvm@...ts.linux.dev
Subject: Re: [PATCH] vDPA/ifcvf: match pointer check to use
On 3/15/2022 11:15 PM, Michael S. Tsirkin wrote:
> On Tue, Mar 15, 2022 at 08:03:26AM -0700, Tom Rix wrote:
>> On 3/15/22 6:28 AM, Michael S. Tsirkin wrote:
>>> On Tue, Mar 15, 2022 at 05:41:30AM -0700, trix@...hat.com wrote:
>>>> From: Tom Rix <trix@...hat.com>
>>>>
>>>> Clang static analysis reports this issue
>>>> ifcvf_main.c:49:4: warning: Called function
>>>> pointer is null (null dereference)
>>>> vf->vring->cb.callback(vring->cb.private);
>>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>
>>>> The check
>>>> vring = &vf->vring[i];
>>>> if (vring->cb.callback)
>>>>
>>>> Does not match the use. Change dereference so they match.
>>>>
>>>> Fixes: 79333575b8bd ("vDPA/ifcvf: implement shared IRQ feature")
>>> Thanks a lot! I squashed this into the offending patch - no point in
>>> breaking bisect. Pushed to linux. However I'm now
>>> having second thoughts about applying that patchset - I'd like
>>> soma analysis explaining how this got through testing.
>> static analysis is something i do treewide.
>>
>> There are currently ~2500 issues in linux-next, do not panic! many are false
>> positives.
>>
>> It is pretty easy to setup and once you have a baseline you can filter only
>> your files.
>>
>> Tom
> Thanks for that info! I was actually directing this question to the
> contributor since the code does not look like it could have ever
> worked. I don't have the hardware in question myself.
Oh, that's my bad introducing this bug by typo, thanks Tom for fixing that!
In my previous testing, I tolerated the lower performance(only one queue
working as you pointed out)
due to the shared irq, and did not check mq status, sorry for that.
After fixing this issue, test again:
(1)set nvectors = 2 after allocate MSI vectors, force the queues share
one vector. from /proc/interrupts, we can see:
[lszhu@...alhost linux]$ cat /proc/interrupts | grep ifcvf
241: 0 0 0 0 0 0
0 0 0 0 0 0 0
0 0 0 0 0 0 0
0 0 0 0 0 0 0 0
0 0 2724424 0 0 0 0
0 0 0 0 0 IR-PCI-MSI
534528-edge ifcvf[0000:01:00.5]-vqs-reused-irq
242: 0 0 0 0 0 0
0 0 0 0 0 0 0
0 0 0 0 0 0 0
0 0 0 0 0 0 0 0
0 0 0 0 0 0 0
0 0 0 0 0 IR-PCI-MSI
534529-edge ifcvf[0000:01:00.5]-config
251: 0 0 0 0 0 0
0 0 0 0 0 2693318 0
0 0 0 0 0 0 0
0 0 0 0 0 0 0 0
0 0 0 0 0 0 0
0 0 0 0 0 IR-PCI-MSI
536576-edge ifcvf[0000:01:00.6]-vqs-reused-irq
252: 0 0 0 0 0 0
0 0 0 0 0 0 0
0 0 0 0 0 0 0
0 0 0 0 0 0 0 0
0 0 0 0 0 0 0
0 0 0 0 0 IR-PCI-MSI
536577-edge ifcvf[0000:01:00.6]-config
(2) after several rounds of scp from a VF to another, at the source
side, ethtool -S shows(cut off, only tx):
localhost:/home/lszhu # ethtool -S eth1
NIC statistics:
tx_queue_0_packets: 437256
tx_queue_0_bytes: 629246017
tx_queue_0_xdp_tx: 0
tx_queue_0_xdp_tx_drops: 0
tx_queue_0_kicks: 34089
tx_queue_1_packets: 73
tx_queue_1_bytes: 96721
tx_queue_1_xdp_tx: 0
tx_queue_1_xdp_tx_drops: 0
tx_queue_1_kicks: 12
tx_queue_2_packets: 294647
tx_queue_2_bytes: 433949815
tx_queue_2_xdp_tx: 0
tx_queue_2_xdp_tx_drops: 0
tx_queue_2_kicks: 14948
tx_queue_3_packets: 20226451
tx_queue_3_bytes: 29735633548
tx_queue_3_xdp_tx: 0
tx_queue_3_xdp_tx_drops: 0
tx_queue_3_kicks: 1123675
so every queue carries traffic now, all enabled
Thanks,
Zhu Lingshan
>
>
>>>> Signed-off-by: Tom Rix <trix@...hat.com>
>>>> ---
>>>> drivers/vdpa/ifcvf/ifcvf_main.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> index 3b48e717e89f7..4366320fb68d3 100644
>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> @@ -46,7 +46,7 @@ static irqreturn_t ifcvf_vqs_reused_intr_handler(int irq, void *arg)
>>>> for (i = 0; i < vf->nr_vring; i++) {
>>>> vring = &vf->vring[i];
>>>> if (vring->cb.callback)
>>>> - vf->vring->cb.callback(vring->cb.private);
>>>> + vring->cb.callback(vring->cb.private);
>>>> }
>>>> return IRQ_HANDLED;
>>>> --
>>>> 2.26.3
Powered by blists - more mailing lists