netdev - Re: [PATCH 2/6] C/R: Basic support for network namespaces and devices (v5)

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]

Message-ID: <871vfuiul6.fsf@caffeine.danplanet.com>
Date:	Mon, 08 Mar 2010 09:36:37 -0800
From:	Dan Smith <danms@...ibm.com>
To:	Oren Laadan <orenl@...columbia.edu>
Cc:	containers@...ts.osdl.org, den@...nvz.org, netdev@...r.kernel.org,
	davem@...emloft.net, ebiederm@...ssion.com, benjamin.thery@...l.net
Subject: Re: [PATCH 2/6] C/R: Basic support for network namespaces and devices (v5)

OL> I'm confused: in checkpoint_ns() inside the for_each_netdev() loop
OL> you first test for dev->netdev_ops->ndo_checkpoint and then call
OL> checkpoint_obj(... CKPT_OBJ_NETDEV) - which in turn will call
OL> checkpoint_netdev(), which will again test for
dev-> netdev_ops->ndo_checkpoint ...  am I reading it wrongly ?

In the case of veth, yes.  It goes something like this:

checkpoint_netns() {
  foreach netdev in netns {
    checkpoint_netdev {
      if netdev is veth {
        checkpoint_peer(); // Will call checkpoint_netdev again
      }
    }
  }
}

It shouldn't happen, but it seems like since we could potentially add
another checkpoint_obj(mydev) somewhere other than in
checkpoint_netdev(), it is reasonable to check that there is actually
something to call before we call it.

Would you prefer a BUG()?

OL> How about this - to me it feels simpler:

OL> 	dev = rtnl_newlink(veth_new_link_msg, &veth, this_name);
OL> 	if (IS_ERR(dev))
OL> 		return dev;

OL> 	peer = dev_get_by_name(current->nsproxy->net_ns, peer_name);
OL> 	if (!peer) {
OL> 		ret = -EINVAL;
OL> 		goto err_dev;
OL> 	}
OL> 	ret = ckpt_obj_insert(ctx, peer, h->veth.peer_ref,
OL> 			      CKPT_OBJ_NETDEV);
OL> 	if (ret < 0)
OL> 		goto err_peer;

OL> 	dev_put(peer);

OL> 	dq.dev = dev;
OL> 	dq.peer = peer;
OL> 	ret = deferqueue_add(ctx->deferqueue, &dq, sizeof(dq),
OL> 			     netdev_noop, netdev_cleanup);
OL> 	if (ret)
OL> 		goto err_peer;

If you fail here you need to unregister_netdev() because the dev_put()
that the objhash will not cause it to happen.  Unless we add something
to allow you to remove your object from the hash, you can't prevent
that final put, so you have to have it in the deferqueue for
later.  You can't check the refcount in the objhash function because it
will differ depending on the number of addresses and protocols the
device has, and those don't get released until unregister_netdev()
which will block if you call it before you've released all of your
references.  If the objhash put function could examine ctx->errno,
then it could drop its reference and then call unregister_netdev(),
but that would involve changing all the drop functions.  What am I
missing?

-- 
Dan Smith
IBM Linux Technology Center
email: danms@...ibm.com
--
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