On Fri 19 Jun 2020 at 12:41, dsatish <satis...@oneconvergence.com> wrote:
> A packet reaches OVS user space, only if, either there is no rule in
> datapath/hardware or there is race condition that the flow is added
> to hardware but before it is processed another packet arrives.
>
> It is possible hardware as part of its limitations/optimizations
> remove certain flows. To handle such cases where the hardware lost
> the flows, tc can offload to hardware if it receives a flow for which
> there exists an entry in its flow table. To handle such cases TC when
> it returns EEXIST error, also programs the flow in hardware, if
> hardware offload is enabled.
>
> Signed-off-by: Chandra Kesava <kesa...@gmail.com>
> Signed-off-by: Prathibha Nagooru <prathibha.nago...@oneconvergence.com>
> Signed-off-by: Satish Dhote <satis...@oneconvergence.com>
> ---
>  net/sched/cls_flower.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index f1a5352cbb04..d718233cd5b9 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -431,7 +431,8 @@ static void fl_hw_destroy_filter(struct tcf_proto *tp, 
> struct cls_fl_filter *f,
>
>  static int fl_hw_replace_filter(struct tcf_proto *tp,
>                               struct cls_fl_filter *f, bool rtnl_held,
> -                             struct netlink_ext_ack *extack)
> +                             struct netlink_ext_ack *extack,
> +                             unsigned long cookie)
>  {
>       struct tcf_block *block = tp->chain->block;
>       struct flow_cls_offload cls_flower = {};
> @@ -444,7 +445,7 @@ static int fl_hw_replace_filter(struct tcf_proto *tp,
>
>       tc_cls_common_offload_init(&cls_flower.common, tp, f->flags, extack);
>       cls_flower.command = FLOW_CLS_REPLACE;
> -     cls_flower.cookie = (unsigned long) f;
> +     cls_flower.cookie = cookie;
>       cls_flower.rule->match.dissector = &f->mask->dissector;
>       cls_flower.rule->match.mask = &f->mask->key;
>       cls_flower.rule->match.key = &f->mkey;
> @@ -2024,11 +2025,25 @@ static int fl_change(struct net *net, struct sk_buff 
> *in_skb,
>       fl_init_unmasked_key_dissector(&fnew->unmasked_key_dissector);
>
>       err = fl_ht_insert_unique(fnew, fold, &in_ht);
> -     if (err)
> +     if (err) {
> +             /* It is possible Hardware lost the flow even though TC has it,
> +              * and flow miss in hardware causes controller to offload flow 
> again.
> +              */
> +             if (err == -EEXIST && !tc_skip_hw(fnew->flags)) {
> +                     struct cls_fl_filter *f =
> +                             __fl_lookup(fnew->mask, &fnew->mkey);

You don't hold neither rcu read lock nor reference to the "f" filter
here, which means it can be concurrently destroyed at any time.

> +
> +                     if (f)
> +                             fl_hw_replace_filter(tp, fnew, rtnl_held,
> +                                                  extack,
> +                                                  (unsigned long)(f));
> +             }

It looks like you are inventing filter replace/overwrite functionality
here. However, such functionality is already supported. fl_change()
receives "fold" via "arg" argument, if filter with specified key exists
in classifier instance and NLM_F_EXCL netlink message flag is not set.

>               goto errout_mask;
> +     }
>
>       if (!tc_skip_hw(fnew->flags)) {
> -             err = fl_hw_replace_filter(tp, fnew, rtnl_held, extack);
> +             err = fl_hw_replace_filter(tp, fnew, rtnl_held, extack,
> +                                        (unsigned long)fnew);
>               if (err)
>                       goto errout_ht;
>       }

Reply via email to