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  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 19 May 2020 14:23:54 +0530
From:   Sriharsha Basavapatna <sriharsha.basavapatna@...adcom.com>
To:     Pablo Neira Ayuso <pablo@...filter.org>
Cc:     netfilter-devel@...r.kernel.org,
        David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
        paulb@...lanox.com, Oz Shlomo <ozsh@...lanox.com>,
        vladbu@...lanox.com, jiri@...nulli.us, kuba@...nel.org,
        saeedm@...lanox.com, Michael Chan <michael.chan@...adcom.com>,
        Sriharsha Basavapatna <sriharsha.basavapatna@...adcom.com>
Subject: Re: [PATCH 7/8 net] bnxt_tc: update indirect block support

On Wed, May 13, 2020 at 10:12 PM Pablo Neira Ayuso <pablo@...filter.org> wrote:
>
> Register ndo callback via flow_indr_dev_register() and
> flow_indr_dev_unregister().
>
> Signed-off-by: Pablo Neira Ayuso <pablo@...filter.org>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.h    |  1 -
>  drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c | 51 +++++---------------
>  2 files changed, 12 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> index f6a3250ef1c5..e7d1c12673bd 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> @@ -1857,7 +1857,6 @@ struct bnxt {
>         u8                      dsn[8];
>         struct bnxt_tc_info     *tc_info;
>         struct list_head        tc_indr_block_list;
> -       struct notifier_block   tc_netdev_nb;
>         struct dentry           *debugfs_pdev;
>         struct device           *hwmon_dev;
>  };
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
> index 782ea0771221..0eef4f5e4a46 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
> @@ -1939,53 +1939,25 @@ static int bnxt_tc_setup_indr_block(struct net_device *netdev, struct bnxt *bp,
>         return 0;
>  }
>
> -static int bnxt_tc_setup_indr_cb(struct net_device *netdev, void *cb_priv,
> -                                enum tc_setup_type type, void *type_data)
> -{
> -       switch (type) {
> -       case TC_SETUP_BLOCK:
> -               return bnxt_tc_setup_indr_block(netdev, cb_priv, type_data);
> -       default:
> -               return -EOPNOTSUPP;
> -       }
> -}
> -
>  static bool bnxt_is_netdev_indr_offload(struct net_device *netdev)
>  {
>         return netif_is_vxlan(netdev);
>  }
>
> -static int bnxt_tc_indr_block_event(struct notifier_block *nb,
> -                                   unsigned long event, void *ptr)
> +static int bnxt_tc_setup_indr_cb(struct net_device *netdev, void *cb_priv,
> +                                enum tc_setup_type type, void *type_data)
>  {
> -       struct net_device *netdev;
> -       struct bnxt *bp;
> -       int rc;
> -
> -       netdev = netdev_notifier_info_to_dev(ptr);
>         if (!bnxt_is_netdev_indr_offload(netdev))
> -               return NOTIFY_OK;
> -
> -       bp = container_of(nb, struct bnxt, tc_netdev_nb);
> +               return -EOPNOTSUPP;
>
> -       switch (event) {
> -       case NETDEV_REGISTER:
> -               rc = __flow_indr_block_cb_register(netdev, bp,
> -                                                  bnxt_tc_setup_indr_cb,
> -                                                  bp);
> -               if (rc)
> -                       netdev_info(bp->dev,
> -                                   "Failed to register indirect blk: dev: %s\n",
> -                                   netdev->name);
> -               break;
> -       case NETDEV_UNREGISTER:
> -               __flow_indr_block_cb_unregister(netdev,
> -                                               bnxt_tc_setup_indr_cb,
> -                                               bp);
> +       switch (type) {
> +       case TC_SETUP_BLOCK:
> +               return bnxt_tc_setup_indr_block(netdev, cb_priv, type_data);
> +       default:
>                 break;
>         }
>
> -       return NOTIFY_DONE;
> +       return -EOPNOTSUPP;
>  }
>
>  static const struct rhashtable_params bnxt_tc_flow_ht_params = {
> @@ -2074,8 +2046,8 @@ int bnxt_init_tc(struct bnxt *bp)
>
>         /* init indirect block notifications */
>         INIT_LIST_HEAD(&bp->tc_indr_block_list);
> -       bp->tc_netdev_nb.notifier_call = bnxt_tc_indr_block_event;
> -       rc = register_netdevice_notifier(&bp->tc_netdev_nb);
> +
> +       rc = flow_indr_dev_register(bnxt_tc_setup_indr_cb, bp);
>         if (!rc)
>                 return 0;
>
> @@ -2101,7 +2073,8 @@ void bnxt_shutdown_tc(struct bnxt *bp)
>         if (!bnxt_tc_flower_enabled(bp))
>                 return;
>
> -       unregister_netdevice_notifier(&bp->tc_netdev_nb);
> +       flow_indr_dev_unregister(bnxt_tc_setup_indr_cb, bp,
> +                                bnxt_tc_setup_indr_block_cb);

Why does the driver need to provide the "cb" again during unregister,
since both "cb" and "cb_priv" are already provided during register() ?
This interface could be simplified/improved if
flow_indr_dev_register() returns an opaque handle to the object it
creates (struct flow_indr_dev *) ? The driver should just pass this
handle during unregistration. Also, why do we need the extra (3rd)
argument (flow_setup_cb_t / bnxt_tc_setup_indr_block_cb) during unreg
? It is handled internally by the driver as a part of FLOW_BLOCK_BIND
/ UNBIND ?

Thanks,
-Harsha

>         rhashtable_destroy(&tc_info->flow_table);
>         rhashtable_destroy(&tc_info->l2_table);
>         rhashtable_destroy(&tc_info->decap_l2_table);
> --
> 2.20.1
>

Powered by blists - more mailing lists