| 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
| ||
|
Date: Tue, 29 Sep 2020 16:06:59 -0700
From: Shannon Nelson <snelson@...sando.io>
To: Thomas Gleixner <tglx@...utronix.de>,
LKML <linux-kernel@...r.kernel.org>
Cc: Peter Zijlstra <peterz@...radead.org>,
Paul McKenney <paulmck@...nel.org>,
Matthew Wilcox <willy@...radead.org>,
Christian Benvenuti <benve@...co.com>,
Govindarajulu Varadarajan <_govind@....com>,
Dave Miller <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
Jonathan Corbet <corbet@....net>,
Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
linux-doc@...r.kernel.org,
Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
Luc Van Oostenryck <luc.vanoostenryck@...il.com>,
Jay Cliburn <jcliburn@...il.com>,
Chris Snook <chris.snook@...il.com>,
Vishal Kulkarni <vishal@...lsio.com>,
Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
intel-wired-lan@...ts.osuosl.org,
Pensando Drivers <drivers@...sando.io>,
Andrew Lunn <andrew@...n.ch>,
Heiner Kallweit <hkallweit1@...il.com>,
Russell King <linux@...linux.org.uk>,
Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
Solarflare linux maintainers <linux-net-drivers@...arflare.com>,
Edward Cree <ecree@...arflare.com>,
Martin Habets <mhabets@...arflare.com>,
Jon Mason <jdmason@...zu.us>, Daniel Drake <dsd@...too.org>,
Ulrich Kunitz <kune@...ne-taler.de>,
Kalle Valo <kvalo@...eaurora.org>,
linux-wireless@...r.kernel.org, linux-usb@...r.kernel.org,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Arend van Spriel <arend.vanspriel@...adcom.com>,
Franky Lin <franky.lin@...adcom.com>,
Hante Meuleman <hante.meuleman@...adcom.com>,
Chi-Hsien Lin <chi-hsien.lin@...ress.com>,
Wright Feng <wright.feng@...ress.com>,
brcm80211-dev-list.pdl@...adcom.com,
brcm80211-dev-list@...ress.com,
Stanislav Yakovlev <stas.yakovlev@...il.com>,
Stanislaw Gruszka <stf_xl@...pl>,
Johannes Berg <johannes.berg@...el.com>,
Emmanuel Grumbach <emmanuel.grumbach@...el.com>,
Luca Coelho <luciano.coelho@...el.com>,
Intel Linux Wireless <linuxwifi@...el.com>,
Jouni Malinen <j@...fi>,
Amitkumar Karwar <amitkarwar@...il.com>,
Ganapathi Bhat <ganapathi.bhat@....com>,
Xinming Hu <huxinming820@...il.com>,
libertas-dev@...ts.infradead.org,
Pascal Terjan <pterjan@...gle.com>,
Ping-Ke Shih <pkshih@...ltek.com>
Subject: Re: [patch V2 11/36] net: ionic: Replace in_interrupt() usage.
On 9/29/20 1:25 PM, Thomas Gleixner wrote:
> From: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
>
> The in_interrupt() usage in this driver tries to figure out which context
> may sleep and which context may not sleep. in_interrupt() is not really
> suitable as it misses both preemption disabled and interrupt disabled
> invocations from task context.
>
> Conditionals like that in driver code are frowned upon in general because
> invocations of functions from invalid contexts might not be detected
> as the conditional papers over it.
>
> ionic_lif_addr() and _ionoc_lif_rx_mode() can be called from:
>
> 1) ->ndo_set_rx_mode() which is under netif_addr_lock_bh()) so it must not
> sleep.
>
> 2) Init and setup functions which are in fully preemptible task context.
>
> ionic_link_status_check_request() has two call paths:
>
> 1) NAPI which obviously cannot sleep
>
> 2) Setup which is again fully preemptible task context
>
> Add arguments which convey the execution context to the affected functions
> and let the callers provide the context instead of letting the functions
> deduce it.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> ---
> V2: Treat _ionoc_lif_rx_mode() correclty (Shannon)
Is it fair to poke at the comments?
s/ionoc/ionic/
s/correclty/correctly/
> ---
> drivers/net/ethernet/pensando/ionic/ionic_dev.c | 2
> drivers/net/ethernet/pensando/ionic/ionic_lif.c | 64 ++++++++++++++++--------
> drivers/net/ethernet/pensando/ionic/ionic_lif.h | 2
> 3 files changed, 47 insertions(+), 21 deletions(-)
>
> --- a/drivers/net/ethernet/pensando/ionic/ionic_dev.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
> @@ -22,7 +22,7 @@ static void ionic_watchdog_cb(struct tim
> hb = ionic_heartbeat_check(ionic);
>
> if (hb >= 0 && ionic->lif)
> - ionic_link_status_check_request(ionic->lif);
> + ionic_link_status_check_request(ionic->lif, false);
> }
>
> void ionic_init_devinfo(struct ionic *ionic)
> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> @@ -151,7 +151,7 @@ static void ionic_link_status_check(stru
> clear_bit(IONIC_LIF_F_LINK_CHECK_REQUESTED, lif->state);
> }
>
> -void ionic_link_status_check_request(struct ionic_lif *lif)
> +void ionic_link_status_check_request(struct ionic_lif *lif, bool can_sleep)
> {
> struct ionic_deferred_work *work;
>
> @@ -159,7 +159,7 @@ void ionic_link_status_check_request(str
> if (test_and_set_bit(IONIC_LIF_F_LINK_CHECK_REQUESTED, lif->state))
> return;
>
> - if (in_interrupt()) {
> + if (!can_sleep) {
> work = kzalloc(sizeof(*work), GFP_ATOMIC);
> if (!work)
> return;
> @@ -798,7 +798,7 @@ static bool ionic_notifyq_service(struct
>
> switch (le16_to_cpu(comp->event.ecode)) {
> case IONIC_EVENT_LINK_CHANGE:
> - ionic_link_status_check_request(lif);
> + ionic_link_status_check_request(lif, false);
> break;
> case IONIC_EVENT_RESET:
> work = kzalloc(sizeof(*work), GFP_ATOMIC);
> @@ -981,7 +981,8 @@ static int ionic_lif_addr_del(struct ion
> return 0;
> }
>
> -static int ionic_lif_addr(struct ionic_lif *lif, const u8 *addr, bool add)
> +static int ionic_lif_addr(struct ionic_lif *lif, const u8 *addr, bool add,
> + bool can_sleep)
> {
> struct ionic *ionic = lif->ionic;
> struct ionic_deferred_work *work;
> @@ -1010,7 +1011,7 @@ static int ionic_lif_addr(struct ionic_l
> lif->nucast--;
> }
>
> - if (in_interrupt()) {
> + if (!can_sleep) {
> work = kzalloc(sizeof(*work), GFP_ATOMIC);
> if (!work) {
> netdev_err(lif->netdev, "%s OOM\n", __func__);
> @@ -1036,12 +1037,22 @@ static int ionic_lif_addr(struct ionic_l
>
> static int ionic_addr_add(struct net_device *netdev, const u8 *addr)
> {
> - return ionic_lif_addr(netdev_priv(netdev), addr, true);
> + return ionic_lif_addr(netdev_priv(netdev), addr, true, true);
> +}
> +
> +static int ionic_ndo_addr_add(struct net_device *netdev, const u8 *addr)
> +{
> + return ionic_lif_addr(netdev_priv(netdev), addr, true, false);
> }
>
> static int ionic_addr_del(struct net_device *netdev, const u8 *addr)
> {
> - return ionic_lif_addr(netdev_priv(netdev), addr, false);
> + return ionic_lif_addr(netdev_priv(netdev), addr, false, true);
> +}
> +
> +static int ionic_ndo_addr_del(struct net_device *netdev, const u8 *addr)
> +{
> + return ionic_lif_addr(netdev_priv(netdev), addr, false, false);
> }
These changes are reasonable, tho' I'm not fond of parameter lists of
"true" and "false" with no context. I'd prefer to have some constants like
#define can_sleep true
so the code can be a little more readable.
Yes, I know we have this problem already in the call to
ionic_lif_addr(), which I'm annoyed with but haven't addressed yet.
So, if you want to deal with this now, fine, otherwise I'll take care of
them both later.
Meanwhile,
Acked-by: Shannon Nelson <snelson@...sando.io>
>
> static void ionic_lif_rx_mode(struct ionic_lif *lif, unsigned int rx_mode)
> @@ -1081,11 +1092,12 @@ static void ionic_lif_rx_mode(struct ion
> lif->rx_mode = rx_mode;
> }
>
> -static void _ionic_lif_rx_mode(struct ionic_lif *lif, unsigned int rx_mode)
> +static void _ionic_lif_rx_mode(struct ionic_lif *lif, unsigned int rx_mode,
> + bool from_ndo)
> {
> struct ionic_deferred_work *work;
>
> - if (in_interrupt()) {
> + if (from_ndo) {
> work = kzalloc(sizeof(*work), GFP_ATOMIC);
> if (!work) {
> netdev_err(lif->netdev, "%s OOM\n", __func__);
> @@ -1100,7 +1112,16 @@ static void _ionic_lif_rx_mode(struct io
> }
> }
>
> -static void ionic_set_rx_mode(struct net_device *netdev)
> +static void ionic_dev_uc_sync(struct net_device *netdev, bool from_ndo)
> +{
> + if (from_ndo)
> + __dev_uc_sync(netdev, ionic_ndo_addr_add, ionic_ndo_addr_del);
> + else
> + __dev_uc_sync(netdev, ionic_addr_add, ionic_addr_del);
> +
> +}
> +
> +static void ionic_set_rx_mode(struct net_device *netdev, bool from_ndo)
> {
> struct ionic_lif *lif = netdev_priv(netdev);
> struct ionic_identity *ident;
> @@ -1122,7 +1143,7 @@ static void ionic_set_rx_mode(struct net
> * we remove our overflow flag and check the netdev flags
> * to see if we can disable NIC PROMISC
> */
> - __dev_uc_sync(netdev, ionic_addr_add, ionic_addr_del);
> + ionic_dev_uc_sync(netdev, from_ndo);
> nfilters = le32_to_cpu(ident->lif.eth.max_ucast_filters);
> if (netdev_uc_count(netdev) + 1 > nfilters) {
> rx_mode |= IONIC_RX_MODE_F_PROMISC;
> @@ -1134,7 +1155,7 @@ static void ionic_set_rx_mode(struct net
> }
>
> /* same for multicast */
> - __dev_mc_sync(netdev, ionic_addr_add, ionic_addr_del);
> + ionic_dev_uc_sync(netdev, from_ndo);
> nfilters = le32_to_cpu(ident->lif.eth.max_mcast_filters);
> if (netdev_mc_count(netdev) > nfilters) {
> rx_mode |= IONIC_RX_MODE_F_ALLMULTI;
> @@ -1146,7 +1167,12 @@ static void ionic_set_rx_mode(struct net
> }
>
> if (lif->rx_mode != rx_mode)
> - _ionic_lif_rx_mode(lif, rx_mode);
> + _ionic_lif_rx_mode(lif, rx_mode, from_ndo);
> +}
> +
> +static void ionic_ndo_set_rx_mode(struct net_device *netdev)
> +{
> + ionic_set_rx_mode(netdev, true);
> }
>
> static __le64 ionic_netdev_features_to_nic(netdev_features_t features)
> @@ -1391,7 +1417,7 @@ static int ionic_start_queues_reconfig(s
> */
> err = ionic_txrx_init(lif);
> mutex_unlock(&lif->queue_lock);
> - ionic_link_status_check_request(lif);
> + ionic_link_status_check_request(lif, true);
> netif_device_attach(lif->netdev);
>
> return err;
> @@ -1720,7 +1746,7 @@ static int ionic_txrx_init(struct ionic_
> if (lif->netdev->features & NETIF_F_RXHASH)
> ionic_lif_rss_init(lif);
>
> - ionic_set_rx_mode(lif->netdev);
> + ionic_set_rx_mode(lif->netdev, false);
>
> return 0;
>
> @@ -2093,7 +2119,7 @@ static const struct net_device_ops ionic
> .ndo_stop = ionic_stop,
> .ndo_start_xmit = ionic_start_xmit,
> .ndo_get_stats64 = ionic_get_stats64,
> - .ndo_set_rx_mode = ionic_set_rx_mode,
> + .ndo_set_rx_mode = ionic_ndo_set_rx_mode,
> .ndo_set_features = ionic_set_features,
> .ndo_set_mac_address = ionic_set_mac_address,
> .ndo_validate_addr = eth_validate_addr,
> @@ -2521,7 +2547,7 @@ static void ionic_lif_handle_fw_up(struc
> }
>
> clear_bit(IONIC_LIF_F_FW_RESET, lif->state);
> - ionic_link_status_check_request(lif);
> + ionic_link_status_check_request(lif, true);
> netif_device_attach(lif->netdev);
> dev_info(ionic->dev, "FW Up: LIFs restarted\n");
>
> @@ -2713,7 +2739,7 @@ static int ionic_station_set(struct ioni
> */
> if (!ether_addr_equal(ctx.comp.lif_getattr.mac,
> netdev->dev_addr))
> - ionic_lif_addr(lif, netdev->dev_addr, true);
> + ionic_lif_addr(lif, netdev->dev_addr, true, true);
> } else {
> /* Update the netdev mac with the device's mac */
> memcpy(addr.sa_data, ctx.comp.lif_getattr.mac, netdev->addr_len);
> @@ -2730,7 +2756,7 @@ static int ionic_station_set(struct ioni
>
> netdev_dbg(lif->netdev, "adding station MAC addr %pM\n",
> netdev->dev_addr);
> - ionic_lif_addr(lif, netdev->dev_addr, true);
> + ionic_lif_addr(lif, netdev->dev_addr, true, true);
>
> return 0;
> }
> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
> @@ -245,7 +245,7 @@ static inline u32 ionic_coal_usec_to_hw(
>
> typedef void (*ionic_reset_cb)(struct ionic_lif *lif, void *arg);
>
> -void ionic_link_status_check_request(struct ionic_lif *lif);
> +void ionic_link_status_check_request(struct ionic_lif *lif, bool can_sleep);
> void ionic_get_stats64(struct net_device *netdev,
> struct rtnl_link_stats64 *ns);
> void ionic_lif_deferred_enqueue(struct ionic_deferred *def,
>
Powered by blists - more mailing lists