netdev - RE: net: fman: IRQ races

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] [day] [month] [year] [list]

Date:   Tue, 7 Jun 2022 14:52:47 +0000
From:   Camelia Alexandra Groza <camelia.groza@....com>
To:     Sean Anderson <sean.anderson@...o.com>,
        Madalin Bucur <madalin.bucur@....com>,
        netdev <netdev@...r.kernel.org>
CC:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Florinel Iordache <florinel.iordache@....com>
Subject: RE: net: fman: IRQ races

> -----Original Message-----
> From: Sean Anderson <sean.anderson@...o.com>
> Sent: Tuesday, May 31, 2022 22:09
> To: Madalin Bucur <madalin.bucur@....com>; netdev
> <netdev@...r.kernel.org>
> Cc: Linux Kernel Mailing List <linux-kernel@...r.kernel.org>; Florinel
> Iordache <florinel.iordache@....com>
> Subject: net: fman: IRQ races
> 
> Hi all,
> 
> I'm doing some refactoring of the dpaa/fman drivers, and I'm a bit
> confused by the way IRQs are handled. To contrast, in GAM/MACB, one of
> the first things IRQ handler does is grab a spinlock guarding register
> access. This lets it do read/modify/writes all it wants. However, I
> don't see anything like that in the FMan code. I'd like to use two
> examples to illustrate.
> 
> First, consider call_mac_isr. It will race with both fman_register_intr:
> 
> CPU0 (call_mac_isr)		CPU1 (fman_register_intr)
> 				isr_cb = foo
> isr_cb(src_handle)
> 				src_handle = bar
> 
> and with fman_unregister_intr
> 
> CPU0 (call_mac_isr)		CPU1 (fman_unregister_intr)
> if (isr_cb)
> 				isr_cb = NULL
> 				src_handle = NULL
> isr_cb(src_handle)
> 
> This is probably not too critical (since hopefully there are no
> interrupts before/after the handler is registered), but it certainly
> looks very strange.
> 
> Second, consider dtsec_isr. It will race with (for example) dtsec_set_allmulti:
> 
> CPU0 (dtsec_isr)		CPU1 (dtsec_set_allmulti)
> <XFUNEN interrupt>
> ioread32be(rctrl)		ioread32be(rctrl)
> 				iowrite32be(rctrl | MPROM)
> iowrite32be(rctrl | GRS)
> 
> and suddenly the MPROM write is dropped. (Actually, the whole
> FM_TX_LOCKUP_ERRATA_DTSEC6 errata code seems funky, since after
> calling
> fman_reset_mac it seems like everything would need to be reinitialized).
> 
> So what's going on here? Is there actually no locking, or am I missing
> something?

Hi

You are right, there is no locking. The original FMan driver design didn't intend
on supporting runtime register changes. Clearly this was a mistake as you
pointed out.

Camelia