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

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: <875xv7idcb.ffs@tglx>
Date: Tue, 21 May 2024 14:00:52 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Dongli Zhang <dongli.zhang@...cle.com>, x86@...nel.org
Cc: mingo@...hat.com, dave.hansen@...ux.intel.com, hpa@...or.com,
 joe.jin@...cle.com, linux-kernel@...r.kernel.org,
 virtualization@...ts.linux.dev, Borislav Petkov <bp@...en8.de>
Subject: Re: [PATCH 1/1] x86/vector: Fix vector leak during CPU offline

On Wed, May 15 2024 at 12:51, Dongli Zhang wrote:
> On 5/13/24 3:46 PM, Thomas Gleixner wrote:
>> So yes, moving the invocation of irq_force_complete_move() before the
>> irq_needs_fixup() call makes sense, but it wants this to actually work
>> correctly:
>> @@ -1097,10 +1098,11 @@ void irq_force_complete_move(struct irq_
>>  		goto unlock;
>>  
>>  	/*
>> -	 * If prev_vector is empty, no action required.
>> +	 * If prev_vector is empty or the descriptor was previously
>> +	 * not on the outgoing CPU no action required.
>>  	 */
>>  	vector = apicd->prev_vector;
>> -	if (!vector)
>> +	if (!vector || apicd->prev_cpu != smp_processor_id())
>>  		goto unlock;
>>  
>
> The above may not work. migrate_one_irq() relies on irq_force_complete_move() to
> always reclaim the apicd->prev_vector. Otherwise, the call of
> irq_do_set_affinity() later may return -EBUSY.

You're right. But that still can be handled in irq_force_complete_move()
with a single unconditional invocation in migrate_one_irq():

	cpu = smp_processor_id();
	if (!vector || (apicd->cur_cpu != cpu && apicd->prev_cpu != cpu))
		goto unlock;

because there are only two cases when a cleanup is required:

   1) The outgoing CPU is the current target

   2) The outgoing CPU was the previous target

No?

Thanks,

        tglx