| Message ID | 20171125140212.105418-1-zhanglkk1990@163.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] openvswitch: fix the incorrect flow action alloc size | expand |
On Sat, Nov 25, 2017 at 7:32 PM, zhangliping <zhanglkk1990@163.com> wrote: > From: zhangliping <zhangliping02@baidu.com> > > If we want to add a datapath flow, which has more than 500 vxlan outputs' > action, we will get the following error reports: > openvswitch: netlink: Flow action size 32832 bytes exceeds max > openvswitch: netlink: Flow action size 32832 bytes exceeds max > openvswitch: netlink: Actions may not be safe on all matching packets > ... ... > > It seems that we can simply enlarge the MAX_ACTIONS_BUFSIZE to fix it, but > this is not the root cause. For example, for a vxlan output action, we need > about 60 bytes for the nlattr, but after it is converted to the flow > action, it only occupies 24 bytes. This means that we can still support > more than 1000 vxlan output actions for a single datapath flow under the > the current 32k max limitation. > > So even if the nla_len(attr) is larger than MAX_ACTIONS_BUFSIZE, we > shouldn't report EINVAL and keep it move on, as the judgement can be > done by the reserve_sfa_size. > > Signed-off-by: zhangliping <zhangliping02@baidu.com> Thanks for the patch. Acked-by: Pravin B Shelar <pshelar@ovn.org>
From: zhangliping <zhanglkk1990@163.com> Date: Sat, 25 Nov 2017 22:02:12 +0800 > From: zhangliping <zhangliping02@baidu.com> > > If we want to add a datapath flow, which has more than 500 vxlan outputs' > action, we will get the following error reports: > openvswitch: netlink: Flow action size 32832 bytes exceeds max > openvswitch: netlink: Flow action size 32832 bytes exceeds max > openvswitch: netlink: Actions may not be safe on all matching packets > ... ... > > It seems that we can simply enlarge the MAX_ACTIONS_BUFSIZE to fix it, but > this is not the root cause. For example, for a vxlan output action, we need > about 60 bytes for the nlattr, but after it is converted to the flow > action, it only occupies 24 bytes. This means that we can still support > more than 1000 vxlan output actions for a single datapath flow under the > the current 32k max limitation. > > So even if the nla_len(attr) is larger than MAX_ACTIONS_BUFSIZE, we > shouldn't report EINVAL and keep it move on, as the judgement can be > done by the reserve_sfa_size. > > Signed-off-by: zhangliping <zhangliping02@baidu.com> Applied, thanks.
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c index dc42479..624ea74 100644 --- a/net/openvswitch/flow_netlink.c +++ b/net/openvswitch/flow_netlink.c @@ -2241,14 +2241,11 @@ int ovs_nla_put_mask(const struct sw_flow *flow, struct sk_buff *skb) #define MAX_ACTIONS_BUFSIZE (32 * 1024) -static struct sw_flow_actions *nla_alloc_flow_actions(int size, bool log) +static struct sw_flow_actions *nla_alloc_flow_actions(int size) { struct sw_flow_actions *sfa; - if (size > MAX_ACTIONS_BUFSIZE) { - OVS_NLERR(log, "Flow action size %u bytes exceeds max", size); - return ERR_PTR(-EINVAL); - } + WARN_ON_ONCE(size > MAX_ACTIONS_BUFSIZE); sfa = kmalloc(sizeof(*sfa) + size, GFP_KERNEL); if (!sfa) @@ -2321,12 +2318,15 @@ static struct nlattr *reserve_sfa_size(struct sw_flow_actions **sfa, new_acts_size = ksize(*sfa) * 2; if (new_acts_size > MAX_ACTIONS_BUFSIZE) { - if ((MAX_ACTIONS_BUFSIZE - next_offset) < req_size) + if ((MAX_ACTIONS_BUFSIZE - next_offset) < req_size) { + OVS_NLERR(log, "Flow action size exceeds max %u", + MAX_ACTIONS_BUFSIZE); return ERR_PTR(-EMSGSIZE); + } new_acts_size = MAX_ACTIONS_BUFSIZE; } - acts = nla_alloc_flow_actions(new_acts_size, log); + acts = nla_alloc_flow_actions(new_acts_size); if (IS_ERR(acts)) return (void *)acts; @@ -3059,7 +3059,7 @@ int ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, { int err; - *sfa = nla_alloc_flow_actions(nla_len(attr), log); + *sfa = nla_alloc_flow_actions(min(nla_len(attr), MAX_ACTIONS_BUFSIZE)); if (IS_ERR(*sfa)) return PTR_ERR(*sfa);