[ovs-dev] [netlink v5 56/61] datapath: Convert ODP_FLOW_* commands to use AF_NETLINK socket layer.

[Ben Pfaff]

On Thu, Jan 27, 2011 at 07:57:20PM -0800, Jesse Gross wrote:
> On Wed, Jan 26, 2011 at 4:23 PM, Ben Pfaff <blp at nicira.com> wrote:
> > diff --git a/datapath/datapath.c b/datapath/datapath.c
> > index 36cc803..b2354d7 100644
> > --- a/datapath/datapath.c
> > +++ b/datapath/datapath.c
> > +static int odp_flow_cmd_del(struct sk_buff *skb, struct genl_info *info)
> [...]
> > - ? ? ? err = copy_flow_to_user(uodp_flow, dp, flow, flowcmd.total_len,
> > - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ((u64)bucket << 32) | obj);
> > + ? ? ? flow_deferred_free(flow);
> >
> > -exit_kfree_skb:
> > - ? ? ? kfree_skb(skb);
> > -exit:
> > - ? ? ? return err;
> > + ? ? ? reply = odp_flow_cmd_build_info(flow, dp, info->snd_pid,
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? info->snd_seq, ODP_FLOW_CMD_DEL);
> 
> We're not holding rcu_read_lock here, so flow_deferred_free() could
> free flow immediately but then we use it in the next line.

At first I was just going to fix the ordering, but then I saw that it
was better to allocate room for the reply before we remove it from the
table, and then to fill it afterward.  So I ended up with the following
(as a context diff this time because it seems more readable in this
case).  Compile tested only:

diff --git a/datapath/datapath.c b/datapath/datapath.c
index b2354d7..2c26aa6 100644
*** a/datapath/datapath.c
--- b/datapath/datapath.c
***************
*** 879,890 ****
  	return err;
  }
  
! static struct sk_buff *odp_flow_cmd_build_info(struct sw_flow *flow, struct datapath *dp,
! 					       u32 pid, u32 seq, u8 cmd)
  {
  	const struct sw_flow_actions *sf_acts;
- 	struct sk_buff *skb;
- 	int retval;
  	int len;
  
  	sf_acts = rcu_dereference_protected(flow->sf_acts,
--- 879,887 ----
  	return err;
  }
  
! static struct sk_buff *odp_flow_cmd_alloc_info(struct sw_flow *flow)
  {
  	const struct sw_flow_actions *sf_acts;
  	int len;
  
  	sf_acts = rcu_dereference_protected(flow->sf_acts,
***************
*** 895,901 ****
  	len += nla_total_size(sizeof(struct odp_flow_stats)); /* ODP_FLOW_ATTR_STATS */
  	len += nla_total_size(1); /* ODP_FLOW_ATTR_TCP_FLAGS */
  	len += nla_total_size(8); /* ODP_FLOW_ATTR_USED */
! 	skb = genlmsg_new(NLMSG_ALIGN(sizeof(struct odp_header)) + len, GFP_KERNEL);
  	if (!skb)
  		return ERR_PTR(-ENOMEM);
  
--- 892,907 ----
  	len += nla_total_size(sizeof(struct odp_flow_stats)); /* ODP_FLOW_ATTR_STATS */
  	len += nla_total_size(1); /* ODP_FLOW_ATTR_TCP_FLAGS */
  	len += nla_total_size(8); /* ODP_FLOW_ATTR_USED */
! 	return genlmsg_new(NLMSG_ALIGN(sizeof(struct odp_header)) + len, GFP_KERNEL);
! }
! 
! static struct sk_buff *odp_flow_cmd_build_info(struct sw_flow *flow, struct datapath *dp,
! 					       u32 pid, u32 seq, u8 cmd)
! {
! 	struct sk_buff *skb;
! 	int retval;
! 
! 	skb = odp_flow_cmd_alloc_info(flow);
  	if (!skb)
  		return ERR_PTR(-ENOMEM);
  
***************
*** 1102,1124 ****
  	flow_node = tbl_lookup(table, &key, flow_hash(&key), flow_cmp);
  	if (!flow_node)
  		return -ENOENT;
  
  	err = tbl_remove(table, flow_node);
! 	if (err)
  		return err;
  
! 	flow = flow_cast(flow_node);
! 	flow_deferred_free(flow);
  
! 	reply = odp_flow_cmd_build_info(flow, dp, info->snd_pid,
! 					info->snd_seq, ODP_FLOW_CMD_DEL);
  
! 	if (!IS_ERR(reply))
! 		genl_notify(reply, genl_info_net(info), info->snd_pid,
! 			    dp_flow_multicast_group.id, info->nlhdr, GFP_KERNEL);
! 	else
! 		netlink_set_err(INIT_NET_GENL_SOCK, 0,
! 				dp_flow_multicast_group.id, PTR_ERR(reply));
  	return 0;
  }
  
--- 1108,1133 ----
  	flow_node = tbl_lookup(table, &key, flow_hash(&key), flow_cmp);
  	if (!flow_node)
  		return -ENOENT;
+ 	flow = flow_cast(flow_node);
+ 
+ 	reply = odp_flow_cmd_alloc_info(flow);
+ 	if (!reply)
+ 		return -ENOMEM;
  
  	err = tbl_remove(table, flow_node);
! 	if (err) {
! 		kfree_skb(reply);
  		return err;
+ 	}
  
! 	err = odp_flow_cmd_fill_info(flow, dp, reply, info->snd_pid,
! 				     info->snd_seq, 0, ODP_FLOW_CMD_DEL);
! 	BUG_ON(err < 0);
  
! 	flow_deferred_free(flow);
  
! 	genl_notify(reply, genl_info_net(info), info->snd_pid,
! 		    dp_flow_multicast_group.id, info->nlhdr, GFP_KERNEL);
  	return 0;
  }