[ovs-dev,ovs-dev,v6,3/3] ipf: ipf_preprocess_conntrack should also consider ipf_ctx

considering a multi-thread PMD setting, when the frags are reassembled
in one PMD, another thread might call *ipf_execute_reass_pkts* and 'steal'
the reassembled packets into its ipf ctx, then this reassembled
packet will enter into another ipf context and causes errors.

This happends when there are multiple CT zones, and frags are
reassembled in ct(zone=X) might be 'stealed' into the ct(zone=Y).

Signed-off-by: Peng He <hepeng.0320@bytedance.com>
---
 lib/conntrack.c |  2 +-
 lib/ipf.c       | 10 ++++++++--
 lib/ipf.h       |  1 +
 3 files changed, 10 insertions(+), 3 deletions(-)

Bleep bloop.  Greetings Peng He, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
ERROR: Author Peng He <xnhp0320@gmail.com> needs to sign off.
WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Peng He <hepeng.0320@bytedance.com>
Lines checked: 90, Warnings: 1, Errors: 1


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot

On Thu, 2021-12-30 at 08:03 +0000, Peng He wrote:
> considering a multi-thread PMD setting, when the frags are
> reassembled
> in one PMD, another thread might call *ipf_execute_reass_pkts* and
> 'steal'
> the reassembled packets into its ipf ctx, then this reassembled
> packet will enter into another ipf context and causes errors.
> 
> This happends when there are multiple CT zones, and frags are
> reassembled in ct(zone=X) might be 'stealed' into the ct(zone=Y).
> 
> Signed-off-by: Peng He <hepeng.0320@bytedance.com>
> ---
>  lib/conntrack.c |  2 +-
>  lib/ipf.c       | 10 ++++++++--
>  lib/ipf.h       |  1 +
>  3 files changed, 10 insertions(+), 3 deletions(-)
> 

Hello Peng,

This patch appears to make a number of tests fail. For example:

make check-system-userspace TESTSUITEFLAGS="-v 65 66 74 75 76 77 78 79
122"


-M

> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 72999f1ae..aafa6b536 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -1443,7 +1443,7 @@ conntrack_execute(struct conntrack *ct, struct
> dp_packet_batch *pkt_batch,
>                    const struct nat_action_info_t *nat_action_info,
>                    long long now, uint32_t tp_id, struct ipf_ctx
> *ipf_ctx)
>  {
> -    ipf_preprocess_conntrack(ct->ipf, pkt_batch, now, dl_type, zone,
> +    ipf_preprocess_conntrack(ct->ipf, pkt_batch, ipf_ctx, now,
> dl_type, zone,
>                               ct->hash_basis);
>  
>      struct dp_packet *packet;
> diff --git a/lib/ipf.c b/lib/ipf.c
> index ef302e59c..bca34f59c 100644
> --- a/lib/ipf.c
> +++ b/lib/ipf.c
> @@ -1139,7 +1139,8 @@ ipf_send_expired_frags(struct ipf *ipf, struct
> dp_packet_batch *pb,
>  /* Adds a reassmebled packet to a packet batch to be processed by
> the caller.
>   */
>  static void
> -ipf_execute_reass_pkts(struct ipf *ipf, struct dp_packet_batch *pb)
> +ipf_execute_reass_pkts(struct ipf *ipf, struct dp_packet_batch *pb,
> +                       struct ipf_ctx *ctx)
>  {
>      if (ovs_list_is_empty(&ipf->reassembled_pkt_list)) {
>          return;
> @@ -1149,6 +1150,10 @@ ipf_execute_reass_pkts(struct ipf *ipf, struct
> dp_packet_batch *pb)
>      struct reassembled_pkt *rp, *next;
>  
>      LIST_FOR_EACH_SAFE (rp, next, rp_list_node, &ipf-
> >reassembled_pkt_list) {
> +        if (ctx && !ipf_ctx_eq(rp->list, ctx, rp->pkt)) {
> +            continue;
> +        }
> +
>          if (!rp->list->reass_execute_ctx &&
>              ipf_dp_packet_batch_add(pb, rp->pkt, false)) {
>              rp->list->reass_execute_ctx = rp->pkt;
> @@ -1250,6 +1255,7 @@ ipf_post_execute_reass_pkts(struct ipf *ipf,
>   * be added to the batch to be sent through conntrack. */
>  void
>  ipf_preprocess_conntrack(struct ipf *ipf, struct dp_packet_batch
> *pb,
> +                         struct ipf_ctx *ipf_ctx,
>                           long long now, ovs_be16 dl_type,
>                           uint16_t zone, uint32_t hash_basis)
>  {
> @@ -1258,7 +1264,7 @@ ipf_preprocess_conntrack(struct ipf *ipf,
> struct dp_packet_batch *pb,
>      }
>  
>      if (ipf_get_enabled(ipf) || atomic_count_get(&ipf->nfrag)) {
> -        ipf_execute_reass_pkts(ipf, pb);
> +        ipf_execute_reass_pkts(ipf, pb, ipf_ctx);
>      }
>  }
>  
> diff --git a/lib/ipf.h b/lib/ipf.h
> index 7bbf453af..8974f15ae 100644
> --- a/lib/ipf.h
> +++ b/lib/ipf.h
> @@ -49,6 +49,7 @@ struct ipf_ctx {
>  struct ipf *ipf_init(void);
>  void ipf_destroy(struct ipf *ipf);
>  void ipf_preprocess_conntrack(struct ipf *ipf, struct
> dp_packet_batch *pb,
> +                              struct ipf_ctx *ctx,
>                                long long now, ovs_be16 dl_type,
> uint16_t zone,
>                                uint32_t hash_basis);
>

Hi, Mike,

The bug in the last email is not correct.

in packet_execute_prepare functions,
it has changed dp_packet md's port into the odp port,

but in the execute->flow, the value is still the ofp port.

 static void
 packet_execute_prepare(struct ofproto *ofproto_,
                        struct ofproto_packet_out *opo)
     OVS_REQUIRES(ofproto_mutex)
{
....
     pkt_metadata_from_flow(&opo->packet->md, opo->flow);
     execute->packet = opo->packet;
     execute->flow = opo->flow;

...
     /* Fix up in_port. */
     ofproto_dpif_set_packet_odp_port(ofproto, opo->flow->in_port.ofp_port,
                                      opo->packet);

....
}

but ipf_ctx is formed using the flow's value instead of packet's value,
this causes the test suite to fail.

the reason of ipf_ctx using the flow's value is at the second time of
calling conntrack_execute, there are no packets in the batch, the batch is
empty and only used as a container for the ipf to inject packets.

I guess maybe we should also change the execute->flow's in_port into the
odp value?

any ideas?

Mike Pattrick <mkp@redhat.com> 于2022年1月8日周六 06:03写道：

> On Thu, 2021-12-30 at 08:03 +0000, Peng He wrote:
> > considering a multi-thread PMD setting, when the frags are
> > reassembled
> > in one PMD, another thread might call *ipf_execute_reass_pkts* and
> > 'steal'
> > the reassembled packets into its ipf ctx, then this reassembled
> > packet will enter into another ipf context and causes errors.
> >
> > This happends when there are multiple CT zones, and frags are
> > reassembled in ct(zone=X) might be 'stealed' into the ct(zone=Y).
> >
> > Signed-off-by: Peng He <hepeng.0320@bytedance.com>
> > ---
> >  lib/conntrack.c |  2 +-
> >  lib/ipf.c       | 10 ++++++++--
> >  lib/ipf.h       |  1 +
> >  3 files changed, 10 insertions(+), 3 deletions(-)
> >
>
> Hello Peng,
>
> This patch appears to make a number of tests fail. For example:
>
> make check-system-userspace TESTSUITEFLAGS="-v 65 66 74 75 76 77 78 79
> 122"
>
>
> -M
>
> > diff --git a/lib/conntrack.c b/lib/conntrack.c
> > index 72999f1ae..aafa6b536 100644
> > --- a/lib/conntrack.c
> > +++ b/lib/conntrack.c
> > @@ -1443,7 +1443,7 @@ conntrack_execute(struct conntrack *ct, struct
> > dp_packet_batch *pkt_batch,
> >                    const struct nat_action_info_t *nat_action_info,
> >                    long long now, uint32_t tp_id, struct ipf_ctx
> > *ipf_ctx)
> >  {
> > -    ipf_preprocess_conntrack(ct->ipf, pkt_batch, now, dl_type, zone,
> > +    ipf_preprocess_conntrack(ct->ipf, pkt_batch, ipf_ctx, now,
> > dl_type, zone,
> >                               ct->hash_basis);
> >
> >      struct dp_packet *packet;
> > diff --git a/lib/ipf.c b/lib/ipf.c
> > index ef302e59c..bca34f59c 100644
> > --- a/lib/ipf.c
> > +++ b/lib/ipf.c
> > @@ -1139,7 +1139,8 @@ ipf_send_expired_frags(struct ipf *ipf, struct
> > dp_packet_batch *pb,
> >  /* Adds a reassmebled packet to a packet batch to be processed by
> > the caller.
> >   */
> >  static void
> > -ipf_execute_reass_pkts(struct ipf *ipf, struct dp_packet_batch *pb)
> > +ipf_execute_reass_pkts(struct ipf *ipf, struct dp_packet_batch *pb,
> > +                       struct ipf_ctx *ctx)
> >  {
> >      if (ovs_list_is_empty(&ipf->reassembled_pkt_list)) {
> >          return;
> > @@ -1149,6 +1150,10 @@ ipf_execute_reass_pkts(struct ipf *ipf, struct
> > dp_packet_batch *pb)
> >      struct reassembled_pkt *rp, *next;
> >
> >      LIST_FOR_EACH_SAFE (rp, next, rp_list_node, &ipf-
> > >reassembled_pkt_list) {
> > +        if (ctx && !ipf_ctx_eq(rp->list, ctx, rp->pkt)) {
> > +            continue;
> > +        }
> > +
> >          if (!rp->list->reass_execute_ctx &&
> >              ipf_dp_packet_batch_add(pb, rp->pkt, false)) {
> >              rp->list->reass_execute_ctx = rp->pkt;
> > @@ -1250,6 +1255,7 @@ ipf_post_execute_reass_pkts(struct ipf *ipf,
> >   * be added to the batch to be sent through conntrack. */
> >  void
> >  ipf_preprocess_conntrack(struct ipf *ipf, struct dp_packet_batch
> > *pb,
> > +                         struct ipf_ctx *ipf_ctx,
> >                           long long now, ovs_be16 dl_type,
> >                           uint16_t zone, uint32_t hash_basis)
> >  {
> > @@ -1258,7 +1264,7 @@ ipf_preprocess_conntrack(struct ipf *ipf,
> > struct dp_packet_batch *pb,
> >      }
> >
> >      if (ipf_get_enabled(ipf) || atomic_count_get(&ipf->nfrag)) {
> > -        ipf_execute_reass_pkts(ipf, pb);
> > +        ipf_execute_reass_pkts(ipf, pb, ipf_ctx);
> >      }
> >  }
> >
> > diff --git a/lib/ipf.h b/lib/ipf.h
> > index 7bbf453af..8974f15ae 100644
> > --- a/lib/ipf.h
> > +++ b/lib/ipf.h
> > @@ -49,6 +49,7 @@ struct ipf_ctx {
> >  struct ipf *ipf_init(void);
> >  void ipf_destroy(struct ipf *ipf);
> >  void ipf_preprocess_conntrack(struct ipf *ipf, struct
> > dp_packet_batch *pb,
> > +                              struct ipf_ctx *ctx,
> >                                long long now, ovs_be16 dl_type,
> > uint16_t zone,
> >                                uint32_t hash_basis);
> >
>
>

just forget to reply all.

---------- Forwarded message ---------
发件人： 贺鹏 <xnhp0320@gmail.com>
Date: 2022年1月9日周日 10:59
Subject: Re: [ovs-dev v6 3/3] ipf: ipf_preprocess_conntrack should also
consider ipf_ctx
To: Mike Pattrick <mkp@redhat.com>


Hi, Mike,

thank you for the review and testing.

it seems that all these broken tests are related to a bug in packet-out
action execution in OVS.
it seems that the packet-out uses the OFP to fill the dp_packet metadata,
and this in_port value has
not been changed during the execution in the datapath, where the value
should be translated into the ODP.

in the test the ofp values equals to 1, while the corresponding odp port
equals to 2, which makes ipf_ctx_eq test failed.

I am trying to think about a solution.
any idaes?



Mike Pattrick <mkp@redhat.com> 于2022年1月8日周六 06:03写道：

> On Thu, 2021-12-30 at 08:03 +0000, Peng He wrote:
> > considering a multi-thread PMD setting, when the frags are
> > reassembled
> > in one PMD, another thread might call *ipf_execute_reass_pkts* and
> > 'steal'
> > the reassembled packets into its ipf ctx, then this reassembled
> > packet will enter into another ipf context and causes errors.
> >
> > This happends when there are multiple CT zones, and frags are
> > reassembled in ct(zone=X) might be 'stealed' into the ct(zone=Y).
> >
> > Signed-off-by: Peng He <hepeng.0320@bytedance.com>
> > ---
> >  lib/conntrack.c |  2 +-
> >  lib/ipf.c       | 10 ++++++++--
> >  lib/ipf.h       |  1 +
> >  3 files changed, 10 insertions(+), 3 deletions(-)
> >
>
> Hello Peng,
>
> This patch appears to make a number of tests fail. For example:
>
> make check-system-userspace TESTSUITEFLAGS="-v 65 66 74 75 76 77 78 79
> 122"
>
>
> -M
>
> > diff --git a/lib/conntrack.c b/lib/conntrack.c
> > index 72999f1ae..aafa6b536 100644
> > --- a/lib/conntrack.c
> > +++ b/lib/conntrack.c
> > @@ -1443,7 +1443,7 @@ conntrack_execute(struct conntrack *ct, struct
> > dp_packet_batch *pkt_batch,
> >                    const struct nat_action_info_t *nat_action_info,
> >                    long long now, uint32_t tp_id, struct ipf_ctx
> > *ipf_ctx)
> >  {
> > -    ipf_preprocess_conntrack(ct->ipf, pkt_batch, now, dl_type, zone,
> > +    ipf_preprocess_conntrack(ct->ipf, pkt_batch, ipf_ctx, now,
> > dl_type, zone,
> >                               ct->hash_basis);
> >
> >      struct dp_packet *packet;
> > diff --git a/lib/ipf.c b/lib/ipf.c
> > index ef302e59c..bca34f59c 100644
> > --- a/lib/ipf.c
> > +++ b/lib/ipf.c
> > @@ -1139,7 +1139,8 @@ ipf_send_expired_frags(struct ipf *ipf, struct
> > dp_packet_batch *pb,
> >  /* Adds a reassmebled packet to a packet batch to be processed by
> > the caller.
> >   */
> >  static void
> > -ipf_execute_reass_pkts(struct ipf *ipf, struct dp_packet_batch *pb)
> > +ipf_execute_reass_pkts(struct ipf *ipf, struct dp_packet_batch *pb,
> > +                       struct ipf_ctx *ctx)
> >  {
> >      if (ovs_list_is_empty(&ipf->reassembled_pkt_list)) {
> >          return;
> > @@ -1149,6 +1150,10 @@ ipf_execute_reass_pkts(struct ipf *ipf, struct
> > dp_packet_batch *pb)
> >      struct reassembled_pkt *rp, *next;
> >
> >      LIST_FOR_EACH_SAFE (rp, next, rp_list_node, &ipf-
> > >reassembled_pkt_list) {
> > +        if (ctx && !ipf_ctx_eq(rp->list, ctx, rp->pkt)) {
> > +            continue;
> > +        }
> > +
> >          if (!rp->list->reass_execute_ctx &&
> >              ipf_dp_packet_batch_add(pb, rp->pkt, false)) {
> >              rp->list->reass_execute_ctx = rp->pkt;
> > @@ -1250,6 +1255,7 @@ ipf_post_execute_reass_pkts(struct ipf *ipf,
> >   * be added to the batch to be sent through conntrack. */
> >  void
> >  ipf_preprocess_conntrack(struct ipf *ipf, struct dp_packet_batch
> > *pb,
> > +                         struct ipf_ctx *ipf_ctx,
> >                           long long now, ovs_be16 dl_type,
> >                           uint16_t zone, uint32_t hash_basis)
> >  {
> > @@ -1258,7 +1264,7 @@ ipf_preprocess_conntrack(struct ipf *ipf,
> > struct dp_packet_batch *pb,
> >      }
> >
> >      if (ipf_get_enabled(ipf) || atomic_count_get(&ipf->nfrag)) {
> > -        ipf_execute_reass_pkts(ipf, pb);
> > +        ipf_execute_reass_pkts(ipf, pb, ipf_ctx);
> >      }
> >  }
> >
> > diff --git a/lib/ipf.h b/lib/ipf.h
> > index 7bbf453af..8974f15ae 100644
> > --- a/lib/ipf.h
> > +++ b/lib/ipf.h
> > @@ -49,6 +49,7 @@ struct ipf_ctx {
> >  struct ipf *ipf_init(void);
> >  void ipf_destroy(struct ipf *ipf);
> >  void ipf_preprocess_conntrack(struct ipf *ipf, struct
> > dp_packet_batch *pb,
> > +                              struct ipf_ctx *ctx,
> >                                long long now, ovs_be16 dl_type,
> > uint16_t zone,
> >                                uint32_t hash_basis);
> >
>
>

Hi,

after I applied this patch, all the failed tests now passed.


diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index cba49a99e..a7df7db6f 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4972,6 +4972,7 @@ packet_execute_prepare(struct ofproto *ofproto_,
     /* Fix up in_port. */
     ofproto_dpif_set_packet_odp_port(ofproto, opo->flow->in_port.ofp_port,
                                      opo->packet);
+    opo->flow->in_port = opo->packet->md.in_port;

     ofproto_dpif_packet_out_delete(aux);
     opo->aux = execute;

and I found a bug in the current patch, I've changed back the pkt pointer
that is used in ipf_ctx_eq

-        if (ctx && !ipf_ctx_eq(ipf_list, ctx)) {
+        if (ctx && !ipf_ctx_eq(ipf_list, ctx, \
+                    ipf_list->frag_list[ipf_list->last_sent_idx + 1].pkt))
{
             continue;
         }

in the v6 patch, it uses ipf_list->reass_execute_ctx, however, this pointer
is freed. will post a v7 patch and fixes this.

about the packet_execute_prepare's bug, I am not sure the flow->execute
should be changed into odp port.

Mike Pattrick <mkp@redhat.com> 于2022年1月8日周六 06:03写道：

> On Thu, 2021-12-30 at 08:03 +0000, Peng He wrote:
> > considering a multi-thread PMD setting, when the frags are
> > reassembled
> > in one PMD, another thread might call *ipf_execute_reass_pkts* and
> > 'steal'
> > the reassembled packets into its ipf ctx, then this reassembled
> > packet will enter into another ipf context and causes errors.
> >
> > This happends when there are multiple CT zones, and frags are
> > reassembled in ct(zone=X) might be 'stealed' into the ct(zone=Y).
> >
> > Signed-off-by: Peng He <hepeng.0320@bytedance.com>
> > ---
> >  lib/conntrack.c |  2 +-
> >  lib/ipf.c       | 10 ++++++++--
> >  lib/ipf.h       |  1 +
> >  3 files changed, 10 insertions(+), 3 deletions(-)
> >
>
> Hello Peng,
>
> This patch appears to make a number of tests fail. For example:
>
> make check-system-userspace TESTSUITEFLAGS="-v 65 66 74 75 76 77 78 79
> 122"
>
>
> -M
>
> > diff --git a/lib/conntrack.c b/lib/conntrack.c
> > index 72999f1ae..aafa6b536 100644
> > --- a/lib/conntrack.c
> > +++ b/lib/conntrack.c
> > @@ -1443,7 +1443,7 @@ conntrack_execute(struct conntrack *ct, struct
> > dp_packet_batch *pkt_batch,
> >                    const struct nat_action_info_t *nat_action_info,
> >                    long long now, uint32_t tp_id, struct ipf_ctx
> > *ipf_ctx)
> >  {
> > -    ipf_preprocess_conntrack(ct->ipf, pkt_batch, now, dl_type, zone,
> > +    ipf_preprocess_conntrack(ct->ipf, pkt_batch, ipf_ctx, now,
> > dl_type, zone,
> >                               ct->hash_basis);
> >
> >      struct dp_packet *packet;
> > diff --git a/lib/ipf.c b/lib/ipf.c
> > index ef302e59c..bca34f59c 100644
> > --- a/lib/ipf.c
> > +++ b/lib/ipf.c
> > @@ -1139,7 +1139,8 @@ ipf_send_expired_frags(struct ipf *ipf, struct
> > dp_packet_batch *pb,
> >  /* Adds a reassmebled packet to a packet batch to be processed by
> > the caller.
> >   */
> >  static void
> > -ipf_execute_reass_pkts(struct ipf *ipf, struct dp_packet_batch *pb)
> > +ipf_execute_reass_pkts(struct ipf *ipf, struct dp_packet_batch *pb,
> > +                       struct ipf_ctx *ctx)
> >  {
> >      if (ovs_list_is_empty(&ipf->reassembled_pkt_list)) {
> >          return;
> > @@ -1149,6 +1150,10 @@ ipf_execute_reass_pkts(struct ipf *ipf, struct
> > dp_packet_batch *pb)
> >      struct reassembled_pkt *rp, *next;
> >
> >      LIST_FOR_EACH_SAFE (rp, next, rp_list_node, &ipf-
> > >reassembled_pkt_list) {
> > +        if (ctx && !ipf_ctx_eq(rp->list, ctx, rp->pkt)) {
> > +            continue;
> > +        }
> > +
> >          if (!rp->list->reass_execute_ctx &&
> >              ipf_dp_packet_batch_add(pb, rp->pkt, false)) {
> >              rp->list->reass_execute_ctx = rp->pkt;
> > @@ -1250,6 +1255,7 @@ ipf_post_execute_reass_pkts(struct ipf *ipf,
> >   * be added to the batch to be sent through conntrack. */
> >  void
> >  ipf_preprocess_conntrack(struct ipf *ipf, struct dp_packet_batch
> > *pb,
> > +                         struct ipf_ctx *ipf_ctx,
> >                           long long now, ovs_be16 dl_type,
> >                           uint16_t zone, uint32_t hash_basis)
> >  {
> > @@ -1258,7 +1264,7 @@ ipf_preprocess_conntrack(struct ipf *ipf,
> > struct dp_packet_batch *pb,
> >      }
> >
> >      if (ipf_get_enabled(ipf) || atomic_count_get(&ipf->nfrag)) {
> > -        ipf_execute_reass_pkts(ipf, pb);
> > +        ipf_execute_reass_pkts(ipf, pb, ipf_ctx);
> >      }
> >  }
> >
> > diff --git a/lib/ipf.h b/lib/ipf.h
> > index 7bbf453af..8974f15ae 100644
> > --- a/lib/ipf.h
> > +++ b/lib/ipf.h
> > @@ -49,6 +49,7 @@ struct ipf_ctx {
> >  struct ipf *ipf_init(void);
> >  void ipf_destroy(struct ipf *ipf);
> >  void ipf_preprocess_conntrack(struct ipf *ipf, struct
> > dp_packet_batch *pb,
> > +                              struct ipf_ctx *ctx,
> >                                long long now, ovs_be16 dl_type,
> > uint16_t zone,
> >                                uint32_t hash_basis);
> >
>
>