On 07.08.24 22:07, Rosen Penev wrote:
On Wed, Aug 7, 2024 at 1:05 PM Felix Fietkau <nbd@xxxxxxxx> wrote:
On 07.08.24 20:52, Rosen Penev wrote:
> On Wed, Aug 7, 2024 at 10:47 AM Felix Fietkau <nbd@xxxxxxxx> 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@xxxxxxxxx>
>> > ---
>> > 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.
- Felix
