Re: [PATCH 1/1] x86/vector: Fix vector leak during CPU offline

[Dave Hansen]

On 5/10/24 12:06, Dongli Zhang wrote:
>  	} else {
> +		/*
> +		 * This call borrows from the comments and implementation
> +		 * of apic_update_vector(): "If the target CPU is offline
> +		 * then the regular release mechanism via the cleanup
> +		 * vector is not possible and the vector can be immediately
> +		 * freed in the underlying matrix allocator.".
> +		 */
> +		irq_matrix_free(vector_matrix, apicd->prev_cpu,
> +				apicd->prev_vector, apicd->is_managed);
>  		apicd->prev_vector = 0;
>  	}

I know it's just two sites, but I'd much rather spend the space on a
helper function than a copy-and-pasted comment.  Wouldn't something like
this make it like stupidly obvious what's going on:

        if (cpu_online(cpu)) {
		...
        } else {
		irq_matrix_free_offline(apicd->prev_cpu,
					apicd->prev_vector,
					apicd->is_managed);
                apicd->prev_vector = 0;
        }

/* Free a vector when the target CPU is offline */
static void irq_matrix_free_offline(...)
{
	lockdep_assert_held(&vector_lock);
	WARN_ON_ONCE(!cpu_offline(apicd->prev_cpu));

	/*
	 * The regular release mechanism via the cleanup vector is not
	 * possible.  Immediately free the vector in the underlying
	 * matrix allocator.
 	 */
	irq_matrix_free(&whatever, cpu, vector, managed);
}

It would also be rather hard to screw up even if someone called it on an
online CPU because you'd get a nice warning.