netdev - Re: [RFC net-next 03/17] net/smc: Remove error handling of unregister

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: <aJvF1PQ6zx53WDBi@linux.alibaba.com>
Date: Wed, 13 Aug 2025 06:53:08 +0800
From: Dust Li <dust.li@...ux.alibaba.com>
To: Alexandra Winter <wintera@...ux.ibm.com>,
	David Miller <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Eric Dumazet <edumazet@...gle.com>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"D. Wythe" <alibuda@...ux.alibaba.com>,
	Sidraya Jayagond <sidraya@...ux.ibm.com>,
	Wenjia Zhang <wenjia@...ux.ibm.com>,
	Julian Ruess <julianr@...ux.ibm.com>
Cc: netdev@...r.kernel.org, linux-s390@...r.kernel.org,
	Heiko Carstens <hca@...ux.ibm.com>,
	Vasily Gorbik <gor@...ux.ibm.com>,
	Alexander Gordeev <agordeev@...ux.ibm.com>,
	Christian Borntraeger <borntraeger@...ux.ibm.com>,
	Sven Schnelle <svens@...ux.ibm.com>,
	Thorsten Winkler <twinkler@...ux.ibm.com>,
	Simon Horman <horms@...nel.org>,
	Mahanta Jambigi <mjambigi@...ux.ibm.com>,
	Tony Lu <tonylu@...ux.alibaba.com>,
	Wen Gu <guwen@...ux.alibaba.com>, Halil Pasic <pasic@...ux.ibm.com>,
	linux-rdma@...r.kernel.org
Subject: Re: [RFC net-next 03/17] net/smc: Remove error handling of
 unregister_dmb()

On 2025-08-11 13:28:39, Alexandra Winter wrote:
>
>
>On 10.08.25 13:03, Dust Li wrote:
>> On 2025-08-06 17:41:08, Alexandra Winter wrote:
>>> smcd_buf_free() calls smc_ism_unregister_dmb(lgr->smcd, buf_desc) and
>>> then unconditionally frees buf_desc.
>>>
>>> Remove the cleaning up of fields of buf_desc in
>>> smc_ism_unregister_dmb(), because it is not helpful.
>>>
>>> This removes the only usage of ISM_ERROR from the smc module. So move it
>>> to drivers/s390/net/ism.h.
>>>
>>> Signed-off-by: Alexandra Winter <wintera@...ux.ibm.com>
>>> Reviewed-by: Mahanta Jambigi <mjambigi@...ux.ibm.com>
>>> ---
>>> drivers/s390/net/ism.h |  1 +
>>> include/net/smc.h      |  2 --
>>> net/smc/smc_ism.c      | 14 +++++---------
>>> net/smc/smc_ism.h      |  3 ++-
>>> 4 files changed, 8 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/s390/net/ism.h b/drivers/s390/net/ism.h
>>> index 047fa6101555..b5b03db52fce 100644
>>> --- a/drivers/s390/net/ism.h
>>> +++ b/drivers/s390/net/ism.h
>>> @@ -10,6 +10,7 @@
>>> #include <asm/pci_insn.h>
>>>
>>> #define UTIL_STR_LEN	16
>>> +#define ISM_ERROR	0xFFFF
>>>
>>> /*
>>>  * Do not use the first word of the DMB bits to ensure 8 byte aligned access.
>>> diff --git a/include/net/smc.h b/include/net/smc.h
>>> index db84e4e35080..a9c023dd1380 100644
>>> --- a/include/net/smc.h
>>> +++ b/include/net/smc.h
>>> @@ -44,8 +44,6 @@ struct smcd_dmb {
>>>
>>> #define ISM_RESERVED_VLANID	0x1FFF
>>>
>>> -#define ISM_ERROR	0xFFFF
>>> -
>>> struct smcd_dev;
>>>
>>> struct smcd_gid {
>>> diff --git a/net/smc/smc_ism.c b/net/smc/smc_ism.c
>>> index 84f98e18c7db..a94e1450d095 100644
>>> --- a/net/smc/smc_ism.c
>>> +++ b/net/smc/smc_ism.c
>>> @@ -205,13 +205,13 @@ int smc_ism_put_vlan(struct smcd_dev *smcd, unsigned short vlanid)
>>> 	return rc;
>>> }
>>>
>>> -int smc_ism_unregister_dmb(struct smcd_dev *smcd, struct smc_buf_desc *dmb_desc)
>>> +void smc_ism_unregister_dmb(struct smcd_dev *smcd,
>>> +			    struct smc_buf_desc *dmb_desc)
>>> {
>>> 	struct smcd_dmb dmb;
>>> -	int rc = 0;
>>>
>>> 	if (!dmb_desc->dma_addr)
>>> -		return rc;
>>> +		return;
>>>
>>> 	memset(&dmb, 0, sizeof(dmb));
>>> 	dmb.dmb_tok = dmb_desc->token;
>>> @@ -219,13 +219,9 @@ int smc_ism_unregister_dmb(struct smcd_dev *smcd, struct smc_buf_desc *dmb_desc)
>>> 	dmb.cpu_addr = dmb_desc->cpu_addr;
>>> 	dmb.dma_addr = dmb_desc->dma_addr;
>>> 	dmb.dmb_len = dmb_desc->len;
>>> -	rc = smcd->ops->unregister_dmb(smcd, &dmb);
>>> -	if (!rc || rc == ISM_ERROR) {
>>> -		dmb_desc->cpu_addr = NULL;
>>> -		dmb_desc->dma_addr = 0;
>>> -	}
>>> +	smcd->ops->unregister_dmb(smcd, &dmb);
>> 
>> Hmm, I think the old way of handling error here is certainly not good.
>> But completely ignoring error handling here would make bugs harder
>> to detect.
>> 
>> What about adding a WARN_ON_ONCE(rc) ?
>> 
>> Also, I think we can just remove the rc == ISM_ERROR to remove
>> the dependency of ISM_ERROR in smc.
>> 
>> Best regards,
>> Dust
>> 
>
>As I wrote in the commit message, I removed rc, because it is ignored by the caller anyhow today.
>If you want to I can add it back into this function and then you can think about how SMC should
>handle such an error.
>
>My thoughts on this are:
>There is not really much smc can do about a problem in unregister_dmb.
>
>I think it is Linux strategy to report and handle errors at the lowest level possible. I have some
>patches on my harddisk to improve error handling and error reporting of the ism device diver. And
>we are already discussing internally which errors should do a WARN_ON_ONCE in the ism device driver.
>
>So I don't think there is much to do in the smc layer at the moment.
>How does that sound to you?

LGTM.

Best regards,
Dust