| 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
| ||
|
Message-ID: <20200710152648.GA14902@salvia>
Date: Fri, 10 Jul 2020 17:26:48 +0200
From: Pablo Neira Ayuso <pablo@...filter.org>
To: Petr Machata <petrm@...lanox.com>
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
On Fri, Jul 10, 2020 at 05:15:39PM +0200, Petr Machata wrote:
>
> 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?
The drivers assume that the xyz_indr_setup_block() always gets a sch
object, which is not always true. Are you really sure this will work
for the TC CT offload?
--- a/net/netfilter/nf_flow_table_offload.c
+++ b/net/netfilter/nf_flow_table_offload.c
@@ -928,26 +928,27 @@ static int nf_flow_table_block_setup(struct nf_flowtable *flowtable,
}
static void nf_flow_table_block_offload_init(struct flow_block_offload *bo,
- struct net *net,
+ struct net_device *dev,
enum flow_block_command cmd,
struct nf_flowtable *flowtable,
struct netlink_ext_ack *extack)
{
memset(bo, 0, sizeof(*bo));
- bo->net = net;
+ bo->net = dev_net(dev);
bo->block = &flowtable->flow_block;
bo->command = cmd;
bo->binder_type = FLOW_BLOCK_BINDER_TYPE_CLSACT_INGRESS;
bo->extack = extack;
+ bo->sch = dev_ingress_queue(dev)->qdisc_sleeping;
INIT_LIST_HEAD(&bo->cb_list);
}
static void nf_flow_table_indr_cleanup(struct flow_block_cb *block_cb)
{
struct nf_flowtable *flowtable = block_cb->indr.data;
- struct net_device *dev = block_cb->indr.dev;
+ struct Qdisc *sch = block_cb->indr.sch;
- nf_flow_table_gc_cleanup(flowtable, dev);
+ nf_flow_table_gc_cleanup(flowtable, sch->dev_queue->dev);
down_write(&flowtable->flow_block_lock);
list_del(&block_cb->list);
list_del(&block_cb->driver_list);
Moreover, the flow_offload infrastructure should also remain
independent from the front-end, either tc/netfilter/ethtool, this is
pulling in tc specific stuff into it, eg.
diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index de395498440d..fda29140bdc5 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -444,6 +444,7 @@ struct flow_block_offload {
struct list_head cb_list;
struct list_head *driver_block_list;
struct netlink_ext_ack *extack;
+ struct Qdisc *sch;
};
enum tc_setup_type;
@@ -454,7 +455,7 @@ struct flow_block_cb;
struct flow_block_indr {
struct list_head list;
- struct net_device *dev;
+ struct Qdisc *sch;
enum flow_block_binder_type binder_type;
void *data;
void *cb_priv;
@@ -479,7 +480,7 @@ struct flow_block_cb *flow_indr_block_cb_alloc(flow_setup_cb_t *cb,
void *cb_ident, void *cb_priv,
void (*release)(void *cb_priv),
struct flow_block_offload *bo,
- struct net_device *dev, void *data,
+ struct Qdisc *sch, void *data,
void *indr_cb_priv,
void (*cleanup)(struct flow_block_cb *block_cb));
void flow_block_cb_free(struct flow_block_cb *block_cb);
Powered by blists - more mailing lists