[ovs-dev] [netlink v4 49/52] datapath: Convert ODP_DP_* commands to use AF_NETLINK socket layer.

[Ben Pfaff]

On Fri, Jan 21, 2011 at 06:38:21PM -0800, Jesse Gross wrote:
> On Tue, Jan 11, 2011 at 9:50 PM, Ben Pfaff <blp at nicira.com> wrote:
> > diff --git a/datapath/datapath.c b/datapath/datapath.c
> > index 5fea1af..5d4e1f2 100644
> > --- a/datapath/datapath.c
> > +++ b/datapath/datapath.c
> > +static int odpdc_new(struct sk_buff *skb, struct genl_info *info)
> [...]
> > - ? ? ? return 0;
> > + ? ? ? return genlmsg_reply(reply, info);
> 
> Should we set an error on the socket if the reply/message creation
> fails?  That's what we (and the bridge) do if new port notification
> fail.

I think you are talking about this code in dp_ifinfo_notify():

            rtnl_notify(skb, &init_net, 0, RTNLGRP_LINK, NULL, GFP_KERNEL);
            return;
    errout:
            if (err < 0)
                    rtnl_set_sk_err(&init_net, RTNLGRP_LINK, err);

The need for this is specific to multicasting notifications.  If you
chase down the tree of functions called by genlmsg_reply(), you end up
in netlink_attachskb(), which calls netlink_overrun() if the socket
buffer is full, and netlink_overrun() in turn sets the socket error code
to -ENOBUFS.

> It's also not really clear to me when we should send a reply.  We have:
> * New objects unicast a response back.
> * Flows unicast a response on deletion.
> * Ports multicast notifications on creation and deletion as the bridge
> does through RTNL.

Yes, this needs to be made consistent.  I believe that the "correct" way
to do it is that each type of object should multicast its status on
creation, deletion, or change.  (I think that the goal is supposed to be
to allow userspace to accurately track the changes in a table over time;
that's essentially what libnl allows one to do.)  This is on my to-do
list, along with using sk_buff frags for long sets of flow actions.  It
shouldn't be too difficult; I was just tired when I wrote this code.

> > -static int modify_datapath(unsigned int cmd, struct odp_datapath __user *uodp_datapath)
> > +static int odpdc_modify(struct sk_buff *skb, struct genl_info *info)
> > ?{
> > - ? ? ? struct nlattr *a[ODPDT_MAX + 1];
> > ? ? ? ?struct datapath *dp;
> > - ? ? ? struct sk_buff *skb;
> > ? ? ? ?int err;
> >
> > - ? ? ? skb = copy_datapath_from_user(uodp_datapath, a);
> > - ? ? ? err = PTR_ERR(skb);
> > - ? ? ? if (IS_ERR(skb))
> > + ? ? ? err = odpdc_validate(info->attrs);
> > + ? ? ? if (err)
> > ? ? ? ? ? ? ? ?goto exit;
> >
> > ? ? ? ?rtnl_lock();
> > ? ? ? ?mutex_lock(&dp_mutex);
> > - ? ? ? dp = lookup_datapath((struct odp_datapath *)skb->data, a);
> > + ? ? ? dp = lookup_datapath(info->userhdr, info->attrs);
> > ? ? ? ?err = PTR_ERR(dp);
> > ? ? ? ?if (IS_ERR(dp))
> > - ? ? ? ? ? ? ? goto exit_free;
> > + ? ? ? ? ? ? ? goto exit_unlock;
> >
> > - ? ? ? if (cmd == ODP_DP_DEL)
> > + ? ? ? /* XXX need to compose reply */
> 
> Do we need to send a response here?

Yes.  I think that the fix is as simple as the following, which builds
OK, except that as I said we really need to multicast the response.  

diff --git a/datapath/datapath.c b/datapath/datapath.c
index c22cc9d..95e8ba7 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -1418,6 +1418,7 @@ err:
 static int odpdc_del(struct sk_buff *skb, struct genl_info *info)
 {
 	struct vport *vport, *next_vport;
+	struct sk_buff *reply;
 	struct datapath *dp;
 	int err;
 
@@ -1432,6 +1433,11 @@ static int odpdc_del(struct sk_buff *skb, struct genl_info *info)
 	if (IS_ERR(dp))
 		goto exit_unlock;
 
+	reply = odpdc_build_info(dp, info->snd_pid, info->snd_seq, ODPDC_DEL);
+	err = PTR_ERR(reply);
+	if (IS_ERR(reply))
+		goto exit_unlock;
+
 	list_for_each_entry_safe (vport, next_vport, &dp->port_list, node)
 		if (vport->port_no != ODPP_LOCAL)
 			dp_detach_port(vport);
@@ -1443,7 +1449,7 @@ static int odpdc_del(struct sk_buff *skb, struct genl_info *info)
 	call_rcu(&dp->rcu, destroy_dp_rcu);
 	module_put(THIS_MODULE);
 
-	err = 0;
+	err = genlmsg_reply(reply, info);
 
 exit_unlock:
 	mutex_unlock(&dp_mutex);


> > +static int odpdc_get(struct sk_buff *skb, struct genl_info *info)
> > ?{
> > - ? ? ? struct nlattr *a[ODPDT_MAX + 1];
> > - ? ? ? struct odp_datapath *odp_datapath;
> > + ? ? ? struct sk_buff *reply;
> > ? ? ? ?struct datapath *dp;
> > - ? ? ? struct sk_buff *skb;
> > ? ? ? ?int err;
> >
> > - ? ? ? skb = copy_datapath_from_user(uodp_datapath, a);
> > - ? ? ? err = PTR_ERR(skb);
> > - ? ? ? if (IS_ERR(skb))
> > - ? ? ? ? ? ? ? goto exit;
> > - ? ? ? odp_datapath = (struct odp_datapath *)skb->data;
> > -
> > ? ? ? ?mutex_lock(&dp_mutex);
> > - ? ? ? dp = lookup_datapath(odp_datapath, a);
> > -
> > + ? ? ? dp = lookup_datapath(info->userhdr, info->attrs);
> 
> Don't we need to validate first?  Specifically, we need to check that
> the name is NULL terminated on older kernels.

Oops, you're right.  Fixed as the following; again, build-tested only:

diff --git a/datapath/datapath.c b/datapath/datapath.c
index 95e8ba7..45eaf12 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -1490,6 +1490,10 @@ static int odpdc_get(struct sk_buff *skb, struct genl_info *info)
 	struct datapath *dp;
 	int err;
 
+	err = odpdc_validate(info->attrs);
+	if (err)
+		goto exit;
+
 	mutex_lock(&dp_mutex);
 	dp = lookup_datapath(info->userhdr, info->attrs);
 	err = PTR_ERR(dp);
@@ -1505,6 +1509,7 @@ static int odpdc_get(struct sk_buff *skb, struct genl_info *info)
 
 exit_unlock:
 	mutex_unlock(&dp_mutex);
+exit:
 	return err;
 }