netdev - RE: [net-next 07/16] net/mlx5: DR, Add support for dumping steering info

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, 31 Dec 2021 01:49:54 +0000
From:   Yevgeny Kliteynik <kliteyn@...dia.com>
To:     Jakub Kicinski <kuba@...nel.org>, Saeed Mahameed <saeed@...nel.org>
CC:     "David S. Miller" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Muhammad Sammar <muhammads@...dia.com>,
        Leon Romanovsky <leonro@...dia.com>,
        Saeed Mahameed <saeedm@...dia.com>
Subject: RE: [net-next  07/16] net/mlx5: DR, Add support for dumping steering
 info

Jakub,

Thanks for the review and the comments.
V2 will be sent shortly.

> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Thursday, December 30, 2021 04:17
> 
> On Tue, 28 Dec 2021 22:24:53 -0800 Saeed Mahameed wrote:
> > From: Muhammad Sammar <muhammads@...dia.com>
> >
> > Extend mlx5 debugfs support to present Software Steering resources:
> > dr_domain including it's tables, matchers and rules.
> > The interface is read-only. While dump is being presented, new steering
> rules cannot be inserted/deleted.
> 
> Looks possibly written against debugfs API as it was a few releases ago?
> The return values have changed, see below.

It appears that you're right.
Actually, this was written based on debugfs functions documentation, where
it states that "if an error occurs, NULL will be returned"

https://www.kernel.org/doc/htmldocs/filesystems/API-debugfs-create-dir.html

Looking at the code, I see that it's no longer the case.

> > +static void
> > +dr_dump_hex_print(char *dest, u32 dest_size, char *src, u32 src_size)
> > +{
> > +     int i;
> > +
> > +     if (dest_size < 2 * src_size)
> > +             return;
> > +
> +     for (i = 0; i < src_size; i++)
> +             snprintf(&dest[2 * i], BUF_SIZE, "%02x", (u8)src[i]);
> 
> bin2hex()

Good idea.

> > +}
> 
> > +static int
> > +dr_dump_rule_mem(struct seq_file *file, struct mlx5dr_ste *ste,
> > +              bool is_rx, const u64 rule_id, u8 format_ver)
> > +{
> > +     char hw_ste_dump[BUF_SIZE] = {};
> 
> seems a little wasteful to zero-init this entire buffer

Ack.

> > +void mlx5dr_dbg_init_dump(struct mlx5dr_domain *dmn)
> > +{
> > +     struct mlx5_core_dev *dev = dmn->mdev;
> > +     char file_name[128];
> > +
> > +     if (dmn->type != MLX5DR_DOMAIN_TYPE_FDB) {
> > +             mlx5_core_warn(dev,
> > +                            "Steering dump is not supported for NIC RX/TX domains\n");
> > +             return;
> > +     }
> > +
> > +     if (!dmn->dump_info.steering_debugfs) {
> > +             dmn->dump_info.steering_debugfs = debugfs_create_dir("steering",
> > +                                                                  dev->priv.dbg_root);
> > +             if (!dmn->dump_info.steering_debugfs)
> 
> debugfs functions no longer return NULL.

Ack.
 
> > +                     return;
> > +     }
> > +
> > +     if (!dmn->dump_info.fdb_debugfs) {
> > +             dmn->dump_info.fdb_debugfs = debugfs_create_dir("fdb",
> > +                                                             dmn->dump_info.steering_debugfs);
> > +             if (!dmn->dump_info.fdb_debugfs) {
> > 
> > ditto, in fact you're not supposed to check the return values of these
> > functions at all, they all check if parent is an error pointer an exit
> > cleanly, so since this is a debug feature just carry on without error
> > checking

Indeed, this is true both for debugfs_create_dir and debugfs_create_file.
This makes the code much cleaner here. Thanks!

-- YK