netdev - Handling set_mac_address in set_rx

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-next>] [day] [month] [year] [list]

Date:	Sat, 30 Jun 2007 20:51:00 +0200
From:	Patrick McHardy <kaber@...sh.net>
To:	Linux Netdev List <netdev@...r.kernel.org>
Subject: Handling set_mac_address in set_rx_mode

While adding support for secondary unicast addresses to 8021q and
macvlan, I've tried keeping dev->dev_addr as global address on
dev->uc_list and have drivers skip them to avoid having all
dev_unicast_add users implement a state machine like this:

open(struct net_device *dev)
{
    struct net_device *lowerdev = ...;

    /* add our address in case it differs */
    if (memcmp(addr, lowerdev->dev_addr, dev->addr_len)
       dev_unicast_add(lowerdev, dev->dev_addr, dev->addr_len);
}

close(struct net_device *dev)
{
    struct net_device *lowerdev = ...;

    /* delete our address in case we've added it */
    if (memcmp(dev->dev_addr, lowerdev->dev_addr, dev->addr_len))
       dev_unicast_delete(lowerdev, dev->dev_addr, dev->addr_len);
}

set_mac_address(struct net_device *dev, void *addr)
{
    struct net_device *lowerdev = ...;

    if (!dev->flags & IFF_UP)
       return 0;

    /* delete our old address in case we've added it */
    if (memcmp(dev->dev_addr, lowerdev->dev_addr, dev->addr_len))
       dev_unicast_delete(lowerdev, dev->dev_addr, dev->addr_len);

    memcpy(dev->dev_addr, addr, dev->addr_len)

    /* add our new address in case it differs */
    if (memcmp(addr, lowerdev->dev_addr, dev->addr_len)
       dev_unicast_add(lowerdev, dev->dev_addr, dev->addr_len);

    store lowerdev->dev_addr somewhere
}

netdev_notifier(struct notifier_block, unsigned long event, void *ptr)
{
    struct net_device *lowerdev = ptr;
    struct net_device *dev = ...;

    switch (event) {
    case NETDEV_CHANGEADDR:
        /* was different before, is equal now: remove our address */
       if (memcmp(dev->dev_addr, last_lowerdev_addr, dev->addr_len) &&
           !memcmp(dev->dev_addr, lowerdev->dev_addr, dev->addr_len))
            dev_unicast_delete(lowerdev, dev->dev_addr, dev->addr_len);

       /* is different now, was equal before: add our address */
       if (memcmp(dev->dev_addr, lowerdev->dev_addr, dev->addr_len) &&
           !memcmp(dev->dev_addr, last_lowerdev_addr, dev->addr_len))
          dev_unicast_add(lowerdev, dev->dev_addr, dev->addr_len);
       break;
    }  
}

Not that its terribly complicated, but its not exactly pretty and
the existing examples of address and flag synchronization all get
*something* wrong, so its better to keep requirements simple.

Having the primary address on dev->uc_list would have the advantage
that the existing refcounting would take care of keeping addresses
around while they're still in use and drivers could just unconditionally
add and remove their own addresses without worrying about needlessly
putting the underlying device in promiscous mode.

Adding dev_addr to the uc_list in dev_set_mac_address turned out to
be more complicated than expected though because rolling back to the
previous state on error always includes an operation that might fail
again (dev->set_mac_address and dev->set_rx_mode).

The easiest way to handle this would be to program the MAC address
in set_rx_mode as well since then we'd only have a single operation
that could fail. For e1000 this would even simplify the code because
set_mac_address and set_rx_mode both program the same filters and handle
the same quirks. I would expect the same to be true for other drivers
as well.

If there are no objections to this I'll code something up tommorrow.

-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html