netdev - Re: [PATCH net-next 01/13] net: sched: Pass qdisc reference in struct flow_block

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]

Date:   Fri, 10 Jul 2020 17:15:39 +0200
From:   Petr Machata <petrm@...lanox.com>
To:     Pablo Neira Ayuso <pablo@...filter.org>
Cc:     Ido Schimmel <idosch@...sch.org>, netdev@...r.kernel.org,
        davem@...emloft.net, kuba@...nel.org, jiri@...lanox.com,
        mlxsw@...lanox.com, michael.chan@...adcom.com, saeedm@...lanox.com,
        leon@...nel.org, kadlec@...filter.org, fw@...len.de,
        jhs@...atatu.com, xiyou.wangcong@...il.com,
        simon.horman@...ronome.com, Ido Schimmel <idosch@...lanox.com>
Subject: Re: [PATCH net-next 01/13] net: sched: Pass qdisc reference in struct flow_block_offload


Pablo Neira Ayuso <pablo@...filter.org> writes:

> On Fri, Jul 10, 2020 at 04:56:54PM +0300, Ido Schimmel wrote:
>> From: Petr Machata <petrm@...lanox.com>
>>
>> Previously, shared blocks were only relevant for the pseudo-qdiscs ingress
>> and clsact. Recently, a qevent facility was introduced, which allows to
>> bind blocks to well-defined slots of a qdisc instance. RED in particular
>> got two qevents: early_drop and mark. Drivers that wish to offload these
>> blocks will be sent the usual notification, and need to know which qdisc it
>> is related to.
>>
>> To that end, extend flow_block_offload with a "sch" pointer, and initialize
>> as appropriate. This prompts changes in the indirect block facility, which
>> now tracks the scheduler instead of the netdevice. Update signatures of
>> several functions similarly. Deduce the device from the scheduler when
>> necessary.
>>
>> Signed-off-by: Petr Machata <petrm@...lanox.com>
>> Reviewed-by: Jiri Pirko <jiri@...lanox.com>
>> Signed-off-by: Ido Schimmel <idosch@...lanox.com>
>> ---
>>  drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c  | 11 ++++++----
>>  .../ethernet/mellanox/mlx5/core/en/rep/tc.c   | 11 +++++-----
>>  .../net/ethernet/netronome/nfp/flower/main.h  |  2 +-
>>  .../ethernet/netronome/nfp/flower/offload.c   | 11 ++++++----
>>  include/net/flow_offload.h                    |  9 ++++----
>>  net/core/flow_offload.c                       | 12 +++++------
>>  net/netfilter/nf_flow_table_offload.c         | 17 +++++++--------
>>  net/netfilter/nf_tables_offload.c             | 20 ++++++++++--------
>>  net/sched/cls_api.c                           | 21 +++++++++++--------
>>  9 files changed, 63 insertions(+), 51 deletions(-)
>>
> [...]
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
>> index eefeb1cdc2ee..4fc42c1955ff 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
>> @@ -404,7 +404,7 @@ static void mlx5e_rep_indr_block_unbind(void *cb_priv)
>>  static LIST_HEAD(mlx5e_block_cb_list);
>>
>>  static int
>> -mlx5e_rep_indr_setup_block(struct net_device *netdev,
>> +mlx5e_rep_indr_setup_block(struct Qdisc *sch,
>>  			   struct mlx5e_rep_priv *rpriv,
>>  			   struct flow_block_offload *f,
>>  			   flow_setup_cb_t *setup_cb,
>> @@ -412,6 +412,7 @@ mlx5e_rep_indr_setup_block(struct net_device *netdev,
>>  			   void (*cleanup)(struct flow_block_cb *block_cb))
>>  {
>>  	struct mlx5e_priv *priv = netdev_priv(rpriv->netdev);
>> +	struct net_device *netdev = sch->dev_queue->dev;
>
> This break indirect block support for netfilter since the driver
> is assuming a Qdisc object.

Sorry, I don't follow. You mean mlx5 driver? What does it mean to
"assume a qdisc object"?

Is it incorrect to rely on the fact that the netdevice can be deduced
from a qdisc, or that there is always a qdisc associated with a block
binding point?