| Message ID | 1568283334-178380-8-git-send-email-linyunsheng@huawei.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
| Series | make node_to_cpumask_map() NUMA_NO_NODE aware | expand |
Hi, On Thu, Sep 12, 2019 at 06:15:33PM +0800, Yunsheng Lin wrote: > When passing the return value of dev_to_node() to cpumask_of_node() > without checking the node id if the node id is NUMA_NO_NODE, there is > global-out-of-bounds detected by KASAN. > > From the discussion [1], NUMA_NO_NODE really means no node affinity, > which also means all cpus should be usable. So the cpumask_of_node() > should always return all cpus online when user passes the node id > as NUMA_NO_NODE, just like similar semantic that page allocator handles > NUMA_NO_NODE. > > But we cannot really copy the page allocator logic. Simply because the > page allocator doesn't enforce the near node affinity. It just picks it > up as a preferred node but then it is free to fallback to any other numa > node. This is not the case here and node_to_cpumask_map will only restrict > to the particular node's cpus which would have really non deterministic > behavior depending on where the code is executed. So in fact we really > want to return cpu_online_mask for NUMA_NO_NODE. > > Since this arch was already NUMA_NO_NODE aware, this patch only changes > it to return cpu_online_mask and use NUMA_NO_NODE instead of "-1". > > [1] https://lore.kernel.org/patchwork/patch/1125789/ > Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com> > Suggested-by: Michal Hocko <mhocko@kernel.org> > --- > V3: Change to only handle NUMA_NO_NODE, and return cpu_online_mask > for NUMA_NO_NODE case, and change the commit log to better justify > the change. > --- > arch/mips/include/asm/mach-ip27/topology.h | 4 ++-- Nit: the subject says "mips:", but this patch only touches sgi-ip27 and loongson is updated as a separate patch. I don't see why both patches cannot be merged. Moreover, the whole set can be made as a single patch, IMHO. > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/mips/include/asm/mach-ip27/topology.h b/arch/mips/include/asm/mach-ip27/topology.h > index 965f079..04505e6 100644 > --- a/arch/mips/include/asm/mach-ip27/topology.h > +++ b/arch/mips/include/asm/mach-ip27/topology.h > @@ -15,8 +15,8 @@ struct cpuinfo_ip27 { > extern struct cpuinfo_ip27 sn_cpu_info[NR_CPUS]; > > #define cpu_to_node(cpu) (sn_cpu_info[(cpu)].p_nodeid) > -#define cpumask_of_node(node) ((node) == -1 ? \ > - cpu_all_mask : \ > +#define cpumask_of_node(node) ((node) == NUMA_NO_NODE ? \ > + cpu_online_mask : \ > &hub_data(node)->h_cpus) > struct pci_bus; > extern int pcibus_to_node(struct pci_bus *); > -- > 2.8.1 >
On 2019/9/15 13:49, Mike Rapoport wrote: > Hi, > > On Thu, Sep 12, 2019 at 06:15:33PM +0800, Yunsheng Lin wrote: >> When passing the return value of dev_to_node() to cpumask_of_node() >> without checking the node id if the node id is NUMA_NO_NODE, there is >> global-out-of-bounds detected by KASAN. >> >> From the discussion [1], NUMA_NO_NODE really means no node affinity, >> which also means all cpus should be usable. So the cpumask_of_node() >> should always return all cpus online when user passes the node id >> as NUMA_NO_NODE, just like similar semantic that page allocator handles >> NUMA_NO_NODE. >> >> But we cannot really copy the page allocator logic. Simply because the >> page allocator doesn't enforce the near node affinity. It just picks it >> up as a preferred node but then it is free to fallback to any other numa >> node. This is not the case here and node_to_cpumask_map will only restrict >> to the particular node's cpus which would have really non deterministic >> behavior depending on where the code is executed. So in fact we really >> want to return cpu_online_mask for NUMA_NO_NODE. >> >> Since this arch was already NUMA_NO_NODE aware, this patch only changes >> it to return cpu_online_mask and use NUMA_NO_NODE instead of "-1". >> >> [1] https://lore.kernel.org/patchwork/patch/1125789/ >> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com> >> Suggested-by: Michal Hocko <mhocko@kernel.org> >> --- >> V3: Change to only handle NUMA_NO_NODE, and return cpu_online_mask >> for NUMA_NO_NODE case, and change the commit log to better justify >> the change. >> --- >> arch/mips/include/asm/mach-ip27/topology.h | 4 ++-- > > Nit: the subject says "mips:", but this patch only touches sgi-ip27 and > loongson is updated as a separate patch. I don't see why both patches > cannot be merged. Moreover, the whole set can be made as a single patch, > IMHO. Thanks for reviewing. As this patchset touches a few files, which may has different maintainer. I am not sure if a separate patch for different arch will make the merging process easy, or a single patch will make the merging process easy? It can be made as a single patch if a single patch will make the merging process easy. > >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/mips/include/asm/mach-ip27/topology.h b/arch/mips/include/asm/mach-ip27/topology.h >> index 965f079..04505e6 100644 >> --- a/arch/mips/include/asm/mach-ip27/topology.h >> +++ b/arch/mips/include/asm/mach-ip27/topology.h >> @@ -15,8 +15,8 @@ struct cpuinfo_ip27 { >> extern struct cpuinfo_ip27 sn_cpu_info[NR_CPUS]; >> >> #define cpu_to_node(cpu) (sn_cpu_info[(cpu)].p_nodeid) >> -#define cpumask_of_node(node) ((node) == -1 ? \ >> - cpu_all_mask : \ >> +#define cpumask_of_node(node) ((node) == NUMA_NO_NODE ? \ >> + cpu_online_mask : \ >> &hub_data(node)->h_cpus) >> struct pci_bus; >> extern int pcibus_to_node(struct pci_bus *); >> -- >> 2.8.1 >> >
On Sun, Sep 15, 2019 at 02:13:51PM +0800, Yunsheng Lin wrote: > On 2019/9/15 13:49, Mike Rapoport wrote: > > Hi, > > > > On Thu, Sep 12, 2019 at 06:15:33PM +0800, Yunsheng Lin wrote: > >> When passing the return value of dev_to_node() to cpumask_of_node() > >> without checking the node id if the node id is NUMA_NO_NODE, there is > >> global-out-of-bounds detected by KASAN. > >> > >> From the discussion [1], NUMA_NO_NODE really means no node affinity, > >> which also means all cpus should be usable. So the cpumask_of_node() > >> should always return all cpus online when user passes the node id > >> as NUMA_NO_NODE, just like similar semantic that page allocator handles > >> NUMA_NO_NODE. > >> > >> But we cannot really copy the page allocator logic. Simply because the > >> page allocator doesn't enforce the near node affinity. It just picks it > >> up as a preferred node but then it is free to fallback to any other numa > >> node. This is not the case here and node_to_cpumask_map will only restrict > >> to the particular node's cpus which would have really non deterministic > >> behavior depending on where the code is executed. So in fact we really > >> want to return cpu_online_mask for NUMA_NO_NODE. > >> > >> Since this arch was already NUMA_NO_NODE aware, this patch only changes > >> it to return cpu_online_mask and use NUMA_NO_NODE instead of "-1". > >> > >> [1] https://lore.kernel.org/patchwork/patch/1125789/ > >> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com> > >> Suggested-by: Michal Hocko <mhocko@kernel.org> > >> --- > >> V3: Change to only handle NUMA_NO_NODE, and return cpu_online_mask > >> for NUMA_NO_NODE case, and change the commit log to better justify > >> the change. > >> --- > >> arch/mips/include/asm/mach-ip27/topology.h | 4 ++-- > > > > Nit: the subject says "mips:", but this patch only touches sgi-ip27 and > > loongson is updated as a separate patch. I don't see why both patches > > cannot be merged. Moreover, the whole set can be made as a single patch, > > IMHO. > > Thanks for reviewing. > > As this patchset touches a few files, which may has different maintainer. > I am not sure if a separate patch for different arch will make the merging > process easy, or a single patch will make the merging process easy? The set makes the same logical change to several definitions of cpumask_of_node(). It's appropriate to have all these changes in a single patch. > It can be made as a single patch if a single patch will make the merging > process easy. > > > > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/arch/mips/include/asm/mach-ip27/topology.h b/arch/mips/include/asm/mach-ip27/topology.h > >> index 965f079..04505e6 100644 > >> --- a/arch/mips/include/asm/mach-ip27/topology.h > >> +++ b/arch/mips/include/asm/mach-ip27/topology.h > >> @@ -15,8 +15,8 @@ struct cpuinfo_ip27 { > >> extern struct cpuinfo_ip27 sn_cpu_info[NR_CPUS]; > >> > >> #define cpu_to_node(cpu) (sn_cpu_info[(cpu)].p_nodeid) > >> -#define cpumask_of_node(node) ((node) == -1 ? \ > >> - cpu_all_mask : \ > >> +#define cpumask_of_node(node) ((node) == NUMA_NO_NODE ? \ > >> + cpu_online_mask : \ > >> &hub_data(node)->h_cpus) > >> struct pci_bus; > >> extern int pcibus_to_node(struct pci_bus *); > >> -- > >> 2.8.1 > >> > > >
On 2019/9/15 14:46, Mike Rapoport wrote: > On Sun, Sep 15, 2019 at 02:13:51PM +0800, Yunsheng Lin wrote: >> On 2019/9/15 13:49, Mike Rapoport wrote: >>> Hi, >>> >>> On Thu, Sep 12, 2019 at 06:15:33PM +0800, Yunsheng Lin wrote: >>>> When passing the return value of dev_to_node() to cpumask_of_node() >>>> without checking the node id if the node id is NUMA_NO_NODE, there is >>>> global-out-of-bounds detected by KASAN. >>>> >>>> From the discussion [1], NUMA_NO_NODE really means no node affinity, >>>> which also means all cpus should be usable. So the cpumask_of_node() >>>> should always return all cpus online when user passes the node id >>>> as NUMA_NO_NODE, just like similar semantic that page allocator handles >>>> NUMA_NO_NODE. >>>> >>>> But we cannot really copy the page allocator logic. Simply because the >>>> page allocator doesn't enforce the near node affinity. It just picks it >>>> up as a preferred node but then it is free to fallback to any other numa >>>> node. This is not the case here and node_to_cpumask_map will only restrict >>>> to the particular node's cpus which would have really non deterministic >>>> behavior depending on where the code is executed. So in fact we really >>>> want to return cpu_online_mask for NUMA_NO_NODE. >>>> >>>> Since this arch was already NUMA_NO_NODE aware, this patch only changes >>>> it to return cpu_online_mask and use NUMA_NO_NODE instead of "-1". >>>> >>>> [1] https://lore.kernel.org/patchwork/patch/1125789/ >>>> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com> >>>> Suggested-by: Michal Hocko <mhocko@kernel.org> >>>> --- >>>> V3: Change to only handle NUMA_NO_NODE, and return cpu_online_mask >>>> for NUMA_NO_NODE case, and change the commit log to better justify >>>> the change. >>>> --- >>>> arch/mips/include/asm/mach-ip27/topology.h | 4 ++-- >>> >>> Nit: the subject says "mips:", but this patch only touches sgi-ip27 and >>> loongson is updated as a separate patch. I don't see why both patches >>> cannot be merged. Moreover, the whole set can be made as a single patch, >>> IMHO. >> >> Thanks for reviewing. >> >> As this patchset touches a few files, which may has different maintainer. >> I am not sure if a separate patch for different arch will make the merging >> process easy, or a single patch will make the merging process easy? > > The set makes the same logical change to several definitions of > cpumask_of_node(). It's appropriate to have all these changes in a single > patch. Ok, thanks. Will have all these changes in a single patch. > >> It can be made as a single patch if a single patch will make the merging >> process easy. >> >>> >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/arch/mips/include/asm/mach-ip27/topology.h b/arch/mips/include/asm/mach-ip27/topology.h >>>> index 965f079..04505e6 100644 >>>> --- a/arch/mips/include/asm/mach-ip27/topology.h >>>> +++ b/arch/mips/include/asm/mach-ip27/topology.h >>>> @@ -15,8 +15,8 @@ struct cpuinfo_ip27 { >>>> extern struct cpuinfo_ip27 sn_cpu_info[NR_CPUS]; >>>> >>>> #define cpu_to_node(cpu) (sn_cpu_info[(cpu)].p_nodeid) >>>> -#define cpumask_of_node(node) ((node) == -1 ? \ >>>> - cpu_all_mask : \ >>>> +#define cpumask_of_node(node) ((node) == NUMA_NO_NODE ? \ >>>> + cpu_online_mask : \ >>>> &hub_data(node)->h_cpus) >>>> struct pci_bus; >>>> extern int pcibus_to_node(struct pci_bus *); >>>> -- >>>> 2.8.1 >>>> >>> >> >
diff --git a/arch/mips/include/asm/mach-ip27/topology.h b/arch/mips/include/asm/mach-ip27/topology.h index 965f079..04505e6 100644 --- a/arch/mips/include/asm/mach-ip27/topology.h +++ b/arch/mips/include/asm/mach-ip27/topology.h @@ -15,8 +15,8 @@ struct cpuinfo_ip27 { extern struct cpuinfo_ip27 sn_cpu_info[NR_CPUS]; #define cpu_to_node(cpu) (sn_cpu_info[(cpu)].p_nodeid) -#define cpumask_of_node(node) ((node) == -1 ? \ - cpu_all_mask : \ +#define cpumask_of_node(node) ((node) == NUMA_NO_NODE ? \ + cpu_online_mask : \ &hub_data(node)->h_cpus) struct pci_bus; extern int pcibus_to_node(struct pci_bus *);
When passing the return value of dev_to_node() to cpumask_of_node() without checking the node id if the node id is NUMA_NO_NODE, there is global-out-of-bounds detected by KASAN. From the discussion [1], NUMA_NO_NODE really means no node affinity, which also means all cpus should be usable. So the cpumask_of_node() should always return all cpus online when user passes the node id as NUMA_NO_NODE, just like similar semantic that page allocator handles NUMA_NO_NODE. But we cannot really copy the page allocator logic. Simply because the page allocator doesn't enforce the near node affinity. It just picks it up as a preferred node but then it is free to fallback to any other numa node. This is not the case here and node_to_cpumask_map will only restrict to the particular node's cpus which would have really non deterministic behavior depending on where the code is executed. So in fact we really want to return cpu_online_mask for NUMA_NO_NODE. Since this arch was already NUMA_NO_NODE aware, this patch only changes it to return cpu_online_mask and use NUMA_NO_NODE instead of "-1". [1] https://lore.kernel.org/patchwork/patch/1125789/ Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com> Suggested-by: Michal Hocko <mhocko@kernel.org> --- V3: Change to only handle NUMA_NO_NODE, and return cpu_online_mask for NUMA_NO_NODE case, and change the commit log to better justify the change. --- arch/mips/include/asm/mach-ip27/topology.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)