On 05/09/2018 08:38 AM, Lukas Wunner wrote: > hi3110_get_berr_counter() may run concurrently to the rest of the driver > but neglects to acquire the lock protecting access to the SPI device. > As a result, it and the rest of the driver may clobber each other's tx > and rx buffers. > > We became aware of this issue because transmission of packets with > "cangen -g 0 -i -x" frequently hung. It turns out that agetty executes > ->do_get_berr_counter every few seconds via the following call stack: > > CPU: 2 PID: 1605 Comm: agetty > [<7f3f7500>] (hi3110_get_berr_counter [hi311x]) > [<7f130204>] (can_fill_info [can_dev]) > [<80693bc0>] (rtnl_fill_ifinfo) > [<806949ec>] (rtnl_dump_ifinfo) > [<806b4834>] (netlink_dump) > [<806b4bc8>] (netlink_recvmsg) > [<8065f180>] (sock_recvmsg) > [<80660f90>] (___sys_recvmsg) > [<80661e7c>] (__sys_recvmsg) > [<80661ec0>] (SyS_recvmsg) > [<80108b20>] (ret_fast_syscall+0x0/0x1c) > > agetty listens to netlink messages in order to update the login prompt > when IP addresses change (if /etc/issue contains \4 or \6 escape codes): > https://git.kernel.org/pub/scm/utils/util-linux/util-linux.git/commit/?id=e36deb6424e8 > > It's a useful feature, though it seems questionable that it causes CAN > bit error statistics to be queried. > > Be that as it may, if hi3110_get_berr_counter() is invoked while a frame > is sent by hi3110_hw_tx(), bogus SPI transfers like the following may > occur: > > => 12 00 (hi3110_get_berr_counter() wanted to transmit > EC 00 to query the transmit error counter, > but the first byte was overwritten by > hi3110_hw_tx_frame()) > > => EA 00 3E 80 01 FB (hi3110_hw_tx_frame() wanted to transmit a > frame, but the first byte was overwritten by > hi3110_get_berr_counter() because it wanted > to query the receive error counter) > > This sequence hangs the transmission because the driver believes it has > sent a frame and waits for the interrupt signaling completion, but in > reality the chip has never sent away the frame since the commands it > received were malformed. > > Fix by acquiring the SPI lock in hi3110_get_berr_counter(). > > I've scrutinized the entire driver for further unlocked SPI accesses but > found no others. > > Cc: Mathias Duckeck <m.duck...@kunbus.de> > Cc: Akshay Bhat <akshay.b...@timesys.com> > Cc: Casey Fitzpatrick <casey.fitzpatr...@timesys.com> > Cc: Stef Walter <st...@redhat.com> > Cc: Karel Zak <k...@redhat.com> > Cc: sta...@vger.kernel.org # v4.12+ > Signed-off-by: Lukas Wunner <lu...@wunner.de> > ---
Reviewed-by: Akshay Bhat <akshay.b...@timesys.com>
