* [PATCH 0/2] net/smc: Adjustments for two function implementations
@ 2023-12-31 14:55 Markus Elfring
2023-12-31 14:58 ` [PATCH 1/2] net/smc: Return directly after a failed kzalloc() in smc_fill_gid_list() Markus Elfring
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Markus Elfring @ 2023-12-31 14:55 UTC (permalink / raw)
To: linux-s390, netdev, kernel-janitors, David S. Miller, D. Wythe,
Eric Dumazet, Jakub Kicinski, Jan Karcher, Paolo Abeni, Tony Lu,
Wen Gu, Wenjia Zhang
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 31 Dec 2023 15:48:45 +0100
A few update suggestions were taken into account
from static source code analysis.
Markus Elfring (2):
Return directly after a failed kzalloc() in smc_fill_gid_list()
Improve exception handling in smc_llc_cli_add_link_invite()
net/smc/af_smc.c | 2 +-
net/smc/smc_llc.c | 15 +++++++--------
2 files changed, 8 insertions(+), 9 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] net/smc: Return directly after a failed kzalloc() in smc_fill_gid_list()
2023-12-31 14:55 [PATCH 0/2] net/smc: Adjustments for two function implementations Markus Elfring
@ 2023-12-31 14:58 ` Markus Elfring
2024-01-02 2:13 ` Tony Lu
2023-12-31 15:00 ` [PATCH 2/2] net/smc: Improve exception handling in smc_llc_cli_add_link_invite() Markus Elfring
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Markus Elfring @ 2023-12-31 14:58 UTC (permalink / raw)
To: linux-s390, netdev, kernel-janitors, David S. Miller, D. Wythe,
Eric Dumazet, Jakub Kicinski, Jan Karcher, Paolo Abeni, Tony Lu,
Wen Gu, Wenjia Zhang
Cc: LKML
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 31 Dec 2023 15:15:19 +0100
The kfree() function was called in one case by
the smc_fill_gid_list() function during error handling
even if the passed variable contained a null pointer.
This issue was detected by using the Coccinelle software.
Thus return directly after a call of the function “kzalloc” failed
at the beginning.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
net/smc/af_smc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 7fc2f3c6d248..a396a9977ba9 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -1180,7 +1180,7 @@ void smc_fill_gid_list(struct smc_link_group *lgr,
alt_ini = kzalloc(sizeof(*alt_ini), GFP_KERNEL);
if (!alt_ini)
- goto out;
+ return;
alt_ini->vlan_id = lgr->vlan_id;
alt_ini->check_smcrv2 = true;
--
2.43.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/2] net/smc: Improve exception handling in smc_llc_cli_add_link_invite()
2023-12-31 14:55 [PATCH 0/2] net/smc: Adjustments for two function implementations Markus Elfring
2023-12-31 14:58 ` [PATCH 1/2] net/smc: Return directly after a failed kzalloc() in smc_fill_gid_list() Markus Elfring
@ 2023-12-31 15:00 ` Markus Elfring
2023-12-31 22:09 ` kernel test robot
` (2 more replies)
2024-01-02 8:13 ` [PATCH 0/2] net/smc: Adjustments for two function implementations Wen Gu
2024-01-03 13:44 ` [PATCH 0/2] " Wenjia Zhang
3 siblings, 3 replies; 15+ messages in thread
From: Markus Elfring @ 2023-12-31 15:00 UTC (permalink / raw)
To: linux-s390, netdev, kernel-janitors, David S. Miller, D. Wythe,
Eric Dumazet, Jakub Kicinski, Jan Karcher, Paolo Abeni, Tony Lu,
Wen Gu, Wenjia Zhang
Cc: LKML
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 31 Dec 2023 15:42:07 +0100
The kfree() function was called in some cases by
the smc_llc_cli_add_link_invite() function during error handling
even if the passed variable contained a null pointer.
This issue was detected by using the Coccinelle software.
* Thus use another label.
* Merge two if statements.
* Omit an initialisation (for the variable “ini”)
which became unnecessary with this refactoring.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
net/smc/smc_llc.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/net/smc/smc_llc.c b/net/smc/smc_llc.c
index 018ce8133b02..2ff24a7feb26 100644
--- a/net/smc/smc_llc.c
+++ b/net/smc/smc_llc.c
@@ -1163,23 +1163,21 @@ static void smc_llc_cli_add_link_invite(struct smc_link *link,
struct smc_llc_qentry *qentry)
{
struct smc_link_group *lgr = smc_get_lgr(link);
- struct smc_init_info *ini = NULL;
+ struct smc_init_info *ini;
if (lgr->smc_version == SMC_V2) {
smc_llc_send_request_add_link(link);
- goto out;
+ goto free_qentry;
}
if (lgr->type == SMC_LGR_SYMMETRIC ||
- lgr->type == SMC_LGR_ASYMMETRIC_PEER)
- goto out;
-
- if (lgr->type == SMC_LGR_SINGLE && lgr->max_links <= 1)
- goto out;
+ lgr->type == SMC_LGR_ASYMMETRIC_PEER ||
+ lgr->type == SMC_LGR_SINGLE && lgr->max_links <= 1)
+ goto free_qentry;
ini = kzalloc(sizeof(*ini), GFP_KERNEL);
if (!ini)
- goto out;
+ goto free_qentry;
ini->vlan_id = lgr->vlan_id;
smc_pnet_find_alt_roce(lgr, ini, link->smcibdev);
@@ -1190,6 +1188,7 @@ static void smc_llc_cli_add_link_invite(struct smc_link *link,
ini->ib_gid, NULL, SMC_LLC_REQ);
out:
kfree(ini);
+free_qentry:
kfree(qentry);
}
--
2.43.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] net/smc: Improve exception handling in smc_llc_cli_add_link_invite()
2023-12-31 15:00 ` [PATCH 2/2] net/smc: Improve exception handling in smc_llc_cli_add_link_invite() Markus Elfring
@ 2023-12-31 22:09 ` kernel test robot
2023-12-31 22:40 ` kernel test robot
2024-01-02 2:19 ` Tony Lu
2 siblings, 0 replies; 15+ messages in thread
From: kernel test robot @ 2023-12-31 22:09 UTC (permalink / raw)
To: Markus Elfring, linux-s390, netdev, kernel-janitors,
David S. Miller, D. Wythe, Eric Dumazet, Jakub Kicinski,
Jan Karcher, Paolo Abeni, Tony Lu, Wen Gu, Wenjia Zhang
Cc: llvm, oe-kbuild-all, LKML
Hi Markus,
kernel test robot noticed the following build warnings:
[auto build test WARNING on net/main]
[also build test WARNING on net-next/main linus/master v6.7-rc7 next-20231222]
[cannot apply to horms-ipvs/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Markus-Elfring/net-smc-Return-directly-after-a-failed-kzalloc-in-smc_fill_gid_list/20231231-231406
base: net/main
patch link: https://lore.kernel.org/r/5253e660-6b66-4775-ae2f-06f5a1d40be5%40web.de
patch subject: [PATCH 2/2] net/smc: Improve exception handling in smc_llc_cli_add_link_invite()
config: x86_64-randconfig-002-20231231 (https://download.01.org/0day-ci/archive/20240101/202401010536.XhzcWDPH-lkp@intel.com/config)
compiler: ClangBuiltLinux clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240101/202401010536.XhzcWDPH-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401010536.XhzcWDPH-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> net/smc/smc_llc.c:1175:34: warning: '&&' within '||' [-Wlogical-op-parentheses]
1174 | lgr->type == SMC_LGR_ASYMMETRIC_PEER ||
| ~~
1175 | lgr->type == SMC_LGR_SINGLE && lgr->max_links <= 1)
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
net/smc/smc_llc.c:1175:34: note: place parentheses around the '&&' expression to silence this warning
1175 | lgr->type == SMC_LGR_SINGLE && lgr->max_links <= 1)
| ^
| ( )
1 warning generated.
vim +1175 net/smc/smc_llc.c
1160
1161 /* as an SMC client, invite server to start the add_link processing */
1162 static void smc_llc_cli_add_link_invite(struct smc_link *link,
1163 struct smc_llc_qentry *qentry)
1164 {
1165 struct smc_link_group *lgr = smc_get_lgr(link);
1166 struct smc_init_info *ini;
1167
1168 if (lgr->smc_version == SMC_V2) {
1169 smc_llc_send_request_add_link(link);
1170 goto free_qentry;
1171 }
1172
1173 if (lgr->type == SMC_LGR_SYMMETRIC ||
1174 lgr->type == SMC_LGR_ASYMMETRIC_PEER ||
> 1175 lgr->type == SMC_LGR_SINGLE && lgr->max_links <= 1)
1176 goto free_qentry;
1177
1178 ini = kzalloc(sizeof(*ini), GFP_KERNEL);
1179 if (!ini)
1180 goto free_qentry;
1181
1182 ini->vlan_id = lgr->vlan_id;
1183 smc_pnet_find_alt_roce(lgr, ini, link->smcibdev);
1184 if (!ini->ib_dev)
1185 goto out;
1186
1187 smc_llc_send_add_link(link, ini->ib_dev->mac[ini->ib_port - 1],
1188 ini->ib_gid, NULL, SMC_LLC_REQ);
1189 out:
1190 kfree(ini);
1191 free_qentry:
1192 kfree(qentry);
1193 }
1194
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] net/smc: Improve exception handling in smc_llc_cli_add_link_invite()
2023-12-31 15:00 ` [PATCH 2/2] net/smc: Improve exception handling in smc_llc_cli_add_link_invite() Markus Elfring
2023-12-31 22:09 ` kernel test robot
@ 2023-12-31 22:40 ` kernel test robot
2024-01-02 2:19 ` Tony Lu
2 siblings, 0 replies; 15+ messages in thread
From: kernel test robot @ 2023-12-31 22:40 UTC (permalink / raw)
To: Markus Elfring, linux-s390, netdev, kernel-janitors,
David S. Miller, D. Wythe, Eric Dumazet, Jakub Kicinski,
Jan Karcher, Paolo Abeni, Tony Lu, Wen Gu, Wenjia Zhang
Cc: oe-kbuild-all, LKML
Hi Markus,
kernel test robot noticed the following build warnings:
[auto build test WARNING on net/main]
[also build test WARNING on net-next/main linus/master v6.7-rc7 next-20231222]
[cannot apply to horms-ipvs/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Markus-Elfring/net-smc-Return-directly-after-a-failed-kzalloc-in-smc_fill_gid_list/20231231-231406
base: net/main
patch link: https://lore.kernel.org/r/5253e660-6b66-4775-ae2f-06f5a1d40be5%40web.de
patch subject: [PATCH 2/2] net/smc: Improve exception handling in smc_llc_cli_add_link_invite()
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20240101/202401010657.eexbMD1F-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240101/202401010657.eexbMD1F-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401010657.eexbMD1F-lkp@intel.com/
All warnings (new ones prefixed by >>):
net/smc/smc_llc.c: In function 'smc_llc_cli_add_link_invite':
>> net/smc/smc_llc.c:1175:41: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
1175 | lgr->type == SMC_LGR_SINGLE && lgr->max_links <= 1)
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
vim +1175 net/smc/smc_llc.c
1160
1161 /* as an SMC client, invite server to start the add_link processing */
1162 static void smc_llc_cli_add_link_invite(struct smc_link *link,
1163 struct smc_llc_qentry *qentry)
1164 {
1165 struct smc_link_group *lgr = smc_get_lgr(link);
1166 struct smc_init_info *ini;
1167
1168 if (lgr->smc_version == SMC_V2) {
1169 smc_llc_send_request_add_link(link);
1170 goto free_qentry;
1171 }
1172
1173 if (lgr->type == SMC_LGR_SYMMETRIC ||
1174 lgr->type == SMC_LGR_ASYMMETRIC_PEER ||
> 1175 lgr->type == SMC_LGR_SINGLE && lgr->max_links <= 1)
1176 goto free_qentry;
1177
1178 ini = kzalloc(sizeof(*ini), GFP_KERNEL);
1179 if (!ini)
1180 goto free_qentry;
1181
1182 ini->vlan_id = lgr->vlan_id;
1183 smc_pnet_find_alt_roce(lgr, ini, link->smcibdev);
1184 if (!ini->ib_dev)
1185 goto out;
1186
1187 smc_llc_send_add_link(link, ini->ib_dev->mac[ini->ib_port - 1],
1188 ini->ib_gid, NULL, SMC_LLC_REQ);
1189 out:
1190 kfree(ini);
1191 free_qentry:
1192 kfree(qentry);
1193 }
1194
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] net/smc: Return directly after a failed kzalloc() in smc_fill_gid_list()
2023-12-31 14:58 ` [PATCH 1/2] net/smc: Return directly after a failed kzalloc() in smc_fill_gid_list() Markus Elfring
@ 2024-01-02 2:13 ` Tony Lu
0 siblings, 0 replies; 15+ messages in thread
From: Tony Lu @ 2024-01-02 2:13 UTC (permalink / raw)
To: Markus Elfring
Cc: linux-s390, netdev, kernel-janitors, David S. Miller, D. Wythe,
Eric Dumazet, Jakub Kicinski, Jan Karcher, Paolo Abeni, Wen Gu,
Wenjia Zhang, LKML
On Sun, Dec 31, 2023 at 03:58:15PM +0100, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 31 Dec 2023 15:15:19 +0100
>
> The kfree() function was called in one case by
> the smc_fill_gid_list() function during error handling
> even if the passed variable contained a null pointer.
> This issue was detected by using the Coccinelle software.
>
> Thus return directly after a call of the function "kzalloc" failed
> at the beginning.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
LGTM, thank you.
Also please add net tag in subject and fixes tag in commit body.
Reviewed-by: Tony Lu <tonylu@linux.alibaba.com>
> ---
> net/smc/af_smc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index 7fc2f3c6d248..a396a9977ba9 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -1180,7 +1180,7 @@ void smc_fill_gid_list(struct smc_link_group *lgr,
>
> alt_ini = kzalloc(sizeof(*alt_ini), GFP_KERNEL);
> if (!alt_ini)
> - goto out;
> + return;
>
> alt_ini->vlan_id = lgr->vlan_id;
> alt_ini->check_smcrv2 = true;
> --
> 2.43.0
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] net/smc: Improve exception handling in smc_llc_cli_add_link_invite()
2023-12-31 15:00 ` [PATCH 2/2] net/smc: Improve exception handling in smc_llc_cli_add_link_invite() Markus Elfring
2023-12-31 22:09 ` kernel test robot
2023-12-31 22:40 ` kernel test robot
@ 2024-01-02 2:19 ` Tony Lu
2 siblings, 0 replies; 15+ messages in thread
From: Tony Lu @ 2024-01-02 2:19 UTC (permalink / raw)
To: Markus Elfring
Cc: linux-s390, netdev, kernel-janitors, David S. Miller, D. Wythe,
Eric Dumazet, Jakub Kicinski, Jan Karcher, Paolo Abeni, Wen Gu,
Wenjia Zhang, LKML
On Sun, Dec 31, 2023 at 04:00:22PM +0100, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 31 Dec 2023 15:42:07 +0100
>
> The kfree() function was called in some cases by
> the smc_llc_cli_add_link_invite() function during error handling
> even if the passed variable contained a null pointer.
> This issue was detected by using the Coccinelle software.
>
> * Thus use another label.
>
> * Merge two if statements.
>
> * Omit an initialisation (for the variable "ini")
> which became unnecessary with this refactoring.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
Thank you, LGTM. Also net and Fixes tags are needed.
Reviewed-by: Tony Lu <tonylu@linux.alibaba.com>
> ---
> net/smc/smc_llc.c | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/net/smc/smc_llc.c b/net/smc/smc_llc.c
> index 018ce8133b02..2ff24a7feb26 100644
> --- a/net/smc/smc_llc.c
> +++ b/net/smc/smc_llc.c
> @@ -1163,23 +1163,21 @@ static void smc_llc_cli_add_link_invite(struct smc_link *link,
> struct smc_llc_qentry *qentry)
> {
> struct smc_link_group *lgr = smc_get_lgr(link);
> - struct smc_init_info *ini = NULL;
> + struct smc_init_info *ini;
>
> if (lgr->smc_version == SMC_V2) {
> smc_llc_send_request_add_link(link);
> - goto out;
> + goto free_qentry;
> }
>
> if (lgr->type == SMC_LGR_SYMMETRIC ||
> - lgr->type == SMC_LGR_ASYMMETRIC_PEER)
> - goto out;
> -
> - if (lgr->type == SMC_LGR_SINGLE && lgr->max_links <= 1)
> - goto out;
> + lgr->type == SMC_LGR_ASYMMETRIC_PEER ||
> + lgr->type == SMC_LGR_SINGLE && lgr->max_links <= 1)
> + goto free_qentry;
>
> ini = kzalloc(sizeof(*ini), GFP_KERNEL);
> if (!ini)
> - goto out;
> + goto free_qentry;
>
> ini->vlan_id = lgr->vlan_id;
> smc_pnet_find_alt_roce(lgr, ini, link->smcibdev);
> @@ -1190,6 +1188,7 @@ static void smc_llc_cli_add_link_invite(struct smc_link *link,
> ini->ib_gid, NULL, SMC_LLC_REQ);
> out:
> kfree(ini);
> +free_qentry:
> kfree(qentry);
> }
>
> --
> 2.43.0
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] net/smc: Adjustments for two function implementations
2023-12-31 14:55 [PATCH 0/2] net/smc: Adjustments for two function implementations Markus Elfring
2023-12-31 14:58 ` [PATCH 1/2] net/smc: Return directly after a failed kzalloc() in smc_fill_gid_list() Markus Elfring
2023-12-31 15:00 ` [PATCH 2/2] net/smc: Improve exception handling in smc_llc_cli_add_link_invite() Markus Elfring
@ 2024-01-02 8:13 ` Wen Gu
2024-01-02 8:44 ` Tony Lu
2024-01-02 8:51 ` [0/2] " Markus Elfring
2024-01-03 13:44 ` [PATCH 0/2] " Wenjia Zhang
3 siblings, 2 replies; 15+ messages in thread
From: Wen Gu @ 2024-01-02 8:13 UTC (permalink / raw)
To: Markus Elfring, linux-s390, netdev, kernel-janitors,
David S. Miller, D. Wythe, Eric Dumazet, Jakub Kicinski,
Jan Karcher, Paolo Abeni, Tony Lu, Wenjia Zhang
On 2023/12/31 22:55, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 31 Dec 2023 15:48:45 +0100
>
> A few update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (2):
> Return directly after a failed kzalloc() in smc_fill_gid_list()
> Improve exception handling in smc_llc_cli_add_link_invite()
>
> net/smc/af_smc.c | 2 +-
> net/smc/smc_llc.c | 15 +++++++--------
> 2 files changed, 8 insertions(+), 9 deletions(-)
>
> --
> 2.43.0
Hi Markus. I see you want to fix the kfree(NULL) issues in these two patches.
But I am wondering if this is necessary, since kfree() can handle NULL correctly.
/**
* kfree - free previously allocated memory
* @object: pointer returned by kmalloc() or kmem_cache_alloc()
*
* If @object is NULL, no operation is performed.
*/
void kfree(const void *object)
{
struct folio *folio;
struct slab *slab;
struct kmem_cache *s;
trace_kfree(_RET_IP_, object);
if (unlikely(ZERO_OR_NULL_PTR(object)))
return;
folio = virt_to_folio(object);
if (unlikely(!folio_test_slab(folio))) {
free_large_kmalloc(folio, (void *)object);
return;
}
slab = folio_slab(folio);
s = slab->slab_cache;
__kmem_cache_free(s, (void *)object, _RET_IP_);
}
EXPORT_SYMBOL(kfree);
Thanks,
Wen Gu
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] net/smc: Adjustments for two function implementations
2024-01-02 8:13 ` [PATCH 0/2] net/smc: Adjustments for two function implementations Wen Gu
@ 2024-01-02 8:44 ` Tony Lu
2024-01-02 8:51 ` [0/2] " Markus Elfring
1 sibling, 0 replies; 15+ messages in thread
From: Tony Lu @ 2024-01-02 8:44 UTC (permalink / raw)
To: Wen Gu
Cc: Markus Elfring, linux-s390, netdev, kernel-janitors,
David S. Miller, D. Wythe, Eric Dumazet, Jakub Kicinski,
Jan Karcher, Paolo Abeni, Wenjia Zhang
On Tue, Jan 02, 2024 at 04:13:09PM +0800, Wen Gu wrote:
>
>
> On 2023/12/31 22:55, Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Sun, 31 Dec 2023 15:48:45 +0100
> >
> > A few update suggestions were taken into account
> > from static source code analysis.
> >
> > Markus Elfring (2):
> > Return directly after a failed kzalloc() in smc_fill_gid_list()
> > Improve exception handling in smc_llc_cli_add_link_invite()
> >
> > net/smc/af_smc.c | 2 +-
> > net/smc/smc_llc.c | 15 +++++++--------
> > 2 files changed, 8 insertions(+), 9 deletions(-)
> >
> > --
> > 2.43.0
>
> Hi Markus. I see you want to fix the kfree(NULL) issues in these two patches.
>
> But I am wondering if this is necessary, since kfree() can handle NULL correctly.
I think the key point is that there are no necessary to call kfree() if
we can avoid this in normal logic.
Tony Lu
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [0/2] net/smc: Adjustments for two function implementations
2024-01-02 8:13 ` [PATCH 0/2] net/smc: Adjustments for two function implementations Wen Gu
2024-01-02 8:44 ` Tony Lu
@ 2024-01-02 8:51 ` Markus Elfring
2024-01-02 11:33 ` Wen Gu
1 sibling, 1 reply; 15+ messages in thread
From: Markus Elfring @ 2024-01-02 8:51 UTC (permalink / raw)
To: Wen Gu, linux-s390, netdev, kernel-janitors, David S. Miller,
D. Wythe, Eric Dumazet, Jakub Kicinski, Jan Karcher, Paolo Abeni,
Tony Lu, Wenjia Zhang
Cc: LKML
…
>> A few update suggestions were taken into account
>> from static source code analysis.
…
>> Return directly after a failed kzalloc() in smc_fill_gid_list()
>> Improve exception handling in smc_llc_cli_add_link_invite()
>>
>> net/smc/af_smc.c | 2 +-
>> net/smc/smc_llc.c | 15 +++++++--------
>> 2 files changed, 8 insertions(+), 9 deletions(-)
…
> I see you want to fix the kfree(NULL) issues in these two patches.
I propose to avoid redundant function calls at various source code places.
> But I am wondering if this is necessary, since kfree() can handle NULL correctly.
Would you prefer only required data processing in affected function implementations?
Regards,
Markus
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [0/2] net/smc: Adjustments for two function implementations
2024-01-02 8:51 ` [0/2] " Markus Elfring
@ 2024-01-02 11:33 ` Wen Gu
2024-01-02 11:50 ` Markus Elfring
2024-01-04 20:40 ` Simon Horman
0 siblings, 2 replies; 15+ messages in thread
From: Wen Gu @ 2024-01-02 11:33 UTC (permalink / raw)
To: Markus Elfring, linux-s390, netdev, kernel-janitors,
David S. Miller, D. Wythe, Eric Dumazet, Jakub Kicinski,
Jan Karcher, Paolo Abeni, Tony Lu, Wenjia Zhang
Cc: LKML
On 2024/1/2 16:51, Markus Elfring wrote:
> …
>>> A few update suggestions were taken into account
>>> from static source code analysis.
> …
>>> Return directly after a failed kzalloc() in smc_fill_gid_list()
>>> Improve exception handling in smc_llc_cli_add_link_invite()
>>>
>>> net/smc/af_smc.c | 2 +-
>>> net/smc/smc_llc.c | 15 +++++++--------
>>> 2 files changed, 8 insertions(+), 9 deletions(-)
> …
>> I see you want to fix the kfree(NULL) issues in these two patches.
>
> I propose to avoid redundant function calls at various source code places.
>
>
>> But I am wondering if this is necessary, since kfree() can handle NULL correctly.
>
> Would you prefer only required data processing in affected function implementations?
>
Thank you Markus. I understood that you want to avoid redundant function calls.
But it is not very attractive to me since the calls occur on low-frequency paths
or unlikely condition, resulting in limited performance loss and the current
kfree() usage is fine and common. So what is the benfit?
I noticed that some other discussions are on-going. It seems like you are trying
to change other similiar places. Let's collect more opinions.
https://lore.kernel.org/netdev/828bb442-29d0-4bb8-b90d-f200bdd4faf6@web.de/
https://lore.kernel.org/netdev/90679f69-951c-47b3-b86f-75fd9fde3da3@web.de/
https://lore.kernel.org/netdev/dc0a1c9d-ceca-473d-9ad5-89b59e6af2e7@web.de/
https://lore.kernel.org/netdev/cde82080-c715-473c-97ac-6ef66bba6d64@web.de/
Thanks.
> Regards,
> Markus
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [0/2] net/smc: Adjustments for two function implementations
2024-01-02 11:33 ` Wen Gu
@ 2024-01-02 11:50 ` Markus Elfring
2024-01-04 20:40 ` Simon Horman
1 sibling, 0 replies; 15+ messages in thread
From: Markus Elfring @ 2024-01-02 11:50 UTC (permalink / raw)
To: Wen Gu, linux-s390, netdev, kernel-janitors, David S. Miller,
D. Wythe, Eric Dumazet, Jakub Kicinski, Jan Karcher, Paolo Abeni,
Tony Lu, Wenjia Zhang
Cc: LKML
> But it is not very attractive to me since the calls occur on low-frequency paths
> or unlikely condition, resulting in limited performance loss and the current
> kfree() usage is fine and common.
The prioritisation of development activities influences progress in related areas.
> So what is the benfit?
* Source code clarity
* Nicer run time characteristics
See also:
https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources
> I noticed that some other discussions are on-going. It seems like you are trying
> to change other similiar places.
I would appreciate if improvements can be achieved also for similarly affected
software components.
Regards,
Markus
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] net/smc: Adjustments for two function implementations
2023-12-31 14:55 [PATCH 0/2] net/smc: Adjustments for two function implementations Markus Elfring
` (2 preceding siblings ...)
2024-01-02 8:13 ` [PATCH 0/2] net/smc: Adjustments for two function implementations Wen Gu
@ 2024-01-03 13:44 ` Wenjia Zhang
2024-01-03 14:16 ` [0/2] " Markus Elfring
3 siblings, 1 reply; 15+ messages in thread
From: Wenjia Zhang @ 2024-01-03 13:44 UTC (permalink / raw)
To: Markus Elfring, linux-s390, netdev, kernel-janitors,
David S. Miller, D. Wythe, Eric Dumazet, Jakub Kicinski,
Jan Karcher, Paolo Abeni, Tony Lu, Wen Gu
On 31.12.23 15:55, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 31 Dec 2023 15:48:45 +0100
>
> A few update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (2):
> Return directly after a failed kzalloc() in smc_fill_gid_list()
> Improve exception handling in smc_llc_cli_add_link_invite()
>
> net/smc/af_smc.c | 2 +-
> net/smc/smc_llc.c | 15 +++++++--------
> 2 files changed, 8 insertions(+), 9 deletions(-)
>
> --
> 2.43.0
>
Hi Markus,
Thank you for trying to improve our code!
However, I'm on the same page with Wen Gu. I could not see the necessity
of the patches.
BTW, if you want to send fix patches, please provide the error messages
you met, the procedure of reproducing the issue and the correspoinding
commit messages. If you want to send feature patches, I'd like to see a
well thought-out patch or patch series. E.g. In our component, the
kfree(NULL) issue doesn't only occur in the positions where you
mentioned in the patch series, also somewhere else. I would be grateful
if all of them would be cleaned up, not just some pieces.
Thanks,
Wenjia
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [0/2] net/smc: Adjustments for two function implementations
2024-01-03 13:44 ` [PATCH 0/2] " Wenjia Zhang
@ 2024-01-03 14:16 ` Markus Elfring
0 siblings, 0 replies; 15+ messages in thread
From: Markus Elfring @ 2024-01-03 14:16 UTC (permalink / raw)
To: Wenjia Zhang, linux-s390, netdev, kernel-janitors,
David S. Miller, D. Wythe, Eric Dumazet, Jakub Kicinski,
Jan Karcher, Paolo Abeni, Tony Lu, Wen Gu
Cc: LKML
…
>> Return directly after a failed kzalloc() in smc_fill_gid_list()
>> Improve exception handling in smc_llc_cli_add_link_invite()
>>
>> net/smc/af_smc.c | 2 +-
>> net/smc/smc_llc.c | 15 +++++++--------
>> 2 files changed, 8 insertions(+), 9 deletions(-)
…
> However, I'm on the same page with Wen Gu. I could not see the necessity of the patches.
> BTW, if you want to send fix patches,
I obviously propose to adjust specific implementation details.
> please provide the error messages you met,
This development concern does not apply here.
> the procedure of reproducing the issue and the correspoinding commit messages.
Would you like to extend the usage of source code analysis tools?
> If you want to send feature patches, I'd like to see a well thought-out patch or patch series.
I presented some thoughts for special transformation patterns
on several software components.
> E.g. In our component, the kfree(NULL) issue doesn't only occur in the positions where you mentioned in the patch series, also somewhere else.
Does your feedback indicate that you would support the avoidance of such a special function call
at more places?
> I would be grateful if all of them would be cleaned up, not just some pieces.
Do you find my patch series too small for the mentioned Linux module at the moment?
Regards,
Markus
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [0/2] net/smc: Adjustments for two function implementations
2024-01-02 11:33 ` Wen Gu
2024-01-02 11:50 ` Markus Elfring
@ 2024-01-04 20:40 ` Simon Horman
1 sibling, 0 replies; 15+ messages in thread
From: Simon Horman @ 2024-01-04 20:40 UTC (permalink / raw)
To: Wen Gu
Cc: Markus Elfring, linux-s390, netdev, kernel-janitors,
David S. Miller, D. Wythe, Eric Dumazet, Jakub Kicinski,
Jan Karcher, Paolo Abeni, Tony Lu, Wenjia Zhang, LKML
On Tue, Jan 02, 2024 at 07:33:18PM +0800, Wen Gu wrote:
>
>
> On 2024/1/2 16:51, Markus Elfring wrote:
> > …
> > > > A few update suggestions were taken into account
> > > > from static source code analysis.
> > …
> > > > Return directly after a failed kzalloc() in smc_fill_gid_list()
> > > > Improve exception handling in smc_llc_cli_add_link_invite()
> > > >
> > > > net/smc/af_smc.c | 2 +-
> > > > net/smc/smc_llc.c | 15 +++++++--------
> > > > 2 files changed, 8 insertions(+), 9 deletions(-)
> > …
> > > I see you want to fix the kfree(NULL) issues in these two patches.
> >
> > I propose to avoid redundant function calls at various source code places.
> >
> >
> > > But I am wondering if this is necessary, since kfree() can handle NULL correctly.
> >
> > Would you prefer only required data processing in affected function implementations?
> >
>
> Thank you Markus. I understood that you want to avoid redundant function calls.
>
> But it is not very attractive to me since the calls occur on low-frequency paths
> or unlikely condition, resulting in limited performance loss and the current
> kfree() usage is fine and common. So what is the benfit?
>
> I noticed that some other discussions are on-going. It seems like you are trying
> to change other similiar places. Let's collect more opinions.
>
> https://lore.kernel.org/netdev/828bb442-29d0-4bb8-b90d-f200bdd4faf6@web.de/
> https://lore.kernel.org/netdev/90679f69-951c-47b3-b86f-75fd9fde3da3@web.de/
> https://lore.kernel.org/netdev/dc0a1c9d-ceca-473d-9ad5-89b59e6af2e7@web.de/
> https://lore.kernel.org/netdev/cde82080-c715-473c-97ac-6ef66bba6d64@web.de/
As as been explained to Markus many times recently,
calling kfree(NULL) is not only perfectly fine,
it is the preferred way of handling things.
Markus, please stop posting patches of this nature to Netdev.
--
pw-bot: rejected
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2024-01-04 20:40 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-31 14:55 [PATCH 0/2] net/smc: Adjustments for two function implementations Markus Elfring
2023-12-31 14:58 ` [PATCH 1/2] net/smc: Return directly after a failed kzalloc() in smc_fill_gid_list() Markus Elfring
2024-01-02 2:13 ` Tony Lu
2023-12-31 15:00 ` [PATCH 2/2] net/smc: Improve exception handling in smc_llc_cli_add_link_invite() Markus Elfring
2023-12-31 22:09 ` kernel test robot
2023-12-31 22:40 ` kernel test robot
2024-01-02 2:19 ` Tony Lu
2024-01-02 8:13 ` [PATCH 0/2] net/smc: Adjustments for two function implementations Wen Gu
2024-01-02 8:44 ` Tony Lu
2024-01-02 8:51 ` [0/2] " Markus Elfring
2024-01-02 11:33 ` Wen Gu
2024-01-02 11:50 ` Markus Elfring
2024-01-04 20:40 ` Simon Horman
2024-01-03 13:44 ` [PATCH 0/2] " Wenjia Zhang
2024-01-03 14:16 ` [0/2] " Markus Elfring
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).