-----Original Message-----
From: Yury Norov <yury.norov@gmail.com>
Sent: Tuesday, December 12, 2023 10:04 PM
To: Souradeep Chakrabarti <redacted>
Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
[off-list ref]; wei.liu@kernel.org; Dexuan Cui
[off-list ref]; davem@davemloft.net; edumazet@google.com;
kuba@kernel.org; pabeni@redhat.com; Long Li [off-list ref];
leon@kernel.org; cai.huoqing@linux.dev; ssengar@linux.microsoft.com;
vkuznets@redhat.com; tglx@linutronix.de; linux-hyperv@vger.kernel.org;
netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
rdma@vger.kernel.org; Souradeep Chakrabarti [off-list ref];
Paul Rosswurm [off-list ref]
Subject: [EXTERNAL] Re: [PATCH V5 net-next] net: mana: Assigning IRQ affinity on
HT cores

[Some people who received this message don't often get email from
yury.norov@gmail.com. Learn why this is important at
https://aka.ms/LearnAboutSenderIdentification ]

quoted

quoted

quoted

quoted

quoted

+     rcu_read_lock();
+     for_each_numa_hop_mask(next, next_node) {
+             cpumask_andnot(curr, next, prev);
+             for (w = cpumask_weight(curr), cnt = 0; cnt < w; ) {
+                     cpumask_copy(cpus, curr);
+                     for_each_cpu(cpu, cpus) {
+                             irq_set_affinity_and_hint(irqs[i],

topology_sibling_cpumask(cpu));

quoted

quoted

quoted

quoted

quoted

+                             if (++i == nvec)
+                                     goto done;

Think what if you're passed with irq_setup(NULL, 0, 0).
That's why I suggested to place this check at the beginning.

irq_setup() is a helper function for mana_gd_setup_irqs(), which
already takes care of no NULL pointer for irqs, and 0 number of interrupts can

not be passed.

quoted

quoted

quoted

nvec = pci_alloc_irq_vectors(pdev, 2, max_irqs, PCI_IRQ_MSIX); if
(nvec < 0)
  return nvec;

I know that. But still it's a bug. The common convention is that if
a 0-length array is passed to a function, it should not dereference
the pointer.

I will add one if check in the begining of irq_setup() to verify the
pointer and the nvec number.

Yes you can, but what for? This is an error anyways, and you don't care about early
return. So instead of adding and bearing extra logic, I'd just swap 2 lines of existing
code.

Problem with the code you had proposed is shown below:

./a.out

 i is 1
 i is 2
 i is 3
 i is 4
 i is 5
 i is 6
 i is 7
 i is 8
 i is 9
 i is 10
in done
lisatest ~

cat test3.c

#include<stdio.h>

main() {
        int i = 0, cur, nvec = 10;
        for (cur = 0; cur < 20; cur++) {
                if (i++ == nvec)
                        goto done;
                printf(" i is %d\n", i);
        }
done:                                                                                                                                                                                                                                                                                  
printf("in done\n");
}

So now it is because post increment operator in i++,
For that reason in the posposed code we will hit irqs[nvec], which may cause crash, as size of
irqs is nvec.

Now if we preincrement, then we will loop correctly, but nvec == 0 check will not happen.

Like here with preincrement in above code we are not hitting (i == nvec) .

./a.out

 i is 1
 i is 2
 i is 3
 i is 4
 i is 5
 i is 6
 i is 7
 i is 8
 i is 9
in done

So with preincrement if we want the check for nvec == 0, we will need the check with extra if condition
before the loop.