* [PATCH net] openvswitch: take into account de-fragmentation in execute_check_pkt_len
@ 2020-06-19 11:48 Lorenzo Bianconi
2020-06-20 7:01 ` Pravin Shelar
0 siblings, 1 reply; 5+ messages in thread
From: Lorenzo Bianconi @ 2020-06-19 11:48 UTC (permalink / raw)
To: netdev; +Cc: davem, nusiddiq, gvrose8192, pshelar, lorenzo.bianconi, dev
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) {
/* Second netlink attribute in 'attr' is always
* 'OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL'.
*/
--
2.26.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net] openvswitch: take into account de-fragmentation in execute_check_pkt_len
2020-06-19 11:48 [PATCH net] openvswitch: take into account de-fragmentation in execute_check_pkt_len Lorenzo Bianconi
@ 2020-06-20 7:01 ` Pravin Shelar
2020-06-22 12:02 ` Lorenzo Bianconi
0 siblings, 1 reply; 5+ messages in thread
From: Pravin Shelar @ 2020-06-20 7:01 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: Linux Kernel Network Developers, David S. Miller, Numan Siddique,
Greg Rose, lorenzo.bianconi, ovs dev
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] openvswitch: take into account de-fragmentation in execute_check_pkt_len
2020-06-20 7:01 ` Pravin Shelar
@ 2020-06-22 12:02 ` Lorenzo Bianconi
2020-06-22 15:59 ` Pravin Shelar
0 siblings, 1 reply; 5+ messages in thread
From: Lorenzo Bianconi @ 2020-06-22 12:02 UTC (permalink / raw)
To: Pravin Shelar
Cc: Linux Kernel Network Developers, David S. Miller, Numan Siddique,
Greg Rose, lorenzo.bianconi, ovs dev
[-- Attachment #1: Type: text/plain, Size: 2478 bytes --]
> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] openvswitch: take into account de-fragmentation in execute_check_pkt_len
2020-06-22 12:02 ` Lorenzo Bianconi
@ 2020-06-22 15:59 ` Pravin Shelar
2020-06-22 20:46 ` Lorenzo Bianconi
0 siblings, 1 reply; 5+ messages in thread
From: Pravin Shelar @ 2020-06-22 15:59 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: Linux Kernel Network Developers, David S. Miller, Numan Siddique,
Greg Rose, lorenzo.bianconi, ovs dev
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] openvswitch: take into account de-fragmentation in execute_check_pkt_len
2020-06-22 15:59 ` Pravin Shelar
@ 2020-06-22 20:46 ` Lorenzo Bianconi
0 siblings, 0 replies; 5+ messages in thread
From: Lorenzo Bianconi @ 2020-06-22 20:46 UTC (permalink / raw)
To: Pravin Shelar
Cc: Lorenzo Bianconi, Linux Kernel Network Developers,
David S. Miller, Numan Siddique, Greg Rose, ovs dev
[-- Attachment #1: Type: text/plain, Size: 2986 bytes --]
> 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
> >
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-06-22 20:47 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-19 11:48 [PATCH net] openvswitch: take into account de-fragmentation in execute_check_pkt_len Lorenzo Bianconi
2020-06-20 7:01 ` Pravin Shelar
2020-06-22 12:02 ` Lorenzo Bianconi
2020-06-22 15:59 ` Pravin Shelar
2020-06-22 20:46 ` Lorenzo Bianconi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.