netdev - Re: [PATCH net-next v2 12/15] net/smc: Add support for obtaining SMCD device list

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] [day] [month] [year] [list]

Message-ID: <0c656b51-cd05-33a0-9bb7-cdfc21131132@linux.ibm.com>
Date:   Fri, 6 Nov 2020 07:38:10 +0100
From:   Karsten Graul <kgraul@...ux.ibm.com>
To:     Saeed Mahameed <saeed@...nel.org>, davem@...emloft.net
Cc:     netdev@...r.kernel.org, linux-s390@...r.kernel.org,
        hca@...ux.ibm.com, raspl@...ux.ibm.com,
        Guevenc Guelce <guvenc@...ux.ibm.com>
Subject: Re: [PATCH net-next v2 12/15] net/smc: Add support for obtaining SMCD
 device list

On 04/11/2020 02:31, Saeed Mahameed wrote:
> On Tue, 2020-11-03 at 11:25 +0100, Karsten Graul wrote:
>> From: Guvenc Gulce <guvenc@...ux.ibm.com>
>>
>> Deliver SMCD device information via netlink based
>> diagnostic interface.
>>
>> Signed-off-by: Guvenc Gulce <guvenc@...ux.ibm.com>
>> Signed-off-by: Karsten Graul <kgraul@...ux.ibm.com>
>> ---
>>  include/uapi/linux/smc.h      |  2 +
>>  include/uapi/linux/smc_diag.h | 20 +++++++++
>>  net/smc/smc_core.h            | 27 +++++++++++++
>>  net/smc/smc_diag.c            | 76
>> +++++++++++++++++++++++++++++++++++
>>  net/smc/smc_ib.h              |  1 -
>>  5 files changed, 125 insertions(+), 1 deletion(-)
>>
> 
>> +
>> +static int smc_diag_prep_smcd_dev(struct smcd_dev_list *dev_list,
>> +				  struct sk_buff *skb,
>> +				  struct netlink_callback *cb,
>> +				  struct smc_diag_req_v2 *req)
>> +{
>> +	struct smc_diag_dump_ctx *cb_ctx = smc_dump_context(cb);
>> +	int snum = cb_ctx->pos[0];
>> +	struct smcd_dev *smcd;
>> +	int rc = 0, num = 0;
>> +
>> +	mutex_lock(&dev_list->mutex);
>> +	list_for_each_entry(smcd, &dev_list->list, list) {
>> +		if (num < snum)
>> +			goto next;
>> +		rc = smc_diag_handle_smcd_dev(smcd, skb, cb, req);
>> +		if (rc < 0)
>> +			goto errout;
>> +next:
>> +		num++;
>> +	}
>> +errout:
>> +	mutex_unlock(&dev_list->mutex);
>> +	cb_ctx->pos[0] = num;
>> +	return rc;
>> +}
>> +
> 
> this function pattern repeats at least 4 times in this series and the
> only difference is the diag handler function, just abstract this
> function out and pass a function pointer as handler to reduce code
> repetition. 
> 

Thank you for your review. We will come up with a v3 to address the comments.

We plan to eliminate additional EXPORTs using an ops array that allows smc_diag to 
retrieve the needed information from the smc module.

We discussed the above comment as well, but there is no clean and easy way to change
it because (nearly) all places iterate over different lists that have different types.
It might be not a good idea to loose type safety here by calling different handlers 
with a void pointer as parameter. Additionally some lists require specific locks.

-- 
Karsten

(I'm a dude)