linux-kernel - Re: [PATCH 2/3] x86/vector: Replace IRQ_MOVE_CLEANUP

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: <20230620072047.GS4253@hirez.programming.kicks-ass.net>
Date:   Tue, 20 Jun 2023 09:20:47 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Xin Li <xin3.li@...el.com>
Cc:     linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org,
        iommu@...ts.linux.dev, linux-hyperv@...r.kernel.org,
        linux-perf-users@...r.kernel.org, x86@...nel.org,
        tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
        dave.hansen@...ux.intel.com, hpa@...or.com, steve.wahl@....com,
        mike.travis@....com, dimitri.sivanich@....com,
        russ.anderson@....com, dvhart@...radead.org, andy@...radead.org,
        joro@...tes.org, suravee.suthikulpanit@....com, will@...nel.org,
        robin.murphy@....com, kys@...rosoft.com, haiyangz@...rosoft.com,
        wei.liu@...nel.org, decui@...rosoft.com, dwmw2@...radead.org,
        baolu.lu@...ux.intel.com, acme@...nel.org, mark.rutland@....com,
        alexander.shishkin@...ux.intel.com, jolsa@...nel.org,
        namhyung@...nel.org, irogers@...gle.com, adrian.hunter@...el.com,
        seanjc@...gle.com, jiangshanlai@...il.com, jgg@...pe.ca,
        yangtiezhu@...ngson.cn
Subject: Re: [PATCH 2/3] x86/vector: Replace IRQ_MOVE_CLEANUP_VECTOR with a
 timer callback

On Mon, Jun 19, 2023 at 04:16:10PM -0700, Xin Li wrote:

> +/*
> + * Called with vector_lock held
> + */

Instead of comments like that, I tend to add a lockdep_assert*()
statement to the same effect. Which unlike comment actually validate the
claim and since it's code it tends to not go stale like comments do.

> +static void __vector_cleanup(struct vector_cleanup *cl, bool check_irr)
>  {
>  	struct apic_chip_data *apicd;
>  	struct hlist_node *tmp;
> +	bool rearm = false;

	lockdep_assert_held(&vector_lock);

> +	hlist_for_each_entry_safe(apicd, tmp, &cl->head, clist) {
>  		unsigned int irr, vector = apicd->prev_vector;
>  
>  		/*
>  		 * Paranoia: Check if the vector that needs to be cleaned
> +		 * up is registered at the APICs IRR. That's clearly a
> +		 * hardware issue if the vector arrived on the old target
> +		 * _after_ interrupts were disabled above. Keep @apicd
> +		 * on the list and schedule the timer again to give the CPU
> +		 * a chance to handle the pending interrupt.
> +		 *
> +		 * Do not check IRR when called from lapic_offline(), because
> +		 * fixup_irqs() was just called to scan IRR for set bits and
> +		 * forward them to new destination CPUs via IPIs.
>  		 */
> +		irr = check_irr ? apic_read(APIC_IRR + (vector / 32 * 0x10)) : 0;
>  		if (irr & (1U << (vector % 32))) {
> +			pr_warn_once("Moved interrupt pending in old target APIC %u\n", apicd->irq);
> +			rearm = true;
>  			continue;
>  		}
>  		free_moved_vector(apicd);
>  	}
>  
> +	/*
> +	 * Must happen under vector_lock to make the timer_pending() check
> +	 * in __vector_schedule_cleanup() race free against the rearm here.
> +	 */
> +	if (rearm)
> +		mod_timer(&cl->timer, jiffies + 1);
> +}
> +
> +static void vector_cleanup_callback(struct timer_list *tmr)
> +{
> +	struct vector_cleanup *cl = container_of(tmr, typeof(*cl), timer);
> +
> +	/* Prevent vectors vanishing under us */
> +	raw_spin_lock_irq(&vector_lock);
> +	__vector_cleanup(cl, true);
> +	raw_spin_unlock_irq(&vector_lock);
>  }