netdev - RE: [EXT] module refcount issues in the liquidio driver

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]

Message-ID: <DM6PR18MB306560B6FA9700B884065FF9B9909@DM6PR18MB3065.namprd18.prod.outlook.com>
Date:   Thu, 11 Mar 2021 17:43:59 +0000
From:   Felix Manlunas <fmanlunas@...vell.com>
To:     Christoph Hellwig <hch@...radead.org>,
        Derek Chickles <dchickles@...vell.com>,
        Satananda Burla <sburla@...vell.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [EXT] module refcount issues in the liquidio driver

From: Christoph Hellwig <hch@...radead.org>
Date: Wed 3/10/2021 11:38 PM -0800

> Hi all,
> 
> I just stumbled over the odd handling of module refcounts in the liquidio
> driver.  The big red flag is the call to module_refcount in
> liquidio_watchdog, which will do the wrong thing for any external module
> refcount, like a userspace open.
>
> But more importantly the whole concept of acquiring module refcounts from
> inside the driver is pretty bogus.  What problem does this try to solve?

The problem is described in the commit log below, in "(2) Decrement the
module refcount ...".

commit bb54be589c7a8451a0504924703abdffeb06b79f
Author: Felix Manlunas <felix.manlunas@...ium.com>
Date:   Tue Apr 4 19:26:57 2017 -0700

    liquidio: fix Octeon core watchdog timeout false alarm
    
    Detection of watchdog timeout of Octeon cores is flawed and susceptible to
    false alarms.  Refactor by removing the detection code, and in its place,
    leverage existing code that monitors for an indication from the NIC
    firmware that an Octeon core crashed; expand the meaning of the indication
    to "an Octeon core crashed or its watchdog timer expired".  Detection of
    watchdog timeout is now delegated to an exception handler in the NIC
    firmware; this is free of false alarms.
    
    Also if there's an Octeon core crash or watchdog timeout:
    (1) Disable VF Ethernet links.
    (2) Decrement the module refcount by an amount equal to the number of
        active VFs of the NIC whose Octeon core crashed or had a watchdog
        timeout.  The refcount will continue to reflect the active VFs of
        other liquidio NIC(s) (if present) whose Octeon cores are faultless.
    
    Item (2) is needed to avoid the case of not being able to unload the driver
    because the module refcount is stuck at some non-zero number.  There is
    code that, in normal cases, decrements the refcount upon receiving a
    message from the firmware that a VF driver was unloaded.  But in
    exceptional cases like an Octeon core crash or watchdog timeout, arrival of
    that particular message from the firmware might be unreliable.  That normal
    case code is changed to not touch the refcount in the exceptional case to
    avoid contention (over the refcount) with the liquidio_watchdog kernel
    thread who will carry out item (2).
    
    Signed-off-by: Felix Manlunas <felix.manlunas@...ium.com>
    Signed-off-by: Derek Chickles <derek.chickles@...ium.com>
    Signed-off-by: David S. Miller <davem@...emloft.net>