netdev - Re: [PATCH net-next,v4 05/12] flow_offload: add statistics retrieval infrastructure and use it

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: <20181129124822.178e17a6@cakuba.netronome.com>
Date:   Thu, 29 Nov 2018 12:48:22 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Pablo Neira Ayuso <pablo@...filter.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net,
        thomas.lendacky@....com, f.fainelli@...il.com,
        ariel.elior@...ium.com, michael.chan@...adcom.com,
        santosh@...lsio.com, madalin.bucur@....com,
        yisen.zhuang@...wei.com, salil.mehta@...wei.com,
        jeffrey.t.kirsher@...el.com, tariqt@...lanox.com,
        saeedm@...lanox.com, jiri@...lanox.com, idosch@...lanox.com,
        peppe.cavallaro@...com, grygorii.strashko@...com, andrew@...n.ch,
        vivien.didelot@...oirfairelinux.com, alexandre.torgue@...com,
        joabreu@...opsys.com, linux-net-drivers@...arflare.com,
        ganeshgr@...lsio.com, ogerlitz@...lanox.com,
        Manish.Chopra@...ium.com, marcelo.leitner@...il.com
Subject: Re: [PATCH net-next,v4 05/12] flow_offload: add statistics
 retrieval infrastructure and use it

On Thu, 29 Nov 2018 03:22:24 +0100, Pablo Neira Ayuso wrote:
> This patch provides the flow_stats structure that acts as container for
> tc_cls_flower_offload, then we can use to restore the statistics on the
> existing TC actions. Hence, tcf_exts_stats_update() is not used from
> drivers anymore.
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@...filter.org>

> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> index dabc819b6cc9..040c092c000a 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -179,4 +179,18 @@ static inline bool flow_rule_match_key(const struct flow_rule *rule,
>  	return dissector_uses_key(rule->match.dissector, key);
>  }
>  
> +struct flow_stats {
> +	u64	pkts;
> +	u64	bytes;
> +	u64	lastused;
> +};
> +
> +static inline void flow_stats_update(struct flow_stats *flow_stats,
> +				     u64 pkts, u64 bytes, u64 lastused)
> +{
> +	flow_stats->pkts	= pkts;
> +	flow_stats->bytes	= bytes;
> +	flow_stats->lastused	= lastused;
> +}
> +
>  #endif /* _NET_FLOW_OFFLOAD_H */
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index abb035f84321..a08c06e383db 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -767,6 +767,7 @@ struct tc_cls_flower_offload {
>  	enum tc_fl_command command;
>  	unsigned long cookie;
>  	struct flow_rule *rule;
> +	struct flow_stats stats;
>  	struct tcf_exts *exts;
>  	u32 classid;
>  };
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index 8898943b8ee6..b88cf29aff7b 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -432,6 +432,10 @@ static void fl_hw_update_stats(struct tcf_proto *tp, struct cls_fl_filter *f)
>  
>  	tc_setup_cb_call(block, &f->exts, TC_SETUP_CLSFLOWER,
>  			 &cls_flower, false);
> +
> +	tcf_exts_stats_update(&f->exts, cls_flower.stats.bytes,
> +			      cls_flower.stats.pkts,
> +			      cls_flower.stats.lastused);
>  }
>  
>  static bool __fl_delete(struct tcf_proto *tp, struct cls_fl_filter *f,

Apart from the bug Venkat mentioned I think this patch exposes a
potentially strange and unexpected TC-ism through to the abstracted API.
The stats for TC store the increment in update cycle, so flow->stats
will be equal to the diff between TC_CLSFLOWER_STATS calls.

Its this behaviour going to be subsystem-specific?  Looks not-so-clean.