Interfaces for setting affinity hint [1] have a rather surprising contract
with the caller [2]. The cpumask memory passed as a hint must be kept
alive, and stay unchanged, until the affinity hint gets updated again or
cleared. Otherwise reading out /proc/irq/N/affinity_hint, which
dereferences the pointer to the cpumask holding the hint, will access
potentially invalid memory.
This forces callers to allocate the affinity hint on heap and mange its
lifetime. That is unless they fall into the category of cpumask helpers -
get_cpu_mask, cpu_mask_of[_node] - which produce a pointer to a mask
constant.
Change the affinity hint interfaces to make a copy of the affinity
hint. This way the operation from the caller point of view becomes a "set
and forget." Also there is no catch any more that the passed cpumask can't
be allocated on stack [3].
No updates are needed to the existing call sites right away. They can be
adapted to simplify resource management on their side on their own
schedule.
[1] commit 65c7cdedeb30 ("genirq: Provide new interfaces for affinity hints")
[2] commit 18a048370b06 ("net: mana: Fix accessing freed irq affinity_hint")
[3] commit 8deec94c6040 ("net: stmmac: set IRQ affinity hint for multi MSI vectors")
Cc: Xuan Zhuo <[email protected]>
Signed-off-by: Jakub Sitnicki <[email protected]>
---
include/linux/interrupt.h | 4 ++++
include/linux/irq.h | 1 +
kernel/irq/irqdesc.c | 32 ++++++++++++++++++++------------
kernel/irq/manage.c | 11 ++++-------
kernel/irq/proc.c | 22 ++++++----------------
5 files changed, 35 insertions(+), 35 deletions(-)
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 4a1dc88ddbff..14ea8d2a39a5 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -320,6 +320,8 @@ extern int __irq_apply_affinity_hint(unsigned int irq, const struct cpumask *m,
* @m: cpumask pointer (NULL to clear the hint)
*
* Updates the affinity hint, but does not change the affinity of the interrupt.
+ *
+ * Memory pointed by @m is not accessed after the call returns.
*/
static inline int
irq_update_affinity_hint(unsigned int irq, const struct cpumask *m)
@@ -335,6 +337,8 @@ irq_update_affinity_hint(unsigned int irq, const struct cpumask *m)
*
* Updates the affinity hint and if @m is not NULL it applies it as the
* affinity of that interrupt.
+ *
+ * Memory pointed by @m is not accessed after the call returns.
*/
static inline int
irq_set_affinity_and_hint(unsigned int irq, const struct cpumask *m)
diff --git a/include/linux/irq.h b/include/linux/irq.h
index d8a6fdce9373..329c7c7be5a1 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -153,6 +153,7 @@ struct irq_common_data {
struct msi_desc *msi_desc;
#ifdef CONFIG_SMP
cpumask_var_t affinity;
+ cpumask_var_t affinity_hint;
#endif
#ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
cpumask_var_t effective_affinity;
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 27ca1c866f29..fb443b702f22 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -55,26 +55,33 @@ static int alloc_masks(struct irq_desc *desc, int node)
{
if (!zalloc_cpumask_var_node(&desc->irq_common_data.affinity,
GFP_KERNEL, node))
- return -ENOMEM;
+ goto err_affinity;
+ if (!zalloc_cpumask_var_node(&desc->irq_common_data.affinity_hint,
+ GFP_KERNEL, node))
+ goto err_affinity_hint;
#ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
if (!zalloc_cpumask_var_node(&desc->irq_common_data.effective_affinity,
- GFP_KERNEL, node)) {
- free_cpumask_var(desc->irq_common_data.affinity);
- return -ENOMEM;
- }
+ GFP_KERNEL, node))
+ goto err_effective_affinity;
#endif
#ifdef CONFIG_GENERIC_PENDING_IRQ
- if (!zalloc_cpumask_var_node(&desc->pending_mask, GFP_KERNEL, node)) {
-#ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
- free_cpumask_var(desc->irq_common_data.effective_affinity);
-#endif
- free_cpumask_var(desc->irq_common_data.affinity);
- return -ENOMEM;
- }
+ if (!zalloc_cpumask_var_node(&desc->pending_mask, GFP_KERNEL, node))
+ goto err_pending_mask;
#endif
return 0;
+
+err_pending_mask:
+#ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
+ free_cpumask_var(desc->irq_common_data.effective_affinity);
+#endif
+err_effective_affinity:
+ free_cpumask_var(desc->irq_common_data.affinity_hint);
+err_affinity_hint:
+ free_cpumask_var(desc->irq_common_data.affinity);
+err_affinity:
+ return -ENOMEM;
}
static void desc_smp_init(struct irq_desc *desc, int node,
@@ -391,6 +398,7 @@ static void free_masks(struct irq_desc *desc)
free_cpumask_var(desc->pending_mask);
#endif
free_cpumask_var(desc->irq_common_data.affinity);
+ free_cpumask_var(desc->irq_common_data.affinity_hint);
#ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
free_cpumask_var(desc->irq_common_data.effective_affinity);
#endif
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index d309ba84e08a..573560645add 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -506,7 +506,10 @@ int __irq_apply_affinity_hint(unsigned int irq, const struct cpumask *m,
if (!desc)
return -EINVAL;
- desc->affinity_hint = m;
+ if (m)
+ cpumask_copy(desc->irq_common_data.affinity_hint, m);
+ else
+ cpumask_clear(desc->irq_common_data.affinity_hint);
irq_put_desc_unlock(desc, flags);
if (m && setaffinity)
__irq_set_affinity(irq, m, false);
@@ -1916,12 +1919,6 @@ static struct irqaction *__free_irq(struct irq_desc *desc, void *dev_id)
irq_shutdown(desc);
}
-#ifdef CONFIG_SMP
- /* make sure affinity_hint is cleaned up */
- if (WARN_ON_ONCE(desc->affinity_hint))
- desc->affinity_hint = NULL;
-#endif
-
raw_spin_unlock_irqrestore(&desc->lock, flags);
/*
* Drop bus_lock here so the changes which were done in the chip
diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
index 623b8136e9af..733324bbfb91 100644
--- a/kernel/irq/proc.c
+++ b/kernel/irq/proc.c
@@ -39,6 +39,7 @@ static struct proc_dir_entry *root_irq_dir;
enum {
AFFINITY,
AFFINITY_LIST,
+ AFFINITY_HINT,
EFFECTIVE,
EFFECTIVE_LIST,
};
@@ -57,6 +58,9 @@ static int show_irq_affinity(int type, struct seq_file *m)
mask = desc->pending_mask;
#endif
break;
+ case AFFINITY_HINT:
+ mask = desc->irq_common_data.affinity_hint;
+ break;
case EFFECTIVE:
case EFFECTIVE_LIST:
#ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
@@ -73,6 +77,7 @@ static int show_irq_affinity(int type, struct seq_file *m)
seq_printf(m, "%*pbl\n", cpumask_pr_args(mask));
break;
case AFFINITY:
+ case AFFINITY_HINT:
case EFFECTIVE:
seq_printf(m, "%*pb\n", cpumask_pr_args(mask));
break;
@@ -82,22 +87,7 @@ static int show_irq_affinity(int type, struct seq_file *m)
static int irq_affinity_hint_proc_show(struct seq_file *m, void *v)
{
- struct irq_desc *desc = irq_to_desc((long)m->private);
- unsigned long flags;
- cpumask_var_t mask;
-
- if (!zalloc_cpumask_var(&mask, GFP_KERNEL))
- return -ENOMEM;
-
- raw_spin_lock_irqsave(&desc->lock, flags);
- if (desc->affinity_hint)
- cpumask_copy(mask, desc->affinity_hint);
- raw_spin_unlock_irqrestore(&desc->lock, flags);
-
- seq_printf(m, "%*pb\n", cpumask_pr_args(mask));
- free_cpumask_var(mask);
-
- return 0;
+ return show_irq_affinity(AFFINITY_HINT, m);
}
int no_irq_affinity;
--
2.41.0
Hi Jakub,
kernel test robot noticed the following build warnings:
[auto build test WARNING on tip/irq/core]
[also build test WARNING on linus/master v6.6-rc7 next-20231025]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jakub-Sitnicki/genirq-Own-affinity-hint/20231026-001606
base: tip/irq/core
patch link: https://lore.kernel.org/r/20231025141517.375378-1-jakub%40cloudflare.com
patch subject: [PATCH] genirq: Own affinity hint
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20231026/[email protected]/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231026/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
kernel/irq/irqdesc.c: In function 'alloc_masks':
>> kernel/irq/irqdesc.c:79:1: warning: label 'err_effective_affinity' defined but not used [-Wunused-label]
79 | err_effective_affinity:
| ^~~~~~~~~~~~~~~~~~~~~~
>> kernel/irq/irqdesc.c:75:1: warning: label 'err_pending_mask' defined but not used [-Wunused-label]
75 | err_pending_mask:
| ^~~~~~~~~~~~~~~~
vim +/err_effective_affinity +79 kernel/irq/irqdesc.c
68
69 #ifdef CONFIG_GENERIC_PENDING_IRQ
70 if (!zalloc_cpumask_var_node(&desc->pending_mask, GFP_KERNEL, node))
71 goto err_pending_mask;
72 #endif
73 return 0;
74
> 75 err_pending_mask:
76 #ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
77 free_cpumask_var(desc->irq_common_data.effective_affinity);
78 #endif
> 79 err_effective_affinity:
80 free_cpumask_var(desc->irq_common_data.affinity_hint);
81 err_affinity_hint:
82 free_cpumask_var(desc->irq_common_data.affinity);
83 err_affinity:
84 return -ENOMEM;
85 }
86
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Wed, Oct 25 2023 at 16:15, Jakub Sitnicki wrote:
> @@ -55,26 +55,33 @@ static int alloc_masks(struct irq_desc *desc, int node)
> {
> if (!zalloc_cpumask_var_node(&desc->irq_common_data.affinity,
> GFP_KERNEL, node))
> - return -ENOMEM;
> + goto err_affinity;
> + if (!zalloc_cpumask_var_node(&desc->irq_common_data.affinity_hint,
> + GFP_KERNEL, node))
This gets allocated for every interrupt descriptor but only a few or
even none will ever use it. Seriously no.
> + goto err_affinity_hint;
>
> #ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
> if (!zalloc_cpumask_var_node(&desc->irq_common_data.effective_affinity,
> - GFP_KERNEL, node)) {
> - free_cpumask_var(desc->irq_common_data.affinity);
> - return -ENOMEM;
> - }
> + GFP_KERNEL, node))
> + goto err_effective_affinity;
> #endif
>
> #ifdef CONFIG_GENERIC_PENDING_IRQ
> - if (!zalloc_cpumask_var_node(&desc->pending_mask, GFP_KERNEL, node)) {
> -#ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
> - free_cpumask_var(desc->irq_common_data.effective_affinity);
> -#endif
> - free_cpumask_var(desc->irq_common_data.affinity);
> - return -ENOMEM;
> - }
> + if (!zalloc_cpumask_var_node(&desc->pending_mask, GFP_KERNEL, node))
> + goto err_pending_mask;
> #endif
> return 0;
> +
> +err_pending_mask:
How is this supposed to compile with CONFIG_GENERIC_PENDING_IRQ=n ?
> +#ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
> + free_cpumask_var(desc->irq_common_data.effective_affinity);
> +#endif
> +err_effective_affinity:
and this with CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK=n ?
Thanks,
tglx
Hi Jakub,
kernel test robot noticed the following build warnings:
[auto build test WARNING on tip/irq/core]
[also build test WARNING on linus/master v6.6-rc7 next-20231026]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jakub-Sitnicki/genirq-Own-affinity-hint/20231026-001606
base: tip/irq/core
patch link: https://lore.kernel.org/r/20231025141517.375378-1-jakub%40cloudflare.com
patch subject: [PATCH] genirq: Own affinity hint
config: mips-malta_kvm_defconfig (https://download.01.org/0day-ci/archive/20231026/[email protected]/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231026/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> kernel/irq/irqdesc.c:75:1: warning: unused label 'err_pending_mask' [-Wunused-label]
err_pending_mask:
^~~~~~~~~~~~~~~~~
1 warning generated.
vim +/err_pending_mask +75 kernel/irq/irqdesc.c
68
69 #ifdef CONFIG_GENERIC_PENDING_IRQ
70 if (!zalloc_cpumask_var_node(&desc->pending_mask, GFP_KERNEL, node))
71 goto err_pending_mask;
72 #endif
73 return 0;
74
> 75 err_pending_mask:
76 #ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
77 free_cpumask_var(desc->irq_common_data.effective_affinity);
78 #endif
79 err_effective_affinity:
80 free_cpumask_var(desc->irq_common_data.affinity_hint);
81 err_affinity_hint:
82 free_cpumask_var(desc->irq_common_data.affinity);
83 err_affinity:
84 return -ENOMEM;
85 }
86
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Wed, Oct 25, 2023 at 10:10 PM +02, Thomas Gleixner wrote:
> On Wed, Oct 25 2023 at 16:15, Jakub Sitnicki wrote:
>> @@ -55,26 +55,33 @@ static int alloc_masks(struct irq_desc *desc, int node)
>> {
>> if (!zalloc_cpumask_var_node(&desc->irq_common_data.affinity,
>> GFP_KERNEL, node))
>> - return -ENOMEM;
>> + goto err_affinity;
>> + if (!zalloc_cpumask_var_node(&desc->irq_common_data.affinity_hint,
>> + GFP_KERNEL, node))
>
> This gets allocated for every interrupt descriptor but only a few or
> even none will ever use it. Seriously no.
Makes sense. I wanted to start the simplest possible approach first.
I expect allocating it lazily will be more involved - have to cover the
not-allocated case.
But thinking about it some more - perhaps what makes more sense is, for
irq_set_affinity[_and]_hint users who don't want to bother with managing
a cpumask on their side, to switch to irq_set_affinity.
That interface doesn't require the cpumask to outlive the call, AFAICT.
After all, setting the affinity hint only buys you an ability to read it
out from /proc/irq/<N>/affinity_hint. Same information is available from
/proc/irq/<N>/smp_affinity. Please set me straight, if I'm missing
something here.
>
>> + goto err_affinity_hint;
>>
>> #ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
>> if (!zalloc_cpumask_var_node(&desc->irq_common_data.effective_affinity,
>> - GFP_KERNEL, node)) {
>> - free_cpumask_var(desc->irq_common_data.affinity);
>> - return -ENOMEM;
>> - }
>> + GFP_KERNEL, node))
>> + goto err_effective_affinity;
>> #endif
>>
>> #ifdef CONFIG_GENERIC_PENDING_IRQ
>> - if (!zalloc_cpumask_var_node(&desc->pending_mask, GFP_KERNEL, node)) {
>> -#ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
>> - free_cpumask_var(desc->irq_common_data.effective_affinity);
>> -#endif
>> - free_cpumask_var(desc->irq_common_data.affinity);
>> - return -ENOMEM;
>> - }
>> + if (!zalloc_cpumask_var_node(&desc->pending_mask, GFP_KERNEL, node))
>> + goto err_pending_mask;
>> #endif
>> return 0;
>> +
>> +err_pending_mask:
>
> How is this supposed to compile with CONFIG_GENERIC_PENDING_IRQ=n ?
>
>> +#ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
>> + free_cpumask_var(desc->irq_common_data.effective_affinity);
>> +#endif
>> +err_effective_affinity:
>
> and this with CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK=n ?
If it wasn't for the unused label (my bad) both cases LGTM.
But I double checked. If I force feed it through the preprocessor:
* CONFIG_GENERIC_PENDING_IRQ=n :
static int alloc_masks(struct irq_desc *desc, int node)
{
if (!zalloc_cpumask_var_node(&desc->irq_common_data.affinity,
((( gfp_t)(0x400u|0x800u)) | (( gfp_t)0x40u) | (( gfp_t)0x80u)), node))
goto err_affinity;
if (!zalloc_cpumask_var_node(&desc->irq_common_data.affinity_hint,
((( gfp_t)(0x400u|0x800u)) | (( gfp_t)0x40u) | (( gfp_t)0x80u)), node))
goto err_affinity_hint;
if (!zalloc_cpumask_var_node(&desc->irq_common_data.effective_affinity,
((( gfp_t)(0x400u|0x800u)) | (( gfp_t)0x40u) | (( gfp_t)0x80u)), node))
goto err_effective_affinity;
return 0;
err_pending_mask:
free_cpumask_var(desc->irq_common_data.effective_affinity);
err_effective_affinity:
free_cpumask_var(desc->irq_common_data.affinity_hint);
err_affinity_hint:
free_cpumask_var(desc->irq_common_data.affinity);
err_affinity:
return -12;
}
* CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK=n :
static int alloc_masks(struct irq_desc *desc, int node)
{
if (!zalloc_cpumask_var_node(&desc->irq_common_data.affinity,
((( gfp_t)(0x400u|0x800u)) | (( gfp_t)0x40u) | (( gfp_t)0x80u)), node))
goto err_affinity;
if (!zalloc_cpumask_var_node(&desc->irq_common_data.affinity_hint,
((( gfp_t)(0x400u|0x800u)) | (( gfp_t)0x40u) | (( gfp_t)0x80u)), node))
goto err_affinity_hint;
if (!zalloc_cpumask_var_node(&desc->pending_mask,
((( gfp_t)(0x400u|0x800u)) | (( gfp_t)0x40u) | (( gfp_t)0x80u)), node))
goto err_pending_mask;
return 0;
err_pending_mask:
err_effective_affinity:
free_cpumask_var(desc->irq_common_data.affinity_hint);
err_affinity_hint:
free_cpumask_var(desc->irq_common_data.affinity);
err_affinity:
return -12;
}
I did forget, however, to clean up the "old" affinity_hint field from
irq_desc struct.
Anyway - not planning on sending a v2, unless you see value in doing a
lazy allocation of the hint from within irq subsys.
Thanks for feedback.