linux-kernel - Re: [PATCH 2/4 net] qca_spi: Fix SPI IRQ handling

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: <bf3dd03e-a1f2-4586-8f00-7003848016aa@gmx.net>
Date:   Fri, 24 Nov 2023 15:01:21 +0100
From:   Stefan Wahren <wahrenst@....net>
To:     Paolo Abeni <pabeni@...hat.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>
Cc:     Lino Sanfilippo <LinoSanfilippo@....de>,
        Florian Fainelli <f.fainelli@...il.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4 net] qca_spi: Fix SPI IRQ handling

Hi Paolo,

Am 23.11.23 um 12:37 schrieb Paolo Abeni:
> On Tue, 2023-11-21 at 17:30 +0100, Stefan Wahren wrote:
>> The functions qcaspi_netdev_open/close are responsible of request &
>> free of the SPI interrupt, which wasn't the best choice. Currently
>> it's possible to trigger a double free of the interrupt by calling
>> qcaspi_netdev_close() after qcaspi_netdev_open() has failed.
>> So let us split IRQ allocation & enabling, so we can take advantage
>> of a device managed IRQ and also fix the issue.
>>
>> Fixes: 291ab06ecf67 ("net: qualcomm: new Ethernet over SPI driver for QCA7000")
>> Signed-off-by: Stefan Wahren <wahrenst@....net>
> The change makes sense, but the changelog is confusing.
>
> qcaspi_netdev_close() and qcaspi_netdev_open() are invoked only via
> ndo_open and ndo_close(), right? So qcaspi_netdev_close() will never be
> invoked qcaspi_netdev_open(), failure - that is when IFF_UP is not set.
sorry, i missed to mention an important part. This issue is partly
connected to patch 3.
Please look at qcaspi_set_ringparam() which also call ndo_close() and
ndo_open(). If you only apply patch 3 you could trigger this issue by
running the following script, interrupt via Strg+C and start again:

#!/bin/bash

while [ true ]; do
   ethtool -G eth1 tx 8
   ethtool -g eth1
   ethtool -G eth1 tx 10
done


[   75.713471] qcaspi spi1.0 eth1: SPI thread exit
[   75.721814] qcaspi spi1.0 eth1: SPI thread created
[   76.795239] qcaspi spi1.0 eth1: SPI thread exit
[   76.815801] qcaspi spi1.0 eth1: SPI thread created
[   77.915872] qcaspi spi1.0 eth1: SPI thread exit
[   77.933982] qcaspi spi1.0 eth1: SPI thread created
[   79.036024] qcaspi spi1.0 eth1: SPI thread exit
[   79.055595] qcaspi spi1.0 eth1: SPI thread created
[   80.076223] qcaspi spi1.0 eth1: SPI thread exit
[   80.097305] qcaspi spi1.0 eth1: SPI thread created
[   81.196471] qcaspi spi1.0 eth1: SPI thread exit
[   81.217351] qcaspi spi1.0 eth1: SPI thread created
[   82.316592] qcaspi spi1.0 eth1: SPI thread exit
[   82.336963] qcaspi spi1.0 eth1: SPI thread created
[   83.436864] qcaspi spi1.0 eth1: SPI thread exit
[   83.461252] qcaspi spi1.0 eth1: SPI thread created
[   84.556950] qcaspi spi1.0 eth1: SPI thread exit
[   84.575897] qcaspi spi1.0 eth1: SPI thread created
[   85.677105] qcaspi spi1.0 eth1: SPI thread exit
[   85.695061] qcaspi spi1.0 eth1: SPI thread created
[   86.717215] qcaspi spi1.0 eth1: SPI thread exit
[   86.739535] qcaspi spi1.0 eth1: SPI thread created
[   87.837355] qcaspi spi1.0 eth1: SPI thread exit
<-- Strg + C
[   87.841072] qcaspi spi1.0 eth1: qcaspi: unable to start kernel thread.
root@...ragon:/srv# ./test_ring_fast.sh
------------[ cut here ]------------
WARNING: CPU: 0 PID: 724 at kernel/irq/manage.c:1887 free_irq+0x23c/0x288
Trying to free already-free IRQ 73
CPU: 0 PID: 724 Comm: ethtool Not tainted
6.1.49-chargebyte-00029-g8c38d497af8a-dirty #108
Hardware name: Freescale i.MX6 Ultralite (Device Tree)
  unwind_backtrace from show_stack+0x10/0x14
  show_stack from dump_stack_lvl+0x24/0x2c
  dump_stack_lvl from __warn+0x74/0xbc
  __warn from warn_slowpath_fmt+0xc8/0x120
  warn_slowpath_fmt from free_irq+0x23c/0x288
  free_irq from qcaspi_netdev_close+0x38/0x5c
  qcaspi_netdev_close from qcaspi_set_ringparam+0x48/0x90
  qcaspi_set_ringparam from ethnl_set_rings+0x2dc/0x320
  ethnl_set_rings from genl_rcv_msg+0x2c4/0x344
  genl_rcv_msg from netlink_rcv_skb+0x98/0xfc
  netlink_rcv_skb from genl_rcv+0x20/0x34
  genl_rcv from netlink_unicast+0x114/0x1a4
  netlink_unicast from netlink_sendmsg+0x314/0x340
  netlink_sendmsg from sock_sendmsg_nosec+0x14/0x24
  sock_sendmsg_nosec from __sys_sendto+0xc4/0xf8
  __sys_sendto from ret_fast_syscall+0x0/0x54
Exception stack(0xe115dfa8 to 0xe115dff0)
dfa0:                   b6ed24dc 0000000c 00000003 005c4238 0000002c
00000000
dfc0: b6ed24dc 0000000c b6f6a5a0 00000122 00472e04 005c41f0 00436b60
005c4190
dfe0: 00000122 bec50b68 b6e5f841 b6dd1ae6
---[ end trace 0000000000000000 ]---
>
> Cheers,
>
> Paolo
>