[ovs-dev] [PATCH] datapath: Preallocate reply skb in ovs_vport_cmd_set().

[Pravin Shelar]

On Wed, Mar 20, 2013 at 4:44 PM, Jesse Gross <jesse at nicira.com> wrote:
> Allocation of the Netlink notification skb can potentially fail
> after changing vport configuration.  In general, we try to avoid
> this by undoing any change we made but that is difficult for existing
> objects.  This avoids the problem by preallocating the buffer (which
> is fixed size).
>
> Signed-off-by: Jesse Gross <jesse at nicira.com>
> ---
>  datapath/datapath.c |   23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index b5eb232..88f5371 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -2005,10 +2005,16 @@ static int ovs_vport_cmd_set(struct sk_buff *skb, struct genl_info *info)
>             nla_get_u32(a[OVS_VPORT_ATTR_TYPE]) != vport->ops->type)
>                 err = -EINVAL;
>
> +       reply = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> +       if (!reply) {
> +               err = -ENOMEM;
> +               goto exit_unlock;
> +       }
> +
>         if (!err && a[OVS_VPORT_ATTR_OPTIONS])
>                 err = ovs_vport_set_options(vport, a[OVS_VPORT_ATTR_OPTIONS]);
>         if (err)
> -               goto exit_unlock;
> +               goto exit_free;
>
>         if (a[OVS_VPORT_ATTR_STATS])
>                 ovs_vport_set_stats(vport, nla_data(a[OVS_VPORT_ATTR_STATS]));
> @@ -2016,17 +2022,18 @@ static int ovs_vport_cmd_set(struct sk_buff *skb, struct genl_info *info)
>         if (a[OVS_VPORT_ATTR_UPCALL_PID])
>                 vport->upcall_portid = nla_get_u32(a[OVS_VPORT_ATTR_UPCALL_PID]);
>
> -       reply = ovs_vport_cmd_build_info(vport, info->snd_portid,
> -                                        info->snd_seq, OVS_VPORT_CMD_NEW);
> -       if (IS_ERR(reply)) {
> -               netlink_set_err(GENL_SOCK(sock_net(skb->sk)), 0,
> -                               ovs_dp_vport_multicast_group.id, PTR_ERR(reply));
> -               goto exit_unlock;
> -       }
> +       err = ovs_vport_cmd_fill_info(vport, reply, info->snd_portid,
> +                                     info->snd_seq, 0, OVS_VPORT_CMD_NEW);
> +       BUG_ON(err < 0);
>
I am not sure about BUG_ON() ?
ovs_vport_cmd_build_info() does not have this assert.

>         genl_notify(reply, genl_info_net(info), info->snd_portid,
>                     ovs_dp_vport_multicast_group.id, info->nlhdr, GFP_KERNEL);
>
> +       rtnl_unlock();
> +       return 0;
> +
> +exit_free:
> +       kfree_skb(reply);
>  exit_unlock:
>         rtnl_unlock();
>  exit:
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev