On Mon, Mar 25, 2013 at 9:39 AM, Jesse Gross <je...@nicira.com> wrote:
> On Thu, Mar 21, 2013 at 8:08 PM, Pravin Shelar <pshe...@nicira.com> wrote:
>> On Wed, Mar 20, 2013 at 4:44 PM, Jesse Gross <je...@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 <je...@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.
ok,
Acked-by: Pravin B Shelar <pshe...@nicira.com>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
