netdev - Re: [PATCH net 04/19] net/sonic: Add mutual exclusion for accessing shared state

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: <CAMuHMdWvJ975X7zz1C=1Sq=Yuf9nYY1zxWaJ=XCXJukfP=nygg@mail.gmail.com>
Date:   Mon, 20 Jan 2020 09:29:14 +0100
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Finn Thain <fthain@...egraphics.com.au>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Chris Zankel <chris@...kel.net>,
        Laurent Vivier <laurent@...ier.eu>,
        netdev <netdev@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net 04/19] net/sonic: Add mutual exclusion for accessing
 shared state

Hi Finn,

On Mon, Jan 20, 2020 at 12:19 AM Finn Thain <fthain@...egraphics.com.au> wrote:
> The netif_stop_queue() call in sonic_send_packet() races with the
> netif_wake_queue() call in sonic_interrupt(). This causes issues
> like "NETDEV WATCHDOG: eth0 (macsonic): transmit queue 0 timed out".
> Fix this by disabling interrupts when accessing tx_skb[] and next_tx.
> Update a comment to clarify the synchronization properties.
>
> Fixes: efcce839360f ("[PATCH] macsonic/jazzsonic network drivers update")
> Tested-by: Stan Johnson <userm57@...oo.com>
> Signed-off-by: Finn Thain <fthain@...egraphics.com.au>

Thanks for your patch!

> --- a/drivers/net/ethernet/natsemi/sonic.c
> +++ b/drivers/net/ethernet/natsemi/sonic.c
> @@ -242,7 +242,7 @@ static void sonic_tx_timeout(struct net_device *dev)
>   *   wake the tx queue
>   * Concurrently with all of this, the SONIC is potentially writing to
>   * the status flags of the TDs.
> - * Until some mutual exclusion is added, this code will not work with SMP. However,
> + * A spin lock is needed to make this work on SMP platforms. However,
>   * MIPS Jazz machines and m68k Macs were all uni-processor machines.
>   */
>
> @@ -252,6 +252,7 @@ static int sonic_send_packet(struct sk_buff *skb, struct net_device *dev)
>         dma_addr_t laddr;
>         int length;
>         int entry;
> +       unsigned long flags;
>
>         netif_dbg(lp, tx_queued, dev, "%s: skb=%p\n", __func__, skb);
>
> @@ -273,6 +274,8 @@ static int sonic_send_packet(struct sk_buff *skb, struct net_device *dev)
>                 return NETDEV_TX_OK;
>         }
>
> +       local_irq_save(flags);
> +

Wouldn't it be better to use a spinlock instead?
It looks like all currently supported platforms (Mac, Jazz, and XT2000)
do no support SMP, but I'm not 100% sure about the latter.
And this generic sonic.c core may end up being used on other platforms
that do support SMP.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds