netdev - Re: [PATCH] net: Correctly sync addresses from multiple sources to single device

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

Date:	Wed, 22 Jan 2014 18:18:21 +0400
From:	Andrey Dmitrov <andrey.dmitrov@...etlabs.ru>
To:	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/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.

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