| Message ID | 74266291a0aba929919f71ff3dbd1c36392bb4c4.1592567032.git.lorenzo@kernel.org |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | [ovs-dev,net] openvswitch: take into account de-fragmentation in execute_check_pkt_len | expand |
On Fri, Jun 19, 2020 at 4:48 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > ovs connection tracking module performs de-fragmentation on incoming > fragmented traffic. Take info account if traffic has been de-fragmented > in execute_check_pkt_len action otherwise we will perform the wrong > nested action considering the original packet size. This issue typically > occurs if ovs-vswitchd adds a rule in the pipeline that requires connection > tracking (e.g. OVN stateful ACLs) before execute_check_pkt_len action. > > Fixes: 4d5ec89fc8d14 ("net: openvswitch: Add a new action check_pkt_len") > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > --- > net/openvswitch/actions.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c > index fc0efd8833c8..9f4dd64e53bb 100644 > --- a/net/openvswitch/actions.c > +++ b/net/openvswitch/actions.c > @@ -1169,9 +1169,10 @@ static int execute_check_pkt_len(struct datapath *dp, struct sk_buff *skb, > struct sw_flow_key *key, > const struct nlattr *attr, bool last) > { > + struct ovs_skb_cb *ovs_cb = OVS_CB(skb); > const struct nlattr *actions, *cpl_arg; > const struct check_pkt_len_arg *arg; > - int rem = nla_len(attr); > + int len, rem = nla_len(attr); > bool clone_flow_key; > > /* The first netlink attribute in 'attr' is always > @@ -1180,7 +1181,8 @@ static int execute_check_pkt_len(struct datapath *dp, struct sk_buff *skb, > cpl_arg = nla_data(attr); > arg = nla_data(cpl_arg); > > - if (skb->len <= arg->pkt_len) { > + len = ovs_cb->mru ? ovs_cb->mru : skb->len; > + if (len <= arg->pkt_len) { We could also check for the segmented packet and use segment length for this check.
> On Fri, Jun 19, 2020 at 4:48 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > > > ovs connection tracking module performs de-fragmentation on incoming > > fragmented traffic. Take info account if traffic has been de-fragmented > > in execute_check_pkt_len action otherwise we will perform the wrong > > nested action considering the original packet size. This issue typically > > occurs if ovs-vswitchd adds a rule in the pipeline that requires connection > > tracking (e.g. OVN stateful ACLs) before execute_check_pkt_len action. > > > > Fixes: 4d5ec89fc8d14 ("net: openvswitch: Add a new action check_pkt_len") > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > --- > > net/openvswitch/actions.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c > > index fc0efd8833c8..9f4dd64e53bb 100644 > > --- a/net/openvswitch/actions.c > > +++ b/net/openvswitch/actions.c > > @@ -1169,9 +1169,10 @@ static int execute_check_pkt_len(struct datapath *dp, struct sk_buff *skb, > > struct sw_flow_key *key, > > const struct nlattr *attr, bool last) > > { > > + struct ovs_skb_cb *ovs_cb = OVS_CB(skb); > > const struct nlattr *actions, *cpl_arg; > > const struct check_pkt_len_arg *arg; > > - int rem = nla_len(attr); > > + int len, rem = nla_len(attr); > > bool clone_flow_key; > > > > /* The first netlink attribute in 'attr' is always > > @@ -1180,7 +1181,8 @@ static int execute_check_pkt_len(struct datapath *dp, struct sk_buff *skb, > > cpl_arg = nla_data(attr); > > arg = nla_data(cpl_arg); > > > > - if (skb->len <= arg->pkt_len) { > > + len = ovs_cb->mru ? ovs_cb->mru : skb->len; > > + if (len <= arg->pkt_len) { > > We could also check for the segmented packet and use segment length > for this check. Hi Pravin, thx for review. By 'segmented packet' and 'segment length', do you mean 'fragment' and 'fragment length'? If so I guess we can't retrieve the original fragment length if we exec OVS_ACTION_ATTR_CT before OVS_ACTION_ATTR_CHECK_PKT_LEN (e.g if we have a stateful ACL in the ingress pipeline) since handle_fragments() will reconstruct the whole IP datagram and it will store frag_max_size in OVS_CB(skb)->mru. Am I missing something? Regards, Lorenzo
On Mon, Jun 22, 2020 at 5:02 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > > On Fri, Jun 19, 2020 at 4:48 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > > > > > ovs connection tracking module performs de-fragmentation on incoming > > > fragmented traffic. Take info account if traffic has been de-fragmented > > > in execute_check_pkt_len action otherwise we will perform the wrong > > > nested action considering the original packet size. This issue typically > > > occurs if ovs-vswitchd adds a rule in the pipeline that requires connection > > > tracking (e.g. OVN stateful ACLs) before execute_check_pkt_len action. > > > > > > Fixes: 4d5ec89fc8d14 ("net: openvswitch: Add a new action check_pkt_len") > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > > --- > > > net/openvswitch/actions.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c > > > index fc0efd8833c8..9f4dd64e53bb 100644 > > > --- a/net/openvswitch/actions.c > > > +++ b/net/openvswitch/actions.c > > > @@ -1169,9 +1169,10 @@ static int execute_check_pkt_len(struct datapath *dp, struct sk_buff *skb, > > > struct sw_flow_key *key, > > > const struct nlattr *attr, bool last) > > > { > > > + struct ovs_skb_cb *ovs_cb = OVS_CB(skb); > > > const struct nlattr *actions, *cpl_arg; > > > const struct check_pkt_len_arg *arg; > > > - int rem = nla_len(attr); > > > + int len, rem = nla_len(attr); > > > bool clone_flow_key; > > > > > > /* The first netlink attribute in 'attr' is always > > > @@ -1180,7 +1181,8 @@ static int execute_check_pkt_len(struct datapath *dp, struct sk_buff *skb, > > > cpl_arg = nla_data(attr); > > > arg = nla_data(cpl_arg); > > > > > > - if (skb->len <= arg->pkt_len) { > > > + len = ovs_cb->mru ? ovs_cb->mru : skb->len; > > > + if (len <= arg->pkt_len) { > > > > We could also check for the segmented packet and use segment length > > for this check. > > Hi Pravin, > > thx for review. > By 'segmented packet' and 'segment length', do you mean 'fragment' and > 'fragment length'? I am actually talking about GSO packets. Thanks. > If so I guess we can't retrieve the original fragment length if we exec > OVS_ACTION_ATTR_CT before OVS_ACTION_ATTR_CHECK_PKT_LEN (e.g if we have a > stateful ACL in the ingress pipeline) since handle_fragments() will reconstruct > the whole IP datagram and it will store frag_max_size in OVS_CB(skb)->mru. > Am I missing something? > > Regards, > Lorenzo >
> On Mon, Jun 22, 2020 at 5:02 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > > > > On Fri, Jun 19, 2020 at 4:48 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > > > > > > > ovs connection tracking module performs de-fragmentation on incoming > > > > fragmented traffic. Take info account if traffic has been de-fragmented > > > > in execute_check_pkt_len action otherwise we will perform the wrong > > > > nested action considering the original packet size. This issue typically > > > > occurs if ovs-vswitchd adds a rule in the pipeline that requires connection > > > > tracking (e.g. OVN stateful ACLs) before execute_check_pkt_len action. > > > > > > > > Fixes: 4d5ec89fc8d14 ("net: openvswitch: Add a new action check_pkt_len") > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > > > --- > > > > net/openvswitch/actions.c | 6 ++++-- > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c > > > > index fc0efd8833c8..9f4dd64e53bb 100644 > > > > --- a/net/openvswitch/actions.c > > > > +++ b/net/openvswitch/actions.c > > > > @@ -1169,9 +1169,10 @@ static int execute_check_pkt_len(struct datapath *dp, struct sk_buff *skb, > > > > struct sw_flow_key *key, > > > > const struct nlattr *attr, bool last) > > > > { > > > > + struct ovs_skb_cb *ovs_cb = OVS_CB(skb); > > > > const struct nlattr *actions, *cpl_arg; > > > > const struct check_pkt_len_arg *arg; > > > > - int rem = nla_len(attr); > > > > + int len, rem = nla_len(attr); > > > > bool clone_flow_key; > > > > > > > > /* The first netlink attribute in 'attr' is always > > > > @@ -1180,7 +1181,8 @@ static int execute_check_pkt_len(struct datapath *dp, struct sk_buff *skb, > > > > cpl_arg = nla_data(attr); > > > > arg = nla_data(cpl_arg); > > > > > > > > - if (skb->len <= arg->pkt_len) { > > > > + len = ovs_cb->mru ? ovs_cb->mru : skb->len; > > > > + if (len <= arg->pkt_len) { > > > > > > We could also check for the segmented packet and use segment length > > > for this check. > > > > Hi Pravin, > > > > thx for review. > > By 'segmented packet' and 'segment length', do you mean 'fragment' and > > 'fragment length'? > > I am actually talking about GSO packets. ack. IIUC you mean to add a check for gso packets as well taking into account gso_size. I will fix in v2. Regards, Lorenzo > > Thanks. > > > If so I guess we can't retrieve the original fragment length if we exec > > OVS_ACTION_ATTR_CT before OVS_ACTION_ATTR_CHECK_PKT_LEN (e.g if we have a > > stateful ACL in the ingress pipeline) since handle_fragments() will reconstruct > > the whole IP datagram and it will store frag_max_size in OVS_CB(skb)->mru. > > Am I missing something? > > > > Regards, > > Lorenzo > > >
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c index fc0efd8833c8..9f4dd64e53bb 100644 --- a/net/openvswitch/actions.c +++ b/net/openvswitch/actions.c @@ -1169,9 +1169,10 @@ static int execute_check_pkt_len(struct datapath *dp, struct sk_buff *skb, struct sw_flow_key *key, const struct nlattr *attr, bool last) { + struct ovs_skb_cb *ovs_cb = OVS_CB(skb); const struct nlattr *actions, *cpl_arg; const struct check_pkt_len_arg *arg; - int rem = nla_len(attr); + int len, rem = nla_len(attr); bool clone_flow_key; /* The first netlink attribute in 'attr' is always @@ -1180,7 +1181,8 @@ static int execute_check_pkt_len(struct datapath *dp, struct sk_buff *skb, cpl_arg = nla_data(attr); arg = nla_data(cpl_arg); - if (skb->len <= arg->pkt_len) { + len = ovs_cb->mru ? ovs_cb->mru : skb->len; + if (len <= arg->pkt_len) { /* Second netlink attribute in 'attr' is always * 'OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL'. */
ovs connection tracking module performs de-fragmentation on incoming fragmented traffic. Take info account if traffic has been de-fragmented in execute_check_pkt_len action otherwise we will perform the wrong nested action considering the original packet size. This issue typically occurs if ovs-vswitchd adds a rule in the pipeline that requires connection tracking (e.g. OVN stateful ACLs) before execute_check_pkt_len action. Fixes: 4d5ec89fc8d14 ("net: openvswitch: Add a new action check_pkt_len") Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> --- net/openvswitch/actions.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)