netdev - Re: [RFC PATCH v2 net-next 6/7] net: ethtool: add a mutex protecting RSS contexts

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: <485ebfeb-61d7-7636-80af-50b6a008b6dc@gmail.com>
Date:   Thu, 13 Apr 2023 22:52:39 +0100
From:   Edward Cree <ecree.xilinx@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Andrew Lunn <andrew@...n.ch>, edward.cree@....com,
        linux-net-drivers@....com, davem@...emloft.net, pabeni@...hat.com,
        edumazet@...gle.com, netdev@...r.kernel.org,
        habetsm.xilinx@...il.com, sudheer.mogilappagari@...el.com
Subject: Re: [RFC PATCH v2 net-next 6/7] net: ethtool: add a mutex protecting
 RSS contexts

On 13/04/2023 03:06, Jakub Kicinski wrote:
> IMO the "MCPU has rebooted" case is a control path, taking rtnl seems
> like the right thing to do. Does that happen often enough or takes so
> long to recover that we need to be concerned about locking down rtnl?

Normally we *do* hold RTNL across the reset handling path, and all the
 callers I can find take it.  But the existence of a more complicated
 condition guarding the ASSERT_RTNL() in EFX_ASSERT_RESET_SERIALISED()
 implies that there is, or at least was, a call site that doesn't;
 that makes me nervous about assuming it.

> aRFS can't sleep IIUC so the mutex is does not seem like a great match.

sfc punts aRFS filter insertion into a workitem, because you can't do
 MCDI without sleeping.  And aRFS was just one example; there's lots
 of other sources of filters in the driver (PTP, sync_rx_mode (device
 UC/MC addresses), VLAN filtering (NETIF_F_HW_VLAN_CTAG_FILTER)...).
 Some of those filters can also have EFX_FILTER_FLAG_RX_RSS (though
 not custom contexts).

So while I *think* the filter insert code could carefully go
   if (spec->flags & EFX_FILTER_FLAG_RX_RSS && spec->rss_context)
       ASSERT_RTNL();
 it's kinda hairy.  And if this is a *normal* level of hair for
 drivers to have in this area, then the "but driver writers don't
 understand locking!" argument doesn't really favour one solution over
 the other.  After all, the driver will still have to hold *some* lock
 to access this stuff, whether it's rss_lock or RTNL.
(Idk, maybe sfc is just uniquely complex and messy.  It wouldn't be
 the first time.)

> IOW I had the same reaction as Andrew first time I saw this mutex :(

Well I seem to be outvoted, so I'll have another crack at getting it
 to work with just RTNL (that's what I went for originally, actually,
 but one of the ASSERT_RTNL()s failed in test and I couldn't figure
 out why at the time.  Possibly trying to argue the case has helped me
 to understand more of the details!).