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

[Tycho Andersen]

On Mon, Nov 30, 2015 at 11:48:12AM +0300, Pavel Emelyanov wrote:
> 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?

Yep, sounds good. Perhaps we can copy __ipv6_addr_needs_scope_id (or
something similar) to the dumping code, and then fail dump when we
require a scope id, dropping that pr_warn. Anyway, I'll take a look
and send a patch.

Tycho