| Message ID | 1640677770-112053-1-git-send-email-guwen@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net] net/smc: Reset conn->lgr when link group registration fails | expand |
On 28/12/2021 08:49, Wen Gu wrote: > SMC connections might fail to be registered to a link group due to > things like unable to find a link to assign to in its creation. As > a result, connection creation will return a failure and most > resources related to the connection won't be applied or initialized, > such as conn->abort_work or conn->lnk. I agree with your fix to set conn->lgr to NULL when smc_lgr_register_conn() fails. It would probably be better to have smc_lgr_register_conn() set conn->lgr instead to set it before in smc_conn_create(). So it would not be set at all then the registration failes. What I do not understand is the extra step after the new label out_unreg: that may invoke smc_lgr_schedule_free_work(). You did not talk about that one. Is the idea to have a new link group get freed() when a connection could not be registered on it? In that case I would expect this code after label create: in smc_lgr_create(), when the rc from smc_lgr_register_conn() is not zero. Thoughts?
Thanks for your reply. On 2021/12/29 9:07 pm, Karsten Graul wrote: > On 28/12/2021 08:49, Wen Gu wrote: >> SMC connections might fail to be registered to a link group due to >> things like unable to find a link to assign to in its creation. As >> a result, connection creation will return a failure and most >> resources related to the connection won't be applied or initialized, >> such as conn->abort_work or conn->lnk. > > I agree with your fix to set conn->lgr to NULL when smc_lgr_register_conn() fails. > > It would probably be better to have smc_lgr_register_conn() set conn->lgr instead to set > it before in smc_conn_create(). So it would not be set at all then the registration failes. > I agree and will improve it, thanks. > > What I do not understand is the extra step after the new label out_unreg: that > may invoke smc_lgr_schedule_free_work(). You did not talk about that one. > Is the idea to have a new link group get freed() when a connection could not > be registered on it? I noticed that smc_conn_create() may be invoked by smc_listen_work(rdma/ism) and __smc_connect(rdma/ism). In smc_listen_work() case, if smc_conn_create() fails at smc_lgr_register_conn() and returns a not-zero rc, the conn->lgr (which won't be reset in original implementation) will be freed through smc_listen_decline()->smc_conn_abort()-> smc_conn_free()->smc_lgr_schedule_free_work(). So I invoke smc_lgr_schedule_free_work() in label 'out_unreg:' to be consistent with the above behavior because the conn->lgr is reset to NULL in my implementation, thus smc_lgr_schedule_free_work() won't be invoked in smc_conn_free(). In __smc_connect() case, I noticed that the behavior of __smc_connect() is not symmetric with smc_listen_work()'s. If smc_conn_create() fails at smc_lgr_register_conn() __smc_connect() will not try to free conn->lgr as what did in smc_listen_work(). I am a bit puzzled about it and want to hear your opinions. In my humble opinion, it also should try to free link group in __smc_connect() case, so I invoke smc_lgr_schedule_free_work() in label 'out_unreg:'. > In that case I would expect this code after label create: > in smc_lgr_create(), when the rc from smc_lgr_register_conn() is not zero. > Thoughts? Maybe we should try to free the link group when the registration fails, no matter it is new created or already existing? If so, is it better to do it in the same place like label 'out_unreg'? Cheers, Wen Gu
On 30/12/2021 04:50, Wen Gu wrote: > Thanks for your reply. > > On 2021/12/29 9:07 pm, Karsten Graul wrote: >> On 28/12/2021 08:49, Wen Gu wrote: >>> SMC connections might fail to be registered to a link group due to >>> things like unable to find a link to assign to in its creation. As >>> a result, connection creation will return a failure and most >>> resources related to the connection won't be applied or initialized, >>> such as conn->abort_work or conn->lnk. >> What I do not understand is the extra step after the new label out_unreg: that >> may invoke smc_lgr_schedule_free_work(). You did not talk about that one. >> Is the idea to have a new link group get freed() when a connection could not >> be registered on it? > Maybe we should try to free the link group when the registration fails, no matter > it is new created or already existing? If so, is it better to do it in the same > place like label 'out_unreg'? I agree with your idea. With the proposed change that conn->lgr gets not even set when the registration fails we would not need the "conn->lgr = NULL;" after label out_unreg? And as far as I understand the invocation of smc_lgr_schedule_free_work(lgr) is only needed after label "create", because when an existing link group was found and the registration failed then its free work would already be started when no more connections are assigned to the link group, right?
Thanks for your reply. On 2022/1/3 6:52 pm, Karsten Graul wrote: > On 30/12/2021 04:50, Wen Gu wrote: >> Thanks for your reply. >> >> On 2021/12/29 9:07 pm, Karsten Graul wrote: >>> On 28/12/2021 08:49, Wen Gu wrote: >>>> SMC connections might fail to be registered to a link group due to >>>> things like unable to find a link to assign to in its creation. As >>>> a result, connection creation will return a failure and most >>>> resources related to the connection won't be applied or initialized, >>>> such as conn->abort_work or conn->lnk. >>> What I do not understand is the extra step after the new label out_unreg: that >>> may invoke smc_lgr_schedule_free_work(). You did not talk about that one. >>> Is the idea to have a new link group get freed() when a connection could not >>> be registered on it? >> Maybe we should try to free the link group when the registration fails, no matter >> it is new created or already existing? If so, is it better to do it in the same >> place like label 'out_unreg'? > > I agree with your idea. > > With the proposed change that conn->lgr gets not even set when the registration fails > we would not need the "conn->lgr = NULL;" after label out_unreg? Yes, conn->lgr now will be reset in smc_lgr_register_conn() if registration fails. > > And as far as I understand the invocation of smc_lgr_schedule_free_work(lgr) is only > needed after label "create", because when an existing link group was found and the registration > failed then its free work would already be started when no more connections are assigned > to the link group, right? Thanks for your explanation. I also agree with only invoking smc_lgr_schedule_free_work(lgr) after label "create" now. I will improve it and send a v2 patch. Thanks, Wen Gu
diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c index 412bc85..1f40b8e 100644 --- a/net/smc/smc_core.c +++ b/net/smc/smc_core.c @@ -1815,7 +1815,7 @@ int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini) } spin_unlock_bh(lgr_lock); if (rc) - return rc; + goto out_unreg; if (role == SMC_CLNT && !ini->first_contact_peer && ini->first_contact_local) { @@ -1836,7 +1836,7 @@ int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini) rc = smc_lgr_register_conn(conn, true); write_unlock_bh(&lgr->conns_lock); if (rc) - goto out; + goto out_unreg; } conn->local_tx_ctrl.common.type = SMC_CDC_MSG_TYPE; conn->local_tx_ctrl.len = SMC_WR_TX_SIZE; @@ -1855,6 +1855,12 @@ int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini) out: return rc; +out_unreg: + /* fail to register connection into a link group */ + if (!lgr->conns_num && !delayed_work_pending(&lgr->free_work)) + smc_lgr_schedule_free_work(lgr); + conn->lgr = NULL; + return rc; } #define SMCD_DMBE_SIZES 6 /* 0 -> 16KB, 1 -> 32KB, .. 6 -> 1MB */
SMC connections might fail to be registered to a link group due to things like unable to find a link to assign to in its creation. As a result, connection creation will return a failure and most resources related to the connection won't be applied or initialized, such as conn->abort_work or conn->lnk. If smc_conn_free() is invoked later, it will try to access the resources related to the connection, which wasn't initialized, thus causing a panic. Here is an example, a SMC-R connection failed to be registered to a link group and conn->lnk is NULL. The following crash will happen if smc_conn_free() tries to access conn->lnk in smc_cdc_tx_dismiss_slots(). BUG: kernel NULL pointer dereference, address: 0000000000000168 #PF: supervisor read access in kernel mode #PF: error_code(0x0000) - not-present page PGD 0 P4D 0 Oops: 0000 [#1] PREEMPT SMP PTI CPU: 4 PID: 68 Comm: kworker/4:1 Kdump: loaded Tainted: G E 5.16.0-rc5+ #52 Workqueue: smc_hs_wq smc_listen_work [smc] RIP: 0010:smc_wr_tx_dismiss_slots+0x1e/0xc0 [smc] Call Trace: <TASK> smc_conn_free+0xd8/0x100 [smc] smc_lgr_cleanup_early+0x15/0x90 [smc] smc_listen_work+0x302/0x1230 [smc] ? process_one_work+0x25c/0x600 process_one_work+0x25c/0x600 worker_thread+0x4f/0x3a0 ? process_one_work+0x600/0x600 kthread+0x15d/0x1a0 ? set_kthread_struct+0x40/0x40 ret_from_fork+0x1f/0x30 </TASK> This patch tries to fix this by resetting conn->lgr to NULL if an abnormal exit due to lgr register failure occurs in smc_conn_create(), thus avoiding the crash caused by accessing the uninitialized resources in smc_conn_free(). Signed-off-by: Wen Gu <guwen@linux.alibaba.com> --- net/smc/smc_core.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)