linux-kernel - Re: [PATCH v3 3/3] vfio_pci: make use of update_irq

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.21.1908151833010.1908@nanos.tec.linutronix.de>
Date:   Thu, 15 Aug 2019 18:45:27 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Ben Luo <luoben@...ux.alibaba.com>
cc:     alex.williamson@...hat.com, linux-kernel@...r.kernel.org,
        tao.ma@...ux.alibaba.com, gerry@...ux.alibaba.com,
        nanhai.zou@...ux.alibaba.com, linyunsheng@...wei.com
Subject: Re: [PATCH v3 3/3] vfio_pci: make use of update_irq_devid and optimize
 irq ops

On Thu, 15 Aug 2019, Ben Luo wrote:
>  	if (vdev->ctx[vector].trigger) {
> -		free_irq(irq, vdev->ctx[vector].trigger);
> -		irq_bypass_unregister_producer(&vdev->ctx[vector].producer);
> -		kfree(vdev->ctx[vector].name);
> -		eventfd_ctx_put(vdev->ctx[vector].trigger);
> -		vdev->ctx[vector].trigger = NULL;
> +		if (vdev->ctx[vector].trigger == trigger) {
> +			irq_bypass_unregister_producer(&vdev->ctx[vector].producer);

What's the point of unregistering the producer, just to register it again below?

> +		} else if (trigger) {
> +			ret = update_irq_devid(irq,
> +					vdev->ctx[vector].trigger, trigger);
> +			if (unlikely(ret)) {
> +				dev_info(&pdev->dev,
> +					 "update_irq_devid %d (token %p) fails: %d\n",

s/fails/failed/

> +					 irq, vdev->ctx[vector].trigger, ret);
> +				eventfd_ctx_put(trigger);
> +				return ret;
> +			}

This lacks any form of comment why this is correct. dev_id is updated and
the producer with the old token is still registered.

> +			irq_bypass_unregister_producer(&vdev->ctx[vector].producer);

Now it's unregistered.

> +			eventfd_ctx_put(vdev->ctx[vector].trigger);
> +			vdev->ctx[vector].producer.token = trigger;

The token is updated and then it's newly registered below.

> +			vdev->ctx[vector].trigger = trigger;
> -	vdev->ctx[vector].producer.token = trigger;
> -	vdev->ctx[vector].producer.irq = irq;
> +	/* always update irte for posted mode */
>  	ret = irq_bypass_register_producer(&vdev->ctx[vector].producer);
>  	if (unlikely(ret))
>  		dev_info(&pdev->dev,
>  		"irq bypass producer (token %p) registration fails: %d\n",
>  		vdev->ctx[vector].producer.token, ret);

I know this code already existed, but again this looks inconsistent. If the
registration fails then a subsequent update will try to unregister a not
registered producer. Does not make any sense to me.

Thanks,

	tglx