netdev - Re: [PATCH net-next v2 4/7] net: nexthop: Expose nexthop group stats to user space

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: <CANn89iLDizzEKi7u0NssSXD_rB6c8EeL==ino-O0a2_BxUN5tw@mail.gmail.com>
Date: Fri, 1 Mar 2024 18:32:26 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Petr Machata <petrm@...dia.com>
Cc: "David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org, 
	Ido Schimmel <idosch@...dia.com>, David Ahern <dsahern@...nel.org>, Simon Horman <horms@...nel.org>, 
	mlxsw@...dia.com
Subject: Re: [PATCH net-next v2 4/7] net: nexthop: Expose nexthop group stats
 to user space

On Thu, Feb 29, 2024 at 7:20 PM Petr Machata <petrm@...dia.com> wrote:
>
> From: Ido Schimmel <idosch@...dia.com>
>
> Add netlink support for reading NH group stats.
>
> This data is only for statistics of the traffic in the SW datapath. HW
> nexthop group statistics will be added in the following patches.
>
> Emission of the stats is keyed to a new op_stats flag to avoid cluttering
> the netlink message with stats if the user doesn't need them:
> NHA_OP_FLAG_DUMP_STATS.
>
> Co-developed-by: Petr Machata <petrm@...dia.com>
> Signed-off-by: Petr Machata <petrm@...dia.com>
> Signed-off-by: Ido Schimmel <idosch@...dia.com>
> ---
>
> Notes:
>     v2:
>     - Use uint to encode NHA_GROUP_STATS_ENTRY_PACKETS
>     - Rename jump target in nla_put_nh_group_stats() to avoid
>       having to rename further in the patchset.
>
>  include/uapi/linux/nexthop.h | 30 ++++++++++++
>  net/ipv4/nexthop.c           | 92 ++++++++++++++++++++++++++++++++----
>  2 files changed, 114 insertions(+), 8 deletions(-)
>
> diff --git a/include/uapi/linux/nexthop.h b/include/uapi/linux/nexthop.h
> index 086444e2946c..f4db63c17085 100644
> --- a/include/uapi/linux/nexthop.h
> +++ b/include/uapi/linux/nexthop.h
> @@ -30,6 +30,8 @@ enum {
>
>  #define NEXTHOP_GRP_TYPE_MAX (__NEXTHOP_GRP_TYPE_MAX - 1)
>
> +#define NHA_OP_FLAG_DUMP_STATS         BIT(0)
> +
>  enum {
>         NHA_UNSPEC,
>         NHA_ID,         /* u32; id for nexthop. id == 0 means auto-assign */
> @@ -63,6 +65,9 @@ enum {
>         /* u32; operation-specific flags */
>         NHA_OP_FLAGS,
>
> +       /* nested; nexthop group stats */
> +       NHA_GROUP_STATS,
> +
>         __NHA_MAX,
>  };
>
> @@ -104,4 +109,29 @@ enum {
>
>  #define NHA_RES_BUCKET_MAX     (__NHA_RES_BUCKET_MAX - 1)
>
> +enum {
> +       NHA_GROUP_STATS_UNSPEC,
> +
> +       /* nested; nexthop group entry stats */
> +       NHA_GROUP_STATS_ENTRY,
> +
> +       __NHA_GROUP_STATS_MAX,
> +};
> +
> +#define NHA_GROUP_STATS_MAX    (__NHA_GROUP_STATS_MAX - 1)
> +
> +enum {
> +       NHA_GROUP_STATS_ENTRY_UNSPEC,
> +
> +       /* u32; nexthop id of the nexthop group entry */
> +       NHA_GROUP_STATS_ENTRY_ID,
> +
> +       /* uint; number of packets forwarded via the nexthop group entry */
> +       NHA_GROUP_STATS_ENTRY_PACKETS,
> +
> +       __NHA_GROUP_STATS_ENTRY_MAX,
> +};
> +
> +#define NHA_GROUP_STATS_ENTRY_MAX      (__NHA_GROUP_STATS_ENTRY_MAX - 1)
> +
>  #endif
> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
> index 4be66622e24f..0ede8777bd66 100644
> --- a/net/ipv4/nexthop.c
> +++ b/net/ipv4/nexthop.c
> @@ -41,7 +41,8 @@ static const struct nla_policy rtm_nh_policy_new[] = {
>
>  static const struct nla_policy rtm_nh_policy_get[] = {
>         [NHA_ID]                = { .type = NLA_U32 },
> -       [NHA_OP_FLAGS]          = NLA_POLICY_MASK(NLA_U32, 0),
> +       [NHA_OP_FLAGS]          = NLA_POLICY_MASK(NLA_U32,
> +                                                 NHA_OP_FLAG_DUMP_STATS),
>  };
>
>  static const struct nla_policy rtm_nh_policy_del[] = {
> @@ -53,7 +54,8 @@ static const struct nla_policy rtm_nh_policy_dump[] = {
>         [NHA_GROUPS]            = { .type = NLA_FLAG },
>         [NHA_MASTER]            = { .type = NLA_U32 },
>         [NHA_FDB]               = { .type = NLA_FLAG },
> -       [NHA_OP_FLAGS]          = NLA_POLICY_MASK(NLA_U32, 0),
> +       [NHA_OP_FLAGS]          = NLA_POLICY_MASK(NLA_U32,
> +                                                 NHA_OP_FLAG_DUMP_STATS),
>  };
>
>  static const struct nla_policy rtm_nh_res_policy_new[] = {
> @@ -661,8 +663,77 @@ static int nla_put_nh_group_res(struct sk_buff *skb, struct nh_group *nhg)
>         return -EMSGSIZE;
>  }
>
> -static int nla_put_nh_group(struct sk_buff *skb, struct nh_group *nhg)
> +static void nh_grp_entry_stats_read(struct nh_grp_entry *nhge,
> +                                   struct nh_grp_entry_stats *stats)
>  {
> +       int i;
> +
> +       memset(stats, 0, sizeof(*stats));
> +       for_each_possible_cpu(i) {
> +               struct nh_grp_entry_stats *cpu_stats;
> +               unsigned int start;
> +               u64 packets;
> +
> +               cpu_stats = per_cpu_ptr(nhge->stats, i);
> +               do {
> +                       start = u64_stats_fetch_begin(&cpu_stats->syncp);
> +                       packets = cpu_stats->packets;

This is not safe, even on 64bit arches.

You should use u64_stats_t, u64_stats_read(), u64_stats_add() ...

> +               } while (u64_stats_fetch_retry(&cpu_stats->syncp, start));
> +
> +               stats->packets += packets;
> +       }
> +}