netdev - Re: [PATCH v10 03/25] net/ethtool: add ULP_DDP_{GET,SET} operations for caps and stats

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: <253k011urcd.fsf@mtr-vdi-124.i-did-not-set--mail-host-address--so-tickle-me>
Date:   Wed, 01 Feb 2023 21:18:42 +0200
From:   Aurelien Aptel <aaptel@...dia.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     linux-nvme@...ts.infradead.org, netdev@...r.kernel.org,
        sagi@...mberg.me, hch@....de, kbusch@...nel.org, axboe@...com,
        chaitanyak@...dia.com, davem@...emloft.net,
        aurelien.aptel@...il.com, smalin@...dia.com, malin1024@...il.com,
        ogerlitz@...dia.com, yorayz@...dia.com, borisp@...dia.com
Subject: Re: [PATCH v10 03/25] net/ethtool: add ULP_DDP_{GET,SET} operations
 for caps and stats

Hi,

We noticed your recent changes and we will adapt the patch to use them.

Jakub Kicinski <kuba@...nel.org> writes:
>>       ETHTOOL_MSG_MM_SET,
>> +     ETHTOOL_MSG_ULP_DDP_GET,
>> +     ETHTOOL_MSG_ULP_DDP_SET,
>
> Please add the definition of the command to
> Documentation/netlink/specs/ethtool.yaml

Ok.

>>       /* add new constants above here */
>>       __ETHTOOL_MSG_USER_CNT,
>> @@ -109,6 +111,8 @@ enum {
>>       ETHTOOL_MSG_PLCA_NTF,
>>       ETHTOOL_MSG_MM_GET_REPLY,
>>       ETHTOOL_MSG_MM_NTF,
>> +     ETHTOOL_MSG_ULP_DDP_GET_REPLY,
>> +     ETHTOOL_MSG_ULP_DDP_SET_REPLY,
>
> What about notifications?

Ok. We will add it and use the return value introduced in your latest
refactor commits.

>> +#include "netlink.h"
>> +#include "common.h"
>> +#include "bitset.h"
>
> alphabetic order?

bitset.h depends on netlink.h but it doesn't include it: it doesn't
compile if you change the include order. We will keep it as is.

>> +static int ulp_ddp_stats64_size(unsigned int count)
>> +{
>> +     unsigned int len = 0;
>> +     unsigned int i;
>> +
>> +     for (i = 0; i < count; i++)
>> +             len += nla_total_size(sizeof(u64));
>
> len = nla_total_size(sizeof(u64)) * count
> ?
> but it's not correct. You need nla_total_size_64bit() here

Ok

>> +     /* outermost nest */
>> +     return nla_total_size(len);
>
> nla_total_size(0) is more common for nests.

Ok

>> +             if (nla_put_64bit(skb, i+1, sizeof(u64), &val[i], 0))
>
> nla_put_u64_64bit()
> And you'll need to add an attr for padding.

Ok

>> +const struct nla_policy ethnl_ulp_ddp_set_policy[] = {
>> +     [ETHTOOL_A_ULP_DDP_HEADER]      =
>> +             NLA_POLICY_NESTED(ethnl_header_policy),
>> +     [ETHTOOL_A_ULP_DDP_WANTED]      = { .type = NLA_NESTED },
>> +};
>
> Let's link the policy here: NLA_POLICY_NESTED(bitset_policy).

bitset_policy is not exported by bitset.c (static).
We will rename it ethnl_bitset_policy and export it.

>> +nla_put_failure:
>> +     nlmsg_free(rskb);
>> +     WARN_ONCE(1, "calculated message payload length (%d) not sufficient\n",
>> +               reply_len);
>> +err:
>> +     GENL_SET_ERR_MSG(info, "failed to send reply message");
>
> Don't overwrite the message, the message should be set close to
> the error, if needed.

Ok.

>> +     if (!tb[ETHTOOL_A_ULP_DDP_WANTED])
>
> GENL_REQ_ATTR_CHECK()

Ok. We will use this macro and move it to a .set_validate() callback.

> We should pass extack to the driver, so that the driver can report a
> meaningful error

Ok.

>> +             if (ret)
>> +                     netdev_err(dev, "set_ulp_ddp_capabilities() returned error %d\n", ret);
>
> and drop this

Ok.

Thanks.