| Message ID | 1678073786-110013-1-git-send-email-alibuda@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net/smc: fix NULL sndbuf_desc in smc_cdc_tx_handler() | expand |
On Mon, Mar 06, 2023 at 11:36:26AM +0800, D. Wythe wrote: > From: "D. Wythe" <alibuda@linux.alibaba.com> > > When performing a stress test on SMC-R by rmmod mlx5_ib driver > during the wrk/nginx test, we found that there is a probability > of triggering a panic while terminating all link groups. > > This issue dues to the race between smc_smcr_terminate_all() > and smc_buf_create(). > > smc_smcr_terminate_all > > smc_buf_create > /* init */ > conn->sndbuf_desc = NULL; > ... > > __smc_lgr_terminate > smc_conn_kill > smc_close_abort > smc_cdc_get_slot_and_msg_send > > __softirqentry_text_start > smc_wr_tx_process_cqe > smc_cdc_tx_handler > READ(conn->sndbuf_desc->len); > /* panic dues to NULL sndbuf_desc */ > > conn->sndbuf_desc = xxx; > > This patch tries to fix the issue by always to check the sndbuf_desc > before send any cdc msg, to make sure that no null pointer is > seen during cqe processing. > > Fixes: 0b29ec643613 ("net/smc: immediate termination for SMCR link groups") > Signed-off-by: D. Wythe <alibuda@linux.alibaba.com> LGTM. Reviewed-by: Tony Lu <tonylu@linux.alibaba.com> > --- > net/smc/smc_cdc.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c > index 53f63bf..2f0e2ee 100644 > --- a/net/smc/smc_cdc.c > +++ b/net/smc/smc_cdc.c > @@ -114,6 +114,9 @@ int smc_cdc_msg_send(struct smc_connection *conn, > union smc_host_cursor cfed; > int rc; > > + if (unlikely(!READ_ONCE(conn->sndbuf_desc))) > + return -EINVAL; > + > smc_cdc_add_pending_send(conn, pend); > > conn->tx_cdc_seq++; > -- > 1.8.3.1
On Mon, 2023-03-06 at 11:36 +0800, D. Wythe wrote: > From: "D. Wythe" <alibuda@linux.alibaba.com> > > When performing a stress test on SMC-R by rmmod mlx5_ib driver > during the wrk/nginx test, we found that there is a probability > of triggering a panic while terminating all link groups. > > This issue dues to the race between smc_smcr_terminate_all() > and smc_buf_create(). > > smc_smcr_terminate_all > > smc_buf_create > /* init */ > conn->sndbuf_desc = NULL; > ... > > __smc_lgr_terminate > smc_conn_kill > smc_close_abort > smc_cdc_get_slot_and_msg_send > > __softirqentry_text_start > smc_wr_tx_process_cqe > smc_cdc_tx_handler > READ(conn->sndbuf_desc->len); > /* panic dues to NULL sndbuf_desc */ > > conn->sndbuf_desc = xxx; > > This patch tries to fix the issue by always to check the sndbuf_desc > before send any cdc msg, to make sure that no null pointer is > seen during cqe processing. > > Fixes: 0b29ec643613 ("net/smc: immediate termination for SMCR link groups") > Signed-off-by: D. Wythe <alibuda@linux.alibaba.com> Looking at the code for __smc_buf_create it seems like you might have more issues hiding in the code. From what I can tell smc_buf_get_slot can only return a pointer or NULL but it is getting checked for being being a PTR_ERR or IS_ERR in several spots that are likely all dead code. > --- > net/smc/smc_cdc.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c > index 53f63bf..2f0e2ee 100644 > --- a/net/smc/smc_cdc.c > +++ b/net/smc/smc_cdc.c > @@ -114,6 +114,9 @@ int smc_cdc_msg_send(struct smc_connection *conn, > union smc_host_cursor cfed; > int rc; > > + if (unlikely(!READ_ONCE(conn->sndbuf_desc))) > + return -EINVAL; > + This return value doesn't seem right to me. Rather than en EINVAL should this be something like a ENOBUFS just to make it easier to debug when this issue is encountered? > smc_cdc_add_pending_send(conn, pend); > > conn->tx_cdc_seq++;
On 06.03.23 17:38, Alexander H Duyck wrote: > On Mon, 2023-03-06 at 11:36 +0800, D. Wythe wrote: >> From: "D. Wythe" <alibuda@linux.alibaba.com> >> >> When performing a stress test on SMC-R by rmmod mlx5_ib driver >> during the wrk/nginx test, we found that there is a probability >> of triggering a panic while terminating all link groups. >> >> This issue dues to the race between smc_smcr_terminate_all() >> and smc_buf_create(). >> >> smc_smcr_terminate_all >> >> smc_buf_create >> /* init */ >> conn->sndbuf_desc = NULL; >> ... >> >> __smc_lgr_terminate >> smc_conn_kill >> smc_close_abort >> smc_cdc_get_slot_and_msg_send >> >> __softirqentry_text_start >> smc_wr_tx_process_cqe >> smc_cdc_tx_handler >> READ(conn->sndbuf_desc->len); >> /* panic dues to NULL sndbuf_desc */ >> >> conn->sndbuf_desc = xxx; >> >> This patch tries to fix the issue by always to check the sndbuf_desc >> before send any cdc msg, to make sure that no null pointer is >> seen during cqe processing. >> >> Fixes: 0b29ec643613 ("net/smc: immediate termination for SMCR link groups") >> Signed-off-by: D. Wythe <alibuda@linux.alibaba.com> > > Looking at the code for __smc_buf_create it seems like you might have > more issues hiding in the code. From what I can tell smc_buf_get_slot > can only return a pointer or NULL but it is getting checked for being > being a PTR_ERR or IS_ERR in several spots that are likely all dead > code. > This smc_buf_get_slot() is used to get a reusable slot, which is originally assigned by smcr_new_buf_create() or smcd_new_buf_create() depending on the device being used. In smcr_new_buf_create()/smcd_new_buf_create(), the pointer values of the return codes are converted from integer values. >> --- >> net/smc/smc_cdc.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c >> index 53f63bf..2f0e2ee 100644 >> --- a/net/smc/smc_cdc.c >> +++ b/net/smc/smc_cdc.c >> @@ -114,6 +114,9 @@ int smc_cdc_msg_send(struct smc_connection *conn, >> union smc_host_cursor cfed; >> int rc; >> >> + if (unlikely(!READ_ONCE(conn->sndbuf_desc))) >> + return -EINVAL; >> + > > This return value doesn't seem right to me. Rather than en EINVAL > should this be something like a ENOBUFS just to make it easier to debug > when this issue is encountered? I agree. > >> smc_cdc_add_pending_send(conn, pend); >> >> conn->tx_cdc_seq++; > >
On Mon, 2023-03-06 at 22:06 +0100, Wenjia Zhang wrote: > > On 06.03.23 17:38, Alexander H Duyck wrote: > > On Mon, 2023-03-06 at 11:36 +0800, D. Wythe wrote: > > > From: "D. Wythe" <alibuda@linux.alibaba.com> > > > > > > When performing a stress test on SMC-R by rmmod mlx5_ib driver > > > during the wrk/nginx test, we found that there is a probability > > > of triggering a panic while terminating all link groups. > > > > > > This issue dues to the race between smc_smcr_terminate_all() > > > and smc_buf_create(). > > > > > > smc_smcr_terminate_all > > > > > > smc_buf_create > > > /* init */ > > > conn->sndbuf_desc = NULL; > > > ... > > > > > > __smc_lgr_terminate > > > smc_conn_kill > > > smc_close_abort > > > smc_cdc_get_slot_and_msg_send > > > > > > __softirqentry_text_start > > > smc_wr_tx_process_cqe > > > smc_cdc_tx_handler > > > READ(conn->sndbuf_desc->len); > > > /* panic dues to NULL sndbuf_desc */ > > > > > > conn->sndbuf_desc = xxx; > > > > > > This patch tries to fix the issue by always to check the sndbuf_desc > > > before send any cdc msg, to make sure that no null pointer is > > > seen during cqe processing. > > > > > > Fixes: 0b29ec643613 ("net/smc: immediate termination for SMCR link groups") > > > Signed-off-by: D. Wythe <alibuda@linux.alibaba.com> > > > > Looking at the code for __smc_buf_create it seems like you might have > > more issues hiding in the code. From what I can tell smc_buf_get_slot > > can only return a pointer or NULL but it is getting checked for being > > being a PTR_ERR or IS_ERR in several spots that are likely all dead > > code. > > > This smc_buf_get_slot() is used to get a reusable slot, which is > originally assigned by smcr_new_buf_create() or smcd_new_buf_create() > depending on the device being used. In > smcr_new_buf_create()/smcd_new_buf_create(), the pointer values of the > return codes are converted from integer values. Ah, okay that is what I was missing.
On Mon, Mar 06, 2023 at 08:38:52AM -0800, Alexander H Duyck wrote: > On Mon, 2023-03-06 at 11:36 +0800, D. Wythe wrote: > > From: "D. Wythe" <alibuda@linux.alibaba.com> > > > > When performing a stress test on SMC-R by rmmod mlx5_ib driver > > during the wrk/nginx test, we found that there is a probability > > of triggering a panic while terminating all link groups. > > > > This issue dues to the race between smc_smcr_terminate_all() > > and smc_buf_create(). > > > > smc_smcr_terminate_all > > > > smc_buf_create > > /* init */ > > conn->sndbuf_desc = NULL; > > ... > > > > __smc_lgr_terminate > > smc_conn_kill > > smc_close_abort > > smc_cdc_get_slot_and_msg_send > > > > __softirqentry_text_start > > smc_wr_tx_process_cqe > > smc_cdc_tx_handler > > READ(conn->sndbuf_desc->len); > > /* panic dues to NULL sndbuf_desc */ > > > > conn->sndbuf_desc = xxx; > > > > This patch tries to fix the issue by always to check the sndbuf_desc > > before send any cdc msg, to make sure that no null pointer is > > seen during cqe processing. > > > > Fixes: 0b29ec643613 ("net/smc: immediate termination for SMCR link groups") > > Signed-off-by: D. Wythe <alibuda@linux.alibaba.com> > > Looking at the code for __smc_buf_create it seems like you might have > more issues hiding in the code. From what I can tell smc_buf_get_slot > can only return a pointer or NULL but it is getting checked for being > being a PTR_ERR or IS_ERR in several spots that are likely all dead > code. > > > --- > > net/smc/smc_cdc.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c > > index 53f63bf..2f0e2ee 100644 > > --- a/net/smc/smc_cdc.c > > +++ b/net/smc/smc_cdc.c > > @@ -114,6 +114,9 @@ int smc_cdc_msg_send(struct smc_connection *conn, > > union smc_host_cursor cfed; > > int rc; > > > > + if (unlikely(!READ_ONCE(conn->sndbuf_desc))) > > + return -EINVAL; > > + > > This return value doesn't seem right to me. Rather than en EINVAL > should this be something like a ENOBUFS just to make it easier to debug > when this issue is encountered? > I agree with you. It is reasonable to use ENOBUFS here. Thanks. > > smc_cdc_add_pending_send(conn, pend); > > > > conn->tx_cdc_seq++; >
diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c index 53f63bf..2f0e2ee 100644 --- a/net/smc/smc_cdc.c +++ b/net/smc/smc_cdc.c @@ -114,6 +114,9 @@ int smc_cdc_msg_send(struct smc_connection *conn, union smc_host_cursor cfed; int rc; + if (unlikely(!READ_ONCE(conn->sndbuf_desc))) + return -EINVAL; + smc_cdc_add_pending_send(conn, pend); conn->tx_cdc_seq++;