netdev - Re: [PATCH net 4/4] macvlan: Let passthru macvlan correctly restore lower mac address

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

Message-ID: <0c267beb-d9f6-402e-671e-b27bcde92047@redhat.com>
Date:   Sat, 17 Jun 2017 11:21:01 -0400
From:   Vlad Yasevich <vyasevic@...hat.com>
To:     Girish Moodalbail <girish.moodalbail@...cle.com>,
        Vladislav Yasevich <vyasevich@...il.com>,
        netdev@...r.kernel.org
Subject: Re: [PATCH net 4/4] macvlan: Let passthru macvlan correctly restore
 lower mac address

On 06/17/2017 12:35 AM, Girish Moodalbail wrote:
> Sorry, it took sometime to wrap around this patch series since they all change one file
> and at times the same function :).
> 
> 
> On 6/16/17 6:36 AM, Vladislav Yasevich wrote:
>> Passthru macvlans directly change the mac address of the lower
>> level device.  That's OK, but after the macvlan is deleted,
>> the lower device is left with changed address and one needs to
>> reboot to bring back the origina HW addresses.
> 
> s/origina/original/
> 
>>
>> This scenario is actually quite common with passthru macvtap devices.
>>
>> This patch attempts to solve this, by storing the mac address
>> of the lower device in macvlan_port structure and keeping track of
>> it through the changes.
>>
>> After this patch, any changes to the lower device mac address
>> done trough the macvlan device, will be reverted back.  Any
>> changes done directly to the lower device mac address will be kept.
>>
>> Signed-off-by: Vladislav Yasevich <vyasevic@...hat.com>
>> ---
>>  drivers/net/macvlan.c | 47 ++++++++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 44 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
>> index eb956ff..c551165 100644
>> --- a/drivers/net/macvlan.c
>> +++ b/drivers/net/macvlan.c
>> @@ -40,6 +40,7 @@
>>  #define MACVLAN_BC_QUEUE_LEN    1000
>>
>>  #define MACVLAN_F_PASSTHRU    1
>> +#define MACVLAN_F_ADDRCHANGE    2
>>
>>  struct macvlan_port {
>>      struct net_device    *dev;
>> @@ -51,6 +52,7 @@ struct macvlan_port {
>>      int            count;
>>      struct hlist_head    vlan_source_hash[MACVLAN_HASH_SIZE];
>>      DECLARE_BITMAP(mc_filter, MACVLAN_MC_FILTER_SZ);
>> +    unsigned char           perm_addr[ETH_ALEN];
>>  };
>>
>>  struct macvlan_source_entry {
>> @@ -78,6 +80,21 @@ static inline void macvlan_set_passthru(struct macvlan_port *port)
>>      port->flags |= MACVLAN_F_PASSTHRU;
>>  }
>>
>> +static inline bool macvlan_addr_change(const struct macvlan_port *port)
>> +{
>> +    return port->flags & MACVLAN_F_ADDRCHANGE;
>> +}
>> +
>> +static inline void macvlan_set_addr_change(struct macvlan_port *port)
>> +{
>> +    port->flags |= MACVLAN_F_ADDRCHANGE;
>> +}
>> +
>> +static inline void macvlan_clear_addr_change(struct macvlan_port *port)
>> +{
>> +    port->flags &= ~MACVLAN_F_ADDRCHANGE;
>> +}
>> +
>>  /* Hash Ethernet address */
>>  static u32 macvlan_eth_hash(const unsigned char *addr)
>>  {
>> @@ -193,11 +210,11 @@ static void macvlan_hash_change_addr(struct macvlan_dev *vlan,
>>  static bool macvlan_addr_busy(const struct macvlan_port *port,
>>                    const unsigned char *addr)
>>  {
>> -    /* Test to see if the specified multicast address is
>> +    /* Test to see if the specified address is
>>       * currently in use by the underlying device or
>>       * another macvlan.
>>       */
>> -    if (!macvlan_passthru(port) &&
>> +    if (!macvlan_passthru(port) && !macvlan_addr_change(port) &&
>>          ether_addr_equal_64bits(port->dev->dev_addr, addr))
>>          return true;
>>
>> @@ -685,6 +702,7 @@ static int macvlan_sync_address(struct net_device *dev, unsigned
>> char *addr)
>>  {
>>      struct macvlan_dev *vlan = netdev_priv(dev);
>>      struct net_device *lowerdev = vlan->lowerdev;
>> +    struct macvlan_port *port = vlan->port;
>>      int err;
>>
>>      if (!(dev->flags & IFF_UP)) {
>> @@ -695,7 +713,7 @@ static int macvlan_sync_address(struct net_device *dev, unsigned
>> char *addr)
>>          if (macvlan_addr_busy(vlan->port, addr))
>>              return -EBUSY;
>>
>> -        if (!macvlan_passthru(vlan->port)) {
>> +        if (!macvlan_passthru(port)) {
>>              err = dev_uc_add(lowerdev, addr);
>>              if (err)
>>                  return err;
>> @@ -705,6 +723,15 @@ static int macvlan_sync_address(struct net_device *dev, unsigned
>> char *addr)
>>
>>          macvlan_hash_change_addr(vlan, addr);
>>      }
>> +    if (macvlan_passthru(port) && !macvlan_addr_change(port)) {
>> +        /* Since addr_change isn't set, we are here due to lower
>> +         * device change.  Save the lower-dev address so we can
>> +         * restore it later.
>> +         */
>> +        ether_addr_copy(vlan->port->perm_addr,
>> +                dev->dev_addr);
> 
> Did you meant to copy `addr' here? Since dev->dev_addr is that of the macvlan device
> whilst `addr' is from the lower parent device.
> 

At this point, it doesn't really matter since dev_addr has already been set in
hash_change_addr().  However, I see your point, and the intent would be clarified
if I used lower_dev->addr.

Thanks
-vlad
> 
> Thanks,
> ~Girish
> 
>