linux-kernel - Re: [PATCH v3 0/4] Introduce pcim_alloc_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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Fri, 19 Feb 2021 12:19:20 +0100
From:   Robert Richter <rric@...nel.org>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Dejin Zheng <zhengdejin5@...il.com>, corbet@....net,
        jarkko.nikula@...ux.intel.com, mika.westerberg@...ux.intel.com,
        bhelgaas@...gle.com, wsa@...nel.org, linux-doc@...r.kernel.org,
        linux-i2c@...r.kernel.org, linux-pci@...r.kernel.org, kw@...ux.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 0/4] Introduce pcim_alloc_irq_vectors()

On 18.02.21 16:01:56, Andy Shevchenko wrote:
> The problem this series solves is an imbalanced API.

This (added) API is bloated and incomplete. It adds functions without
benefit, the only is to have a single pcim alloc function in addition
to the pairing of alloc/free functions. I agree, it is hard to detect
which parts are released if pcim_enable_device() is used.

Additional, you need to go through pcim_release() to add other
pcim_*() functions for everything else that is released there.
Otherwise that new API is still incomplete. But this adds another
bunch of useless functions.

> Christoph IIRC was clear that if we want to use PCI IRQ allocation API the
> caller must know what's going on. Hiding this behind the scenes is not good.
> And this series unhides that.

IMO, this is more a documentation issue. pcim_enable_device() must be
better documented and list all enable/alloc functions that are going
to be released out of the box later.

Even better, make sure everything is managed and thus all of a pci_dev
is released, no matter how it was setup (this could even already be
the case).

In addition you could implement a static code checker.

> Also, you may go and clean up all pci_free_irq_vectors() when
> pcim_enable_device() is called, but I guess you will get painful process and
> rejection in a pile of cases.

Why should something be rejected if it is not correctly freed?

Even if pci_free_irq_vectors() is called, pcim_release() will not
complain if it was already freed before. So using
pci_free_irq_vectors() is ok even in conjunction with
pcim_enable_device().

In the end, let's make sure everything is released in pci_dev if it is
managed and document this.

Thanks,

-Robert