On Mon, Mar 25, 2013 at 12:20 PM, Pravin Shelar <pshe...@nicira.com> wrote:
> 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>
Thanks, I added an assert to ovs_vport_cmd_build_info() for
consistency and then pushed to master.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
