Re: [PATCH] net: ath9k: use devm for request_irq

[Kalle Valo <kvalo@kernel.org>]

Toke Høiland-Jørgensen <toke@kernel.org> writes:

> Felix Fietkau <nbd@nbd.name> writes:
>
>> On 07.08.24 22:07, Rosen Penev wrote:
>>> On Wed, Aug 7, 2024 at 1:05 PM Felix Fietkau <nbd@nbd.name> wrote:
>>>>
>>>> On 07.08.24 20:52, Rosen Penev wrote:
>>>> > On Wed, Aug 7, 2024 at 10:47 AM Felix Fietkau <nbd@nbd.name> wrote:
>>>> >>
>>>> >> On 31.07.24 23:02, Rosen Penev wrote:
>>>> >> > Avoids having to manually call free_irq. Simplifies code slightly.
>>>> >> >
>>>> >> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
>>>> >> > ---
>>>> >> >   drivers/net/wireless/ath/ath9k/ahb.c | 7 ++-----
>>>> >> >   drivers/net/wireless/ath/ath9k/pci.c | 9 +++------
>>>> >> >   2 files changed, 5 insertions(+), 11 deletions(-)
>>>> >> >
>>>> >> > diff --git a/drivers/net/wireless/ath/ath9k/ahb.c b/drivers/net/wireless/ath/ath9k/ahb.c
>>>> >> > index 1a6697b6e3b4..29f67ded8fe2 100644
>>>> >> > --- a/drivers/net/wireless/ath/ath9k/ahb.c
>>>> >> > +++ b/drivers/net/wireless/ath/ath9k/ahb.c
>>>> >> > @@ -118,7 +118,7 @@ static int ath_ahb_probe(struct platform_device *pdev)
>>>> >> >       sc->mem = mem;
>>>> >> >       sc->irq = irq;
>>>> >> >
>>>> >> > -     ret = request_irq(irq, ath_isr, IRQF_SHARED, "ath9k", sc);
>>>> >> > +     ret = devm_request_irq(&pdev->dev, irq, ath_isr, IRQF_SHARED, "ath9k", sc);
>>>> >> Sorry for the late response, but I think this patch is wrong any may
>>>> >> need to be reverted. If there is an error during probe, and the IRQ
>>>> >> fires for some reason, there could be an use-after-free bug when the IRQ
>>>> >> handler accesses the data in sc.
>>>> >> The explicit freq_irq calls were preventing that from happening.
>>>> > How about keeping the devm variant and replacing free_irq with
>>>> > devm_free_irq in probe?
>>>>
>>>> If you do that, then using the devm variant is completely pointless.
>>>> I think a full revert is the best option.
>>> OTOH it still allows removing free_irq from _remove, but I see your point.
>>
>> No, because you'd have the same use-after-free bug there as well.
>
> Alright, let's revert. Kalle, can you just do the revert, or should I
> send a patch for it?

Thanks, the best is to send a patch.

But honestly more and more I'm starting to think that we should just
reject all these "drive-by cleanups". We have better things to do than
fixing unnecessary their bugs. Thoughts?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches