svn commit: r349198 - releng/12.0/sys/net

[Gleb Smirnoff]

  Hi,

On Wed, Jun 19, 2019 at 04:41:18PM +0000, Gordon Tetlow wrote:
G> Author: gordon
G> Date: Wed Jun 19 16:41:18 2019
G> New Revision: 349198
G> URL: https://svnweb.freebsd.org/changeset/base/349198
G> 
G> Log:
G>   Fix incorrect locking in networking stack.
G>   
G>   Approved by:	so
G>   Security:	FreeBSD-EN-19:11.net

do I understand correct that problem isn't applicable to head, since
it was accidentially covered in r340413? Either fix for stable or
r340413 aren't a good looking code.

My general comment about if_addr_rlock/if_maddr_lock is that they
are my most hated functions, since they are one of biggest obstacles
on making NIC drivers less aware of what's inside struct ifnet.

In my non-finished projects/ifnet branch, there is a different
KPI to achieve traversal of address lists:

/*
 * Traversing through interface address lists.
 */
typedef void    ifaddr_cb_t(void *, struct sockaddr *, struct sockaddr *,
                    struct sockaddr *);
typedef void    ifmaddr_cb_t(void *, struct sockaddr *);
void    if_foreach_addr(if_t, ifaddr_cb_t, void *);
void    if_foreach_maddr(if_t, ifmaddr_cb_t, void *);

The functions are internal to if.c and do whatever locking
is approprite in our kernel. Mutexes before, and epoch now.
 
And in the driver side this would look like this:

static void
bge_hash_maddr(void *arg, struct sockaddr *maddr)
{
        struct sockaddr_dl *sdl = (struct sockaddr_dl *)maddr;
        uint32_t *hashes = arg;
        int h;
   
        if (sdl->sdl_family != AF_LINK) 
                return;
 
        h = ether_crc32_le(LLADDR(sdl), ETHER_ADDR_LEN) & 0x7F;
        hashes[(h & 0x60) >> 5] |= 1 << (h & 0x1F);
}
 
static void
bge_setmulti(struct bge_softc *sc)
{
        uint32_t hashes[4] = { 0, 0, 0, 0 };
        int i;
 
        BGE_LOCK_ASSERT(sc);
 
        if (sc->bge_if_flags & (IFF_ALLMULTI | IFF_PROMISC)) {
                for (i = 0; i < 4; i++)
                        CSR_WRITE_4(sc, BGE_MAR0 + (i * 4), 0xFFFFFFFF);
                return;
        }
 
        /* First, zot all the existing filters. */
        for (i = 0; i < 4; i++)
                CSR_WRITE_4(sc, BGE_MAR0 + (i * 4), 0);
 
        if_foreach_maddr(sc->bge_ifp, bge_hash_maddr, hashes);
 
        for (i = 0; i < 4; i++)
                CSR_WRITE_4(sc, BGE_MAR0 + (i * 4), hashes[i]);
}

IMHO, much cleaner and independent of struct ifnet than
if_maddr_lock() + traversal, and better than if_multiaddr_count +
if_multiaddr_array since doesn't require allocating memory.

I'd say that there have been enough pain around interface address
traversal, so I would like to go forward with the following plan:

1) Introduce if_foreach_addr/if_foreach_maddr
2) Convert all drivers to this KPI.
3) Declare if_addr_lock/if_maddr_lock and if_multiaddr_count+if_multiaddr_array
   gone in 14.0
4) Indeed make them gone in head once stable/13 is forked.

Any objections?
   
-- 
Gleb Smirnoff