Re: [PATCH net-next] net: ipv6: add tokenized interface identifier support

[Daniel Borkmann <dborkman@redhat.com>]

On 04/09/2013 12:21 AM, Hannes Frederic Sowa wrote:
> Sorry, I was a bit busy and just had more time to look at your patch
> again. Perhaps you could look into my comments. (A new patch would be
> needed as it already landed in net-next).

Right, I'll prepare this follow-up patch(es) by tomorrow. Thanks for your
feedback Hannes!

> On Mon, Apr 08, 2013 at 04:01:30PM +0200, Daniel Borkmann wrote:
>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>> index a33b157..65d8139 100644
>> --- a/net/ipv6/addrconf.c
>> +++ b/net/ipv6/addrconf.c
>> @@ -422,6 +422,7 @@ static struct inet6_dev *ipv6_add_dev(struct net_device *dev)
>>   		ipv6_regen_rndid((unsigned long) ndev);
>>   	}
>>   #endif
>> +	memset(ndev->token.s6_addr, 0, sizeof(ndev->token.s6_addr));
>
> ndev is allocated with __GFP_ZERO so no need to clear it. Otherwise I would do
>
> ndev->token = in6addr_any;
>
> to make the check in addrconf_prefix_rcv more clear.
>
>> +static int inet6_set_iftoken(struct inet6_dev *idev, struct in6_addr *token)
>> +{
>> +	struct in6_addr ll_addr;
>> +	struct inet6_ifaddr *ifp;
>> +	struct net_device *dev = idev->dev;
>> +
>> +	if (token == NULL)
>> +		return -EINVAL;
>> +	if (ipv6_addr_any(token))
>> +		return -EINVAL;
>> +	if (dev->flags & (IFF_LOOPBACK | IFF_NOARP))
>> +		return -EINVAL;
>> +	if (idev->dead || !(idev->if_flags & IF_READY))
>> +		return -EINVAL;
>> +	if (!ipv6_accept_ra(idev))
>> +		return -EINVAL;
>> +	if (idev->cnf.rtr_solicits <= 0)
>> +		return -EINVAL;
>
> IF_READY marks an interface which is already up. So we are not allowed to set
> a token on an interface which is down? I would drop this requirement, seems
> like a usability issue. ;)
>
>> +
>> +	write_lock_bh(&idev->lock);
>> +
>> +	BUILD_BUG_ON(sizeof(token->s6_addr) != 16);
>> +	memcpy(idev->token.s6_addr + 8, token->s6_addr + 8, 8);
>> +
>> +	write_unlock_bh(&idev->lock);
>> +
>
> {
>> +	ipv6_get_lladdr(dev, &ll_addr, IFA_F_TENTATIVE | IFA_F_OPTIMISTIC);
>> +	ndisc_send_rs(dev, &ll_addr, &in6addr_linklocal_allrouters);
>> +
>> +	write_lock_bh(&idev->lock);
>> +	idev->if_flags |= IF_RS_SENT;
> }
>
> This should then be only called if IF_READY is set. Otherwise normal ifup
> handling will send out the rs. If one day there is the possibility to add more
> than one token we would actually have to check the minimum solicitation
> intervals. I think this does not matter now.
>
>> +
>> +	/* Well, that's kinda nasty ... */
>> +	list_for_each_entry(ifp, &idev->addr_list, if_list) {
>> +		spin_lock(&ifp->lock);
>> +		if (ipv6_addr_src_scope(&ifp->addr) ==
>> +		    IPV6_ADDR_SCOPE_GLOBAL) {
>> +			ifp->valid_lft = 0;
>> +			ifp->prefered_lft = 0;
>> +		}
>> +		spin_unlock(&ifp->lock);
>> +	}
>
> As I understand this logic it also does deprecate current statically configured ip
> addresses? Perhaps another per-inet6_ifaddr flag to mark the ip address as
> token configured and just clean these address.
>
> The flag would have to be set in addrconf_prefix_rcv if tokens are active.
>
> Thanks,
>
>    Hannes
>