linux-kernel - Re: [PATCH v3] PCI: pci_call_probe: call local_pci

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]
Message-ID: <ea5a5c52-69ca-9504-dd80-a90c3000d9c6@loongson.cn>
Date: Wed, 24 Jul 2024 15:05:32 +0800
From: Hongchen Zhang <zhanghongchen@...ngson.cn>
To: Ethan Zhao <haifeng.zhao@...ux.intel.com>,
 Markus Elfring <Markus.Elfring@....de>, Bjorn Helgaas <bhelgaas@...gle.com>
Cc: Alex Belits <abelits@...vell.com>,
 "Peter Zijlstra (Intel)" <peterz@...radead.org>,
 Nitesh Narayan Lal <nitesh@...hat.com>,
 Frederic Weisbecker <frederic@...nel.org>, linux-pci@...r.kernel.org,
 linux-kernel@...r.kernel.org, loongarch@...ts.linux.dev,
 stable@...r.kernel.org, Huacai Chen <chenhuacai@...ngson.cn>
Subject: Re: [PATCH v3] PCI: pci_call_probe: call local_pci_probe() when
 selected cpu is offline

On 2024/7/24 下午2:40, Ethan Zhao wrote:
> On 7/24/2024 11:09 AM, Hongchen Zhang wrote:
>> Hi Ethan,
>>
>> On 2024/7/24 上午10:47, Ethan Zhao wrote:
>>> On 7/24/2024 9:58 AM, Hongchen Zhang wrote:
>>>> Hi Ethan,
>>>> On 2024/7/22 PM 3:39, Ethan Zhao wrote:
>>>>>
>>>>> On 6/13/2024 3:42 PM, Hongchen Zhang wrote:
>>>>>> Call work_on_cpu(cpu, fn, arg) in pci_call_probe() while the argument
>>>>>> @cpu is a offline cpu would cause system stuck forever.
>>>>>>
>>>>>> This can be happen if a node is online while all its CPUs are
>>>>>> offline (We can use "maxcpus=1" without "nr_cpus=1" to reproduce it).
>>>>>>
>>>>>> So, in the above case, let pci_call_probe() call local_pci_probe()
>>>>>> instead of work_on_cpu() when the best selected cpu is offline.
>>>>>>
>>>>>> Fixes: 69a18b18699b ("PCI: Restrict probe functions to 
>>>>>> housekeeping CPUs")
>>>>>> Cc: <stable@...r.kernel.org>
>>>>>> Signed-off-by: Huacai Chen <chenhuacai@...ngson.cn>
>>>>>> Signed-off-by: Hongchen Zhang <zhanghongchen@...ngson.cn>
>>>>>> ---
>>>>>> v2 -> v3: Modify commit message according to Markus's suggestion
>>>>>> v1 -> v2: Add a method to reproduce the problem
>>>>>> ---
>>>>>>   drivers/pci/pci-driver.c | 2 +-
>>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
>>>>>> index af2996d0d17f..32a99828e6a3 100644
>>>>>> --- a/drivers/pci/pci-driver.c
>>>>>> +++ b/drivers/pci/pci-driver.c
>>>>>> @@ -386,7 +386,7 @@ static int pci_call_probe(struct pci_driver 
>>>>>> *drv, struct pci_dev *dev,
>>>>>>           free_cpumask_var(wq_domain_mask);
>>>>>>       }
>>>>>> -    if (cpu < nr_cpu_ids)
>>>>>
>>>>> Why not choose the right cpu to callwork_on_cpu() ? the one that is 
>>>>> online. Thanks, Ethan
>>>> Yes, let housekeeping_cpumask() return online cpu is a good idea, 
>>>> but it may be changed by command line. so the simplest way is to 
>>>> call local_pci_probe when the best selected cpu is offline.
>>>
>>> Hmm..... housekeeping_cpumask() should never return offline CPU, so
>>> I guess you didn't hit issue with the CPU isolation, but the following
>>> code seems not good.
>> The issue is the dev node is online but the best selected cpu is 
>> offline, so it seems that there is no better way to directly set the 
>> cpu to nr_cpu_ids.
> 
> I mean where the bug is ? you should debug more about that.
> just add one cpu_online(cpu) check there might suggest there
> is bug in the cpu selection stage.
> 
> 
> if (node < 0 || node >= MAX_NUMNODES || !node_online(node) ||
>          pci_physfn_is_probed(dev)) {
>          cpu = nr_cpu_ids; // <----- if you hit here, then 
> local_pci_probe() should be called.
>      } else {
>          cpumask_var_t wq_domain_mask;
> 
>          if (!zalloc_cpumask_var(&wq_domain_mask, GFP_KERNEL)) {
>              error = -ENOMEM;
>              goto out;
>          }
>          cpumask_and(wq_domain_mask,
>                  housekeeping_cpumask(HK_TYPE_WQ),
>                  housekeeping_cpumask(HK_TYPE_DOMAIN));
> 
>          cpu = cpumask_any_and(cpumask_of_node(node),
>                        wq_domain_mask);
>          free_cpumask_var(wq_domain_mask);
>                  // <----- if you hit here, then work_on_cpu(cpu, 
> local_pci_probe, &ddi) should be called.
Yes, but if the offline cpu is selected, local_pci_probe should be called.
>                  // do you mean there one offline cpu is selected ?
Yes, the offline cpu is selected.
> 
>      }
> 
>      if (cpu < nr_cpu_ids)
>          error = work_on_cpu(cpu, local_pci_probe, &ddi);
>      else
>          error = local_pci_probe(&ddi);
> 
> 
> Thanks,
> Ethan
> 
>>>
>>> ...
>>>
>>> if (node < 0 || node >= MAX_NUMNODES || !node_online(node) ||
>>>          pci_physfn_is_probed(dev)) {
>>>          cpu = nr_cpu_ids;
>>>      } else {
>>>
>>> ....
>>>
>>> perhaps you could change the logic there and fix it  ?
>>>
>>> Thanks
>>> Ethan
>>>
>>>
>>>
>>>>>
>>>>>> +    if ((cpu < nr_cpu_ids) && cpu_online(cpu))
>>>>>>           error = work_on_cpu(cpu, local_pci_probe, &ddi);
>>>>>>       else
>>>>>>           error = local_pci_probe(&ddi);
>>>>
>>>>
>>
>>


-- 
Best Regards
Hongchen Zhang