[CRIU] [PATCH v2 5/6] net: add support for macvlan link types

[Tycho Andersen]

Hi Pavel,

On Mon, Oct 17, 2016 at 02:09:22PM +0300, Pavel Emelyanov wrote:
> > -	ret = dump(&netdev, fds, tb);
> > +	if (tb[IFLA_LINKINFO]) {
> > +		ret = nla_parse_nested(info, IFLA_INFO_MAX, tb[IFLA_LINKINFO], NULL);
> > +		if (ret < 0) {
> > +			pr_err("failed to parse nested linkinfo\n");
> > +			return -1;
> > +		}
> > +		arg = info;
> 
> Shouldn't this rather be in dump_macvlan()? The tb is there already :)

Oh, this is actually a bad section of this patch. I was thinking we
should only pass link info instead of all the tb, because of your
request on the last series to have some symmetry in the way we
checkpoint in restore. So I think this should be folded in to the
previous patch where we add the arg, and the arg should be renamed.
I'll do that for the next version.

> > +	if (extras) {
> > +		if (extras->netns_id >= 0)
> > +			addattr_l(&req->h, sizeof(*req), IFLA_LINK_NETNSID, &extras->netns_id, sizeof(extras->netns_id));
> > +
> > +		if (extras->link >= 0)
> > +			addattr_l(&req->h, sizeof(*req), IFLA_LINK, &extras->link, sizeof(extras->link));
> > +
> > +		if (extras->target_netns >= 0) {
> > +			addattr_l(&req->h, sizeof(*req), IFLA_NET_NS_FD, &extras->target_netns, sizeof(extras->target_netns));
> > +		}
> > +
> > +	}
> 
> Shouldn't this be in the macvlan_link_info()?

It could be, but I thought the idea was to have "common" properties
restored in the populate_newlink_req() function. If we want to move
these, we have to do something clever about how we're wrapping the
link_info call in populate, since it already adds an IFLA_LINKINFO
message before calling the link_info handler.

> > +		struct newlink_extras extras = {
> > +			.netns_id = -1,
> > +			.link = -1,
> > +			.target_netns = -1,
> > +		};
> > +		char key[100], *val;
> > +
> > +		snprintf(key, sizeof(key), "macvlan[%s]", nde->name);
> > +		val = external_lookup_data(key);
> > +		if (IS_ERR_OR_NULL(val)) {
> > +			pr_err("a macvlan parent for %s is required\n", nde->name);
> > +			return -1;
> > +		}
> > +
> > +		extras.link = (int) (unsigned long) val;
> >  
> > +		extras.netns_id = get_criu_netnsid(nlsk);
> > +		if (extras.netns_id < 0) {
> > +			pr_err("failed to get criu's netnsid\n");
> > +			return -1;
> > +		}
> > +
> > +		if (root_ns_mask & CLONE_NEWUSER) {
> > +			struct newlink_req req;
> > +
> > +			if (populate_newlink_req(&req, RTM_NEWLINK, nde, macvlan_link_info, &extras) < 0)
> > +				return -1;
> > +
> > +			if (userns_call(userns_restore_one_link, 0, &req, sizeof(req), -1) < 0) {
> > +				pr_err("couldn't restore macvlan interface %s via usernsd\n", nde->name);
> > +				return -1;
> > +			}
> > +		} else {
> > +			int my_netns, ret, root_nlsk;
> > +			int ns_fd = get_service_fd(NS_FD_OFF);
> > +
> > +			my_netns = open_proc(PROC_SELF, "ns/net");
> > +			if (my_netns < 0) {
> > +				pr_perror("couldn't get my netns");
> > +				return -1;
> > +			}
> > +
> > +			extras.target_netns = my_netns;
> 
> For userns case the target netns is identified by pid.
> Can we do it the same way here?

Yep, I think so. I just used this because we already had the ns fd
open, but I can switch it.

> > +
> > +			if (setns(ns_fd, CLONE_NEWNET) < 0) {
> > +				close(my_netns);
> > +				pr_perror("couldn't setns to parent ns");
> > +				return -1;
> > +			}
> > +
> > +			root_nlsk = socket(PF_NETLINK, SOCK_RAW, NETLINK_ROUTE);
> 
> So we need a socket in criu netns for both cases -- userns and
> non-userns, don't we?

Yes, unfortunately we can't use the same code path, because the skb
needs to have CAP_NET_ADMIN for the netns referred to by
IFLA_LINK_NETNSID, which we don't in the userns case, so we have to
ask usernsd to do the create.

I'll make all the other changes you've suggested.

Thanks for the review!

Tycho