netdev - Re: [PATCH rdma-next 06/10] RDMA/nldev: Add support to add and remove optional counters

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: <abcb4753-7bf9-54de-29a2-f384ac283faf@nvidia.com>
Date:   Tue, 24 Aug 2021 10:09:47 +0800
From:   Mark Zhang <markzhang@...dia.com>
To:     Jason Gunthorpe <jgg@...dia.com>
CC:     <dledford@...hat.com>, <saeedm@...dia.com>,
        <linux-rdma@...r.kernel.org>, <netdev@...r.kernel.org>,
        <aharonl@...dia.com>, <netao@...dia.com>, <leonro@...dia.com>
Subject: Re: [PATCH rdma-next 06/10] RDMA/nldev: Add support to add and remove
 optional counters

On 8/24/2021 3:42 AM, Jason Gunthorpe wrote:
> On Wed, Aug 18, 2021 at 02:24:24PM +0300, Mark Zhang wrote:
>> From: Aharon Landau <aharonl@...dia.com>
>>
>> This patch adds the ability to add/remove optional counter to a link
>> through RDMA netlink. Limit it to users with ADMIN capability only.
>>
>> Examples:
>> $ sudo rdma statistic add link rocep8s0f0/1 optional-set cc_rx_ce_pkts
>> $ sudo rdma statistic remove link rocep8s0f0/1 optional-set cc_rx_ce_pkts
>>
>> Signed-off-by: Aharon Landau <aharonl@...dia.com>
>> Signed-off-by: Neta Ostrovsky <netao@...dia.com>
>> Signed-off-by: Mark Zhang <markzhang@...dia.com>
>>   drivers/infiniband/core/counters.c | 50 ++++++++++++++++
>>   drivers/infiniband/core/device.c   |  2 +
>>   drivers/infiniband/core/nldev.c    | 93 ++++++++++++++++++++++++++++++
>>   include/rdma/ib_verbs.h            |  7 +++
>>   include/rdma/rdma_counter.h        |  4 ++
>>   include/rdma/rdma_netlink.h        |  1 +
>>   include/uapi/rdma/rdma_netlink.h   |  9 +++
>>   7 files changed, 166 insertions(+)
>>
>> diff --git a/drivers/infiniband/core/counters.c b/drivers/infiniband/core/counters.c

...

>> +static int nldev_stat_set_op_stat(struct sk_buff *skb,
>> +				  struct nlmsghdr *nlh,
>> +				  struct netlink_ext_ack *extack,
>> +				  bool cmd_add)
>> +{

...

>> +
>> +	nlh = nlmsg_put(msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
>> +			RDMA_NL_GET_TYPE(RDMA_NL_NLDEV,
>> +					 (cmd_add ?
>> +					  RDMA_NLDEV_CMD_STAT_ADD_OPCOUNTER :
>> +					  RDMA_NLDEV_CMD_STAT_REMOVE_OPCOUNTER)),
>> +			0, 0);
>> +
>> +	if (cmd_add)
>> +		ret = rdma_opcounter_add(device, port, opcounter);
>> +	else
>> +		ret = rdma_opcounter_remove(device, port, opcounter);
>> +	if (ret)
>> +		goto err_msg;
>> +
>> +	nlmsg_end(msg, nlh);
> 
> Shouldn't the netlink message for a 'set' always return the current
> value of the thing being set on return? Eg the same output that GET
> would generate?

May I ask why can't just return an error code?

>> +static int nldev_stat_add_op_stat_doit(struct sk_buff *skb,
>> +				       struct nlmsghdr *nlh,
>> +				       struct netlink_ext_ack *extack)
>> +{
>> +	return nldev_stat_set_op_stat(skb, nlh, extack, true);
>> +}
>> +
>> +static int nldev_stat_remove_op_stat_doit(struct sk_buff *skb,
>> +					  struct nlmsghdr *nlh,
>> +					  struct netlink_ext_ack *extack)
>> +{
>> +	return nldev_stat_set_op_stat(skb, nlh, extack, false);
>> +}
>> +
>>   static int nldev_stat_set_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
>>   			       struct netlink_ext_ack *extack)
>>   {
>> @@ -2342,6 +2427,14 @@ static const struct rdma_nl_cbs nldev_cb_table[RDMA_NLDEV_NUM_OPS] = {
>>   		.dump = nldev_res_get_mr_raw_dumpit,
>>   		.flags = RDMA_NL_ADMIN_PERM,
>>   	},
>> +	[RDMA_NLDEV_CMD_STAT_ADD_OPCOUNTER] = {
>> +		.doit = nldev_stat_add_op_stat_doit,
>> +		.flags = RDMA_NL_ADMIN_PERM,
>> +	},
>> +	[RDMA_NLDEV_CMD_STAT_REMOVE_OPCOUNTER] = {
>> +		.doit = nldev_stat_remove_op_stat_doit,
>> +		.flags = RDMA_NL_ADMIN_PERM,
>> +	},
>>   };
> 
> And here I wonder if this is the cannonical way to manipulate lists of
> strings in netlink? I'm trying to think of another case like this, did
> you reference something?

For add/remove, we only support one op-counter at one time (for 
simplicity), so it's just a string, not a list of string.

This is supported:
#  rdma stat add link mlx5_0/1 optional-set cc_rx_ce_pkts

This is not supported:
# rdma stat add link mlx5_0/1 optional-set cc_rx_ce_pkts cc_tx_cnp_pkts

> Are you sure this shouldn't be done via some set on some counter
> object?

Currently we don't support do on a counter object, just per-port.

> Jason
>