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

Jesse Gross jesse at nicira.com
Mon Mar 25 16:39:20 UTC 2013


On Thu, Mar 21, 2013 at 8:08 PM, Pravin Shelar <pshelar at nicira.com> wrote:
> 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.

I went back and forth on this some as well.  We currently have
something similar for flows (which we allocate based on the expected
size).  The default message size should be plenty large for ports but
it certainly is a little riskier.



More information about the dev mailing list