Source-Changes-HG archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

[src/trunk]: src/sys Store IFF_ALLMULTI in ec_flags instead of if_flags to av...



details:   https://anonhg.NetBSD.org/src/rev/126bc6375f8a
branches:  trunk
changeset: 451308:126bc6375f8a
user:      ozaki-r <ozaki-r%NetBSD.org@localhost>
date:      Wed May 15 02:56:47 2019 +0000

description:
Store IFF_ALLMULTI in ec_flags instead of if_flags to avoid data races

IFF_ALLMULTI is set/unset to if_flags via if_mcast_op.  To avoid data races on
if_flags, IFNET_LOCK was added for if_mcast_op.  Unfortunately it produces
a deadlock so we want to remove added IFNET_LOCK by avoiding the data races by
another approach.

This fix introduces ec_flags to struct ethercom and stores IFF_ALLMULTI to it.
ec_flags is protected by ETHER_LOCK and thus IFNET_LOCK is no longer necessary
for if_mcast_op.  Note that the fix is applied only to MP-safe drivers that
the data races matter.

In the kernel, IFF_ALLMULTI is set by a driver and used by the driver itself.
So changing the storing place doesn't break anything.  One exception is
ioctl(SIOCGIFFLAGS); we have to include IFF_ALLMULTI in a result if needed to
export the flag as well as before.

A upcoming commit will remove IFNET_LOCK.

PR kern/54189

diffstat:

 sys/dev/ic/dwc_gmac.c     |  12 ++++++------
 sys/dev/pci/if_wm.c       |  11 +++++++----
 sys/dev/pci/ixgbe/ixgbe.c |  18 +++++++++---------
 sys/net/if.c              |  11 ++---------
 sys/net/if_ether.h        |  10 +++++++++-
 sys/net/if_ethersubr.c    |  13 +++++++++++--
 6 files changed, 44 insertions(+), 31 deletions(-)

diffs (286 lines):

diff -r d02d133cc9f2 -r 126bc6375f8a sys/dev/ic/dwc_gmac.c
--- a/sys/dev/ic/dwc_gmac.c     Wed May 15 01:24:43 2019 +0000
+++ b/sys/dev/ic/dwc_gmac.c     Wed May 15 02:56:47 2019 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: dwc_gmac.c,v 1.59 2019/04/15 06:00:04 ozaki-r Exp $ */
+/* $NetBSD: dwc_gmac.c,v 1.60 2019/05/15 02:56:47 ozaki-r Exp $ */
 
 /*-
  * Copyright (c) 2013, 2014 The NetBSD Foundation, Inc.
@@ -41,7 +41,7 @@
 
 #include <sys/cdefs.h>
 
-__KERNEL_RCSID(1, "$NetBSD: dwc_gmac.c,v 1.59 2019/04/15 06:00:04 ozaki-r Exp $");
+__KERNEL_RCSID(1, "$NetBSD: dwc_gmac.c,v 1.60 2019/05/15 02:56:47 ozaki-r Exp $");
 
 /* #define     DWC_GMAC_DEBUG  1 */
 
@@ -1357,21 +1357,21 @@
                goto special_filter;
        }
 
-       ifp->if_flags &= ~IFF_ALLMULTI;
        ffilt &= ~(AWIN_GMAC_MAC_FFILT_PM|AWIN_GMAC_MAC_FFILT_PR);
 
        bus_space_write_4(sc->sc_bst, sc->sc_bsh, AWIN_GMAC_MAC_HTLOW, 0);
        bus_space_write_4(sc->sc_bst, sc->sc_bsh, AWIN_GMAC_MAC_HTHIGH, 0);
 
        ETHER_LOCK(ec);
+       ec->ec_flags &= ~ETHER_F_ALLMULTI;
        ETHER_FIRST_MULTI(step, ec, enm);
        mcnt = 0;
        while (enm != NULL) {
                if (memcmp(enm->enm_addrlo, enm->enm_addrhi,
                    ETHER_ADDR_LEN) != 0) {
+                       ffilt |= AWIN_GMAC_MAC_FFILT_PM;
+                       ec->ec_flags |= ETHER_F_ALLMULTI;
                        ETHER_UNLOCK(ec);
-                       ffilt |= AWIN_GMAC_MAC_FFILT_PM;
-                       ifp->if_flags |= IFF_ALLMULTI;
                        goto special_filter;
                }
 
@@ -1395,7 +1395,7 @@
            hashes[0]);
        bus_space_write_4(sc->sc_bst, sc->sc_bsh, AWIN_GMAC_MAC_HTHIGH,
            hashes[1]);
-       sc->sc_if_flags = sc->sc_ec.ec_if.if_flags;
+       sc->sc_if_flags = ifp->if_flags;
 
 #ifdef DWC_GMAC_DEBUG
        dwc_gmac_dump_ffilt(sc, ffilt);
diff -r d02d133cc9f2 -r 126bc6375f8a sys/dev/pci/if_wm.c
--- a/sys/dev/pci/if_wm.c       Wed May 15 01:24:43 2019 +0000
+++ b/sys/dev/pci/if_wm.c       Wed May 15 02:56:47 2019 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: if_wm.c,v 1.635 2019/05/14 09:43:55 ozaki-r Exp $      */
+/*     $NetBSD: if_wm.c,v 1.636 2019/05/15 02:56:47 ozaki-r Exp $      */
 
 /*
  * Copyright (c) 2001, 2002, 2003, 2004 Wasabi Systems, Inc.
@@ -82,7 +82,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: if_wm.c,v 1.635 2019/05/14 09:43:55 ozaki-r Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if_wm.c,v 1.636 2019/05/15 02:56:47 ozaki-r Exp $");
 
 #ifdef _KERNEL_OPT
 #include "opt_net_mpsafe.h"
@@ -3707,6 +3707,9 @@
                sc->sc_rctl |= RCTL_BAM;
        if (ifp->if_flags & IFF_PROMISC) {
                sc->sc_rctl |= RCTL_UPE;
+               ETHER_LOCK(ec);
+               ec->ec_flags |= ETHER_F_ALLMULTI;
+               ETHER_UNLOCK(ec);
                goto allmulti;
        }
 
@@ -3757,6 +3760,7 @@
        ETHER_FIRST_MULTI(step, ec, enm);
        while (enm != NULL) {
                if (memcmp(enm->enm_addrlo, enm->enm_addrhi, ETHER_ADDR_LEN)) {
+                       ec->ec_flags |= ETHER_F_ALLMULTI;
                        ETHER_UNLOCK(ec);
                        /*
                         * We must listen to a range of multicast addresses.
@@ -3804,13 +3808,12 @@
 
                ETHER_NEXT_MULTI(step, enm);
        }
+       ec->ec_flags &= ~ETHER_F_ALLMULTI;
        ETHER_UNLOCK(ec);
 
-       ifp->if_flags &= ~IFF_ALLMULTI;
        goto setit;
 
  allmulti:
-       ifp->if_flags |= IFF_ALLMULTI;
        sc->sc_rctl |= RCTL_MPE;
 
  setit:
diff -r d02d133cc9f2 -r 126bc6375f8a sys/dev/pci/ixgbe/ixgbe.c
--- a/sys/dev/pci/ixgbe/ixgbe.c Wed May 15 01:24:43 2019 +0000
+++ b/sys/dev/pci/ixgbe/ixgbe.c Wed May 15 02:56:47 2019 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: ixgbe.c,v 1.182 2019/05/14 09:43:55 ozaki-r Exp $ */
+/* $NetBSD: ixgbe.c,v 1.183 2019/05/15 02:56:47 ozaki-r Exp $ */
 
 /******************************************************************************
 
@@ -3017,10 +3017,10 @@
        KASSERT(mutex_owned(&adapter->core_mtx));
        rctl = IXGBE_READ_REG(&adapter->hw, IXGBE_FCTRL);
        rctl &= (~IXGBE_FCTRL_UPE);
-       if (ifp->if_flags & IFF_ALLMULTI)
+       ETHER_LOCK(ec);
+       if (ec->ec_flags & ETHER_F_ALLMULTI)
                mcnt = MAX_NUM_MULTICAST_ADDRESSES;
        else {
-               ETHER_LOCK(ec);
                ETHER_FIRST_MULTI(step, ec, enm);
                while (enm != NULL) {
                        if (mcnt == MAX_NUM_MULTICAST_ADDRESSES)
@@ -3028,7 +3028,6 @@
                        mcnt++;
                        ETHER_NEXT_MULTI(step, enm);
                }
-               ETHER_UNLOCK(ec);
        }
        if (mcnt < MAX_NUM_MULTICAST_ADDRESSES)
                rctl &= (~IXGBE_FCTRL_MPE);
@@ -3037,11 +3036,12 @@
        if (ifp->if_flags & IFF_PROMISC) {
                rctl |= (IXGBE_FCTRL_UPE | IXGBE_FCTRL_MPE);
                IXGBE_WRITE_REG(&adapter->hw, IXGBE_FCTRL, rctl);
-       } else if (ifp->if_flags & IFF_ALLMULTI) {
+       } else if (ec->ec_flags & ETHER_F_ALLMULTI) {
                rctl |= IXGBE_FCTRL_MPE;
                rctl &= ~IXGBE_FCTRL_UPE;
                IXGBE_WRITE_REG(&adapter->hw, IXGBE_FCTRL, rctl);
        }
+       ETHER_UNLOCK(ec);
 } /* ixgbe_set_promisc */
 
 /************************************************************************
@@ -4354,14 +4354,14 @@
        mta = adapter->mta;
        bzero(mta, sizeof(*mta) * MAX_NUM_MULTICAST_ADDRESSES);
 
-       ifp->if_flags &= ~IFF_ALLMULTI;
        ETHER_LOCK(ec);
+       ec->ec_flags &= ~ETHER_F_ALLMULTI;
        ETHER_FIRST_MULTI(step, ec, enm);
        while (enm != NULL) {
                if ((mcnt == MAX_NUM_MULTICAST_ADDRESSES) ||
                    (memcmp(enm->enm_addrlo, enm->enm_addrhi,
                        ETHER_ADDR_LEN) != 0)) {
-                       ifp->if_flags |= IFF_ALLMULTI;
+                       ec->ec_flags |= ETHER_F_ALLMULTI;
                        break;
                }
                bcopy(enm->enm_addrlo,
@@ -4370,15 +4370,15 @@
                mcnt++;
                ETHER_NEXT_MULTI(step, enm);
        }
-       ETHER_UNLOCK(ec);
 
        fctrl = IXGBE_READ_REG(&adapter->hw, IXGBE_FCTRL);
        fctrl &= ~(IXGBE_FCTRL_UPE | IXGBE_FCTRL_MPE);
        if (ifp->if_flags & IFF_PROMISC)
                fctrl |= (IXGBE_FCTRL_UPE | IXGBE_FCTRL_MPE);
-       else if (ifp->if_flags & IFF_ALLMULTI) {
+       else if (ec->ec_flags & ETHER_F_ALLMULTI) {
                fctrl |= IXGBE_FCTRL_MPE;
        }
+       ETHER_UNLOCK(ec);
 
        IXGBE_WRITE_REG(&adapter->hw, IXGBE_FCTRL, fctrl);
 
diff -r d02d133cc9f2 -r 126bc6375f8a sys/net/if.c
--- a/sys/net/if.c      Wed May 15 01:24:43 2019 +0000
+++ b/sys/net/if.c      Wed May 15 02:56:47 2019 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: if.c,v 1.451 2019/04/20 22:16:47 pgoyette Exp $        */
+/*     $NetBSD: if.c,v 1.452 2019/05/15 02:56:48 ozaki-r Exp $ */
 
 /*-
  * Copyright (c) 1999, 2000, 2001, 2008 The NetBSD Foundation, Inc.
@@ -90,7 +90,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: if.c,v 1.451 2019/04/20 22:16:47 pgoyette Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if.c,v 1.452 2019/05/15 02:56:48 ozaki-r Exp $");
 
 #if defined(_KERNEL_OPT)
 #include "opt_inet.h"
@@ -3625,13 +3625,6 @@
        int rc;
        struct ifreq ifr;
 
-       /* There remain some paths that don't hold IFNET_LOCK yet */
-#ifdef NET_MPSAFE
-       /* CARP and MROUTING still don't deal with the lock yet */
-#if (!defined(NCARP) || (NCARP == 0)) && !defined(MROUTING)
-       KASSERT(IFNET_LOCKED(ifp));
-#endif
-#endif
        if (ifp->if_mcastop != NULL)
                rc = (*ifp->if_mcastop)(ifp, cmd, sa);
        else {
diff -r d02d133cc9f2 -r 126bc6375f8a sys/net/if_ether.h
--- a/sys/net/if_ether.h        Wed May 15 01:24:43 2019 +0000
+++ b/sys/net/if_ether.h        Wed May 15 02:56:47 2019 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: if_ether.h,v 1.77 2019/03/05 08:25:03 msaitoh Exp $    */
+/*     $NetBSD: if_ether.h,v 1.78 2019/05/15 02:56:48 ozaki-r Exp $    */
 
 /*
  * Copyright (c) 1982, 1986, 1993
@@ -189,6 +189,8 @@
         */
        ether_cb_t                              ec_ifflags_cb;
        kmutex_t                                *ec_lock;
+       /* Flags used only by the kernel */
+       int                                     ec_flags;
 #ifdef MBUFTRACE
        struct  mowner ec_rx_mowner;            /* mbufs received */
        struct  mowner ec_tx_mowner;            /* mbufs transmitted */
@@ -225,6 +227,12 @@
 };
 
 #ifdef _KERNEL
+/*
+ * Flags for ec_flags
+ */
+/* Store IFF_ALLMULTI in ec_flags instead of if_flags to avoid data races. */
+#define ETHER_F_ALLMULTI       __BIT(0)
+
 extern const uint8_t etherbroadcastaddr[ETHER_ADDR_LEN];
 extern const uint8_t ethermulticastaddr_slowprotocols[ETHER_ADDR_LEN];
 extern const uint8_t ether_ipmulticast_min[ETHER_ADDR_LEN];
diff -r d02d133cc9f2 -r 126bc6375f8a sys/net/if_ethersubr.c
--- a/sys/net/if_ethersubr.c    Wed May 15 01:24:43 2019 +0000
+++ b/sys/net/if_ethersubr.c    Wed May 15 02:56:47 2019 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: if_ethersubr.c,v 1.273 2019/02/04 10:11:34 mrg Exp $   */
+/*     $NetBSD: if_ethersubr.c,v 1.274 2019/05/15 02:56:48 ozaki-r Exp $       */
 
 /*
  * Copyright (C) 1995, 1996, 1997, and 1998 WIDE Project.
@@ -61,7 +61,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: if_ethersubr.c,v 1.273 2019/02/04 10:11:34 mrg Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if_ethersubr.c,v 1.274 2019/05/15 02:56:48 ozaki-r Exp $");
 
 #ifdef _KERNEL_OPT
 #include "opt_inet.h"
@@ -997,6 +997,7 @@
 
        LIST_INIT(&ec->ec_multiaddrs);
        ec->ec_lock = mutex_obj_alloc(MUTEX_DEFAULT, IPL_NET);
+       ec->ec_flags = 0;
        ifp->if_broadcastaddr = etherbroadcastaddr;
        bpf_attach(ifp, DLT_EN10MB, sizeof(struct ether_header));
 #ifdef MBUFTRACE
@@ -1470,6 +1471,14 @@
                if ((error = ifioctl_common(ifp, cmd, data)) != 0)
                        return error;
                return ether_ioctl_reinit(ec);
+       case SIOCGIFFLAGS:
+               error = ifioctl_common(ifp, cmd, data);
+               if (error == 0) {
+                       /* Set IFF_ALLMULTI for backcompat */
+                       ifr->ifr_flags |= (ec->ec_flags & ETHER_F_ALLMULTI) ?
+                           IFF_ALLMULTI : 0;
+               }
+               return error;
        case SIOCGETHERCAP:
                eccr = (struct eccapreq *)data;
                eccr->eccr_capabilities = ec->ec_capabilities;



Home | Main Index | Thread Index | Old Index