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 <vyase...@redhat.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
>
>
