On Tue, Dec 12, 2023 at 05:18:31PM +0000, Souradeep Chakrabarti wrote:
>
>
> >-----Original Message-----
> >From: Yury Norov <yury.no...@gmail.com>
> >Sent: Tuesday, December 12, 2023 10:04 PM
> >To: Souradeep Chakrabarti <schakraba...@linux.microsoft.com>
> >Cc: KY Srinivasan <k...@microsoft.com>; Haiyang Zhang
> ><haiya...@microsoft.com>; wei....@kernel.org; Dexuan Cui
> ><de...@microsoft.com>; da...@davemloft.net; eduma...@google.com;
> >k...@kernel.org; pab...@redhat.com; Long Li <lon...@microsoft.com>;
> >l...@kernel.org; cai.huoq...@linux.dev; ssen...@linux.microsoft.com;
> >vkuzn...@redhat.com; t...@linutronix.de; linux-hyperv@vger.kernel.org;
> >net...@vger.kernel.org; linux-ker...@vger.kernel.org; linux-
> >r...@vger.kernel.org; Souradeep Chakrabarti <schakraba...@microsoft.com>;
> >Paul Rosswurm <paul...@microsoft.com>
> >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.no...@gmail.com. Learn why this is important at
> >https://aka.ms/LearnAboutSenderIdentification ]
> >
> >> > > > > + 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));
> >> > > > > + 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.
> >> > >
> >> > > 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.
OK, I see. Then just separate it:
for (cur = 0; cur < 20; cur++) {
if (i == nvec)
goto done;
printf(" i is %d\n", i++);
