[CRIU] [PATCH v2 2/3] inet: remember ipv6 connections' ifindex for restore

[Pavel Emelyanov]

On 11/27/2015 07:02 PM, Tycho Andersen wrote:
> On Wed, Nov 25, 2015 at 12:35:21PM +0300, Pavel Emelyanov wrote:
>>> @@ -298,6 +301,21 @@ static int do_dump_one_inet_fd(int lfd, u32 id, const struct fd_parms *p, int fa
>>>  
>>>  		ie.v6only = val ? true : false;
>>>  		ie.has_v6only = true;
>>> +
>>> +		/* ifindex only matters on source ports for bind, so let's
>>> +		 * find only that ifindex. */
>>> +		if (getsockopt(lfd, SOL_SOCKET, SO_BINDTODEVICE, device, &len) < 0) {
>>> +			pr_perror("can't get ifname");
>>> +			goto err;
>>> +		}
>>> +
>>> +		if (len > 0) {
>>> +			ie.ifname = xstrdup(device);
>>> +			if (!ie.ifname)
>>> +				goto err;
>>> +		} else {
>>> +			pr_warn("couldn't find ifname for %d, can't bind\n", id);
>>
>> The "no device" is a valid situation actually. Would you (incrementally is OK)
>> relax this check to
>>
>> 	else if (len == 0)
>> 		/* do nothing, it's OK not to have a device */
>> 		;
>>
>> ?
> 
> Right, that's why I just put it as a warning, because it's not
> necessarily fatal unless there is something bound to this device.
> Anyway, I can send another version (or patch on top if you happen to
> apply this before monday).

OK, I've applied 2 and 3. About the 1st one -- Andrey had some concern about it,
I've asked him to send his thoughts on the mailing list.

>>> +		}
>>>  	}
>>>  
>>>  	ie.src_addr = xmalloc(pb_repeated_size(&ie, src_addr));
>>> @@ -607,7 +625,7 @@ union sockaddr_inet {
>>>  };
>>>  
>>>  static int restore_sockaddr(union sockaddr_inet *sa,
>>> -		int family, u32 pb_port, u32 *pb_addr)
>>> +		int family, u32 pb_port, u32 *pb_addr, u32 ifindex)
>>>  {
>>>  	BUILD_BUG_ON(sizeof(sa->v4.sin_addr.s_addr) > PB_ALEN_INET * sizeof(u32));
>>>  	BUILD_BUG_ON(sizeof(sa->v6.sin6_addr.s6_addr) > PB_ALEN_INET6 * sizeof(u32));
>>> @@ -625,6 +643,12 @@ static int restore_sockaddr(union sockaddr_inet *sa,
>>>  		sa->v6.sin6_family = AF_INET6;
>>>  		sa->v6.sin6_port = htons(pb_port);
>>>  		memcpy(sa->v6.sin6_addr.s6_addr, pb_addr, sizeof(sa->v6.sin6_addr.s6_addr));
>>> +
>>> +		/* Here although the struct member is called scope_id, the
>>> +		 * kernel really wants ifindex. See
>>> +		 * /net/ipv6/af_inet6.c:inet6_bind for details.
>>
>> The inet6_bind doesn't _always_ require a device, but only for __ipv6_addr_needs_scope_id :)
>> Do I get it right that for those cases we will have ie->ifname == NULL at dump?
> 
> I think you mean ie->ifname != NULL here? I don't know, but I sure
> hope so :). If the device doesn't have an ifname, I don't know how we
> can resolve the ifindex to pass to bind() on restore. Since the
> original socket creator would have this restriction too, I think it
> *should* always work :)

OK, that's how I thought. Would you also add this as an additional code comment?

-- Pavel