linux-kernel - Re: [PATCH net] net/smc: correct the reason code in smc_listen_find

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]

Date:   Tue, 17 Oct 2023 16:52:10 +0800
From:   Guangguan Wang <guangguan.wang@...ux.alibaba.com>
To:     dust.li@...ux.alibaba.com, kgraul@...ux.ibm.com,
        wenjia@...ux.ibm.com, jaka@...ux.ibm.com, davem@...emloft.net,
        edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com
Cc:     tonylu@...ux.alibaba.com, alibuda@...ux.alibaba.com,
        guwen@...ux.alibaba.com, linux-s390@...r.kernel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net/smc: correct the reason code in
 smc_listen_find_device when fallback



On 2023/10/17 15:37, Dust Li wrote:
> On Mon, Oct 16, 2023 at 02:11:53PM +0800, Guangguan Wang wrote:
> 
> Hi guangguan,
> 
> This patch doesn't apply clearly on net because it conflict with my
> previous patch 4abbd2e3c1db(net/smc: return the right falback reason
> when prefix checks fail), pls rebase.
>

This patch sent before the patch 4abbd2e3c1db applied. I will rebase it
in the next version.
 
>> The function smc_find_ism_store_rc is not only used for ism, so it is
>> reasonable to change the function name to smc_find_device_store_rc.
>>
>> The ini->rc is used to store the last error happened when finding usable
>> ism or rdma device in smc_listen_find_device, and is set by calling smc_
>> find_device_store_rc. Once the ini->rc is assigned to an none-zero value,
>> the value can not be overwritten anymore. So the ini-rc should be set to
>> the error reason only when an error actually occurs.
>>
>> When finding ISM/RDMA devices, device not found is not a real error, as
>> not all machine have ISM/RDMA devices. Failures after device found, when
>> initializing device or when initializing connection, is real errors, and
>> should be store in ini->rc.
>>
>> SMC_CLC_DECL_DIFFPREFIX also is not a real error, as for SMC-RV2, it is
>> not require same prefix.
> 
> I think it's better to seperate this patch into 2:
> - one for changing the name from smc_find_ism_store_rc to smc_find_device_store_rc.
> - one for fixing the return reason.
> 
> More comments below.
> 
OK, I will seperate it.
>>
>> Signed-off-by: Guangguan Wang <guangguan.wang@...ux.alibaba.com>

>> @@ -2213,11 +2211,12 @@ static void smc_find_ism_v1_device_serv(struct smc_sock *new_smc,
>> 		goto not_found;
>> 	ini->ism_selected = 0;
>> 	rc = smc_listen_ism_init(new_smc, ini);
>> -	if (!rc)
>> +	if (!rc) {
>> +		smc_find_device_store_rc(rc, ini);
> 
> This smc_find_device_store_rc() seems useless when rc == 0 here ?

Oh, I see. The code here should be:

rc = smc_listen_ism_init(new_smc, ini);
if (!rc)
    return;
smc_find_device_store_rc(rc, ini);

not_found:
    ini->smcd_version &= ~SMC_V1;
    ini->ism_dev[0] = NULL;
    ini->is_smcd = false;


Thanks,
Guangguan Wang.
> 
>> 		return;		/* V1 ISM device found */
>> +	}
>>
>> not_found:
>> -	smc_find_ism_store_rc(rc, ini);
>> 	ini->smcd_version &= ~SMC_V1;
>> 	ini->ism_dev[0] = NULL;
>> 	ini->is_smcd = false;