netdev - Re: [PATCH net-next] net: sched: set dedicated tcf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAM_iQpWmRQP8-xBiULnCDHkDTHW7WaKuBmcsYJ7j-yRxfgwOaA@mail.gmail.com>
Date:   Mon, 25 Feb 2019 14:52:50 -0800
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Vlad Buslov <vladbu@...lanox.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>,
        David Miller <davem@...emloft.net>
Subject: Re: [PATCH net-next] net: sched: set dedicated tcf_walker flag when
 tp is empty

On Mon, Feb 25, 2019 at 7:38 AM Vlad Buslov <vladbu@...lanox.com> wrote:
>
> Using tcf_walker->stop flag to determine when tcf_walker->fn() was called
> at least once is unreliable. Some classifiers set 'stop' flag on error
> before calling walker callback, other classifiers used to call it with NULL
> filter pointer when empty. In order to prevent further regressions, extend
> tcf_walker structure with dedicated 'nonempty' flag. Set this flag in
> tcf_walker->fn() implementation that is used to check if classifier has
> filters configured.


So, after this patch commits like 31a998487641 ("net: sched: fw: don't
set arg->stop in fw_walk() when empty") can be reverted??


>
> Fixes: 8b64678e0af8 ("net: sched: refactor tp insert/delete for concurrent execution")
> Signed-off-by: Vlad Buslov <vladbu@...lanox.com>
> Suggested-by: Cong Wang <xiyou.wangcong@...il.com>
> ---
>  include/net/pkt_cls.h |  1 +
>  net/sched/cls_api.c   | 13 +++++++++----
>  2 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index 232f801f2a21..422dd8800478 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -17,6 +17,7 @@ struct tcf_walker {
>         int     stop;
>         int     skip;
>         int     count;
> +       bool    nonempty;
>         unsigned long cookie;
>         int     (*fn)(struct tcf_proto *, void *node, struct tcf_walker *);
>  };
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index e2c888961379..3543be31d400 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -238,18 +238,23 @@ static void tcf_proto_put(struct tcf_proto *tp, bool rtnl_held,
>                 tcf_proto_destroy(tp, rtnl_held, extack);
>  }
>
> -static int walker_noop(struct tcf_proto *tp, void *d, struct tcf_walker *arg)
> +static int walker_check_empty(struct tcf_proto *tp, void *d,
> +                             struct tcf_walker *arg)
>  {
> -       return -1;
> +       if (tp) {
> +               arg->nonempty = true;
> +               return -1;
> +       }
> +       return 0;

How does this even work? If we can simply check tp!=NULL as
non-empty, why do we even need a walker??

For me, it must be pushed down to each implementation to
determine how it is empty.