| 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
| ||
|
Message-ID: <52DFDE89.4060905@gmail.com>
Date: Wed, 22 Jan 2014 10:06:49 -0500
From: Vlad Yasevich <vyasevich@...il.com>
To: Andrey Dmitrov <andrey.dmitrov@...etlabs.ru>,
Vlad Yasevich <vyasevic@...hat.com>
CC: netdev@...r.kernel.org, davem@...emloft.net,
"Alexandra N. Kossovsky" <Alexandra.Kossovsky@...etlabs.ru>,
Konstantin Ushakov <Konstantin.Ushakov@...etlabs.ru>
Subject: Re: [PATCH] net: Correctly sync addresses from multiple sources to
single device
On 01/22/2014 09:18 AM, Andrey Dmitrov wrote:
> On 01/17/2014 11:52 PM, Vlad Yasevich wrote:
>> When we have multiple devices attempting to sync the same address
>> to a single destination, each device should be permitted to sync
>> it once. To accomplish this, pass the sync count of the source
>> address to __hw_addr_add_ex(). 'sync_cnt' tracks how many time
>> a given address has been successfully synced. If the address
>> is found in the destination list, but the 'sync_cnt' of the source
>> is 0, then this address has not been synced from this interface
>> and we need to allow the sync operation to succeed thus incrementing
>> reference counts.
>>
>> Reported-by: Andrey Dmitrov <andrey.dmitrov@...etlabs.ru>
>> CC: Andrey Dmitrov <andrey.dmitrov@...etlabs.ru>
>> Signed-off-by: Vlad Yasevich <vyasevic@...hat.com>
>> ---
>> net/core/dev_addr_lists.c | 12 +++++++-----
>> 1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
>> index ec40a84..bd1b880 100644
>> --- a/net/core/dev_addr_lists.c
>> +++ b/net/core/dev_addr_lists.c
>> @@ -48,7 +48,8 @@ static int __hw_addr_create_ex(struct
>> netdev_hw_addr_list *list,
>> static int __hw_addr_add_ex(struct netdev_hw_addr_list *list,
>> const unsigned char *addr, int addr_len,
>> - unsigned char addr_type, bool global, bool sync)
>> + unsigned char addr_type, bool global, bool sync,
>> + int sync_count)
>> {
>> struct netdev_hw_addr *ha;
>> @@ -66,7 +67,7 @@ static int __hw_addr_add_ex(struct
>> netdev_hw_addr_list *list,
>> ha->global_use = true;
>> }
>> if (sync) {
>> - if (ha->synced)
>> + if (ha->synced && sync_count)
>> return -EEXIST;
>> else
>> ha->synced = true;
>> @@ -84,7 +85,8 @@ static int __hw_addr_add(struct netdev_hw_addr_list
>> *list,
>> const unsigned char *addr, int addr_len,
>> unsigned char addr_type)
>> {
>> - return __hw_addr_add_ex(list, addr, addr_len, addr_type, false,
>> false);
>> + return __hw_addr_add_ex(list, addr, addr_len, addr_type, false,
>> false,
>> + 0);
>> }
>> static int __hw_addr_del_entry(struct netdev_hw_addr_list *list,
>> @@ -139,7 +141,7 @@ static int __hw_addr_sync_one(struct
>> netdev_hw_addr_list *to_list,
>> int err;
>> err = __hw_addr_add_ex(to_list, ha->addr, addr_len, ha->type,
>> - false, true);
>> + false, true, ha->sync_count);
>> if (err && err != -EEXIST)
>> return err;
>> @@ -676,7 +678,7 @@ static int __dev_mc_add(struct net_device *dev,
>> const unsigned char *addr,
>> netif_addr_lock_bh(dev);
>> err = __hw_addr_add_ex(&dev->mc, addr, dev->addr_len,
>> - NETDEV_HW_ADDR_T_MULTICAST, global, false);
>> + NETDEV_HW_ADDR_T_MULTICAST, global, false, 0);
>> if (!err)
>> __dev_set_rx_mode(dev);
>> netif_addr_unlock_bh(dev);
>
> Thanks. The patch was tested with linux-3.12.6.
>
> However, while building the kernel I got complains on:
>
>> err = __hw_addr_add_ex(to_list, ha->addr, addr_len, ha->type,
>> - false, true);
>> + false, true, ha->sync_count);
>
> Namely ha (struct netdev_hw_addr) has no field sync_count. It has sync_cnt.
> So I’ve fixed this when testing the patch. Is it a typo or something
> intentional?
>
> Your patch solves the initial problem, but it looks like it introduces a
> new one.
>
> When interface joins multicast group corresponding MAC address is added
> to the interface maddr list. It should be removed when socket explicitly
> leaves the group or is just closed. It’s the case without your patch.
> However, with the patched kernel address is still there after socket is
> closed.
>
> You can reproduce it with mcast_client binary (mcast_client.c was
> attached to the first letter in the thread). To be specific:
>
> $ ip maddr show eth3
> 9: eth3
> link 33:33:00:00:00:01 users 3
> link 01:00:5e:00:00:01 users 3
> link 33:33:ff:01:39:7c users 3
> link 01:80:c2:00:00:21 users 2
> inet 224.0.0.1
> inet6 ff02::1:ff01:397c
> inet6 ff02::1
> inet6 ff01::1
>
> $ gcc mcast_client.c -o cl
> $ sudo ./cl
> Abort it with Ctrl+c
> $ ip maddr show eth3
> 9: eth3
> link 33:33:00:00:00:01 users 3
> link 01:00:5e:00:00:01 users 3
> link 33:33:ff:01:39:7c users 3
> link 01:80:c2:00:00:21 users 2
> link 01:00:5e:11:58:a8
> inet 224.0.0.1
> inet6 ff02::1:ff01:397c
> inet6 ff02::1
> inet6 ff01::1
>
> 'link 01:00:5e:11:58:a8’ address is still there. Should’ve been removed.
You are right. Sync really doesn't deal well with with multiple sources
syncing the same address. Looks like we need to keep track of how
many time the address has been synced from the different sources.
Looks like I have to restore
commit 4543fbefe6e06a9e40d9f2b28d688393a299f079. :)
-vlad
>
> Andrey
> --
> 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
--
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
Powered by blists - more mailing lists