* [bug report] IB/cm: Improve the calling of cm_init_av_for_lap and cm_init_av_by_path
@ 2022-01-18 9:16 Dan Carpenter
2022-01-18 9:52 ` Leon Romanovsky
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2022-01-18 9:16 UTC (permalink / raw)
To: markzhang; +Cc: linux-rdma
Hello Mark Zhang,
The patch 7345201c3963: "IB/cm: Improve the calling of
cm_init_av_for_lap and cm_init_av_by_path" from Jun 2, 2021, leads to
the following Smatch static checker warning:
drivers/infiniband/core/cm.c:3373 cm_lap_handler() warn: inconsistent refcounting 'cm_id_priv->refcount.refs.counter':
inc on: 3325
dec on: 3373
drivers/infiniband/core/cm.c
3278 static int cm_lap_handler(struct cm_work *work)
3279 {
3280 struct cm_id_private *cm_id_priv;
3281 struct cm_lap_msg *lap_msg;
3282 struct ib_cm_lap_event_param *param;
3283 struct ib_mad_send_buf *msg = NULL;
3284 struct rdma_ah_attr ah_attr;
3285 struct cm_av alt_av = {};
3286 int ret;
3287
3288 /* Currently Alternate path messages are not supported for
3289 * RoCE link layer.
3290 */
3291 if (rdma_protocol_roce(work->port->cm_dev->ib_device,
3292 work->port->port_num))
3293 return -EINVAL;
3294
3295 /* todo: verify LAP request and send reject APR if invalid. */
3296 lap_msg = (struct cm_lap_msg *)work->mad_recv_wc->recv_buf.mad;
3297 cm_id_priv = cm_acquire_id(
3298 cpu_to_be32(IBA_GET(CM_LAP_REMOTE_COMM_ID, lap_msg)),
3299 cpu_to_be32(IBA_GET(CM_LAP_LOCAL_COMM_ID, lap_msg)));
cm_acquire_id() bumps the refcount.
3300 if (!cm_id_priv)
3301 return -EINVAL;
3302
3303 param = &work->cm_event.param.lap_rcvd;
3304 memset(&work->path[0], 0, sizeof(work->path[1]));
3305 cm_path_set_rec_type(work->port->cm_dev->ib_device,
3306 work->port->port_num, &work->path[0],
3307 IBA_GET_MEM_PTR(CM_LAP_ALTERNATE_LOCAL_PORT_GID,
3308 lap_msg));
3309 param->alternate_path = &work->path[0];
3310 cm_format_path_from_lap(cm_id_priv, param->alternate_path, lap_msg);
3311 work->cm_event.private_data =
3312 IBA_GET_MEM_PTR(CM_LAP_PRIVATE_DATA, lap_msg);
3313
3314 ret = ib_init_ah_attr_from_wc(work->port->cm_dev->ib_device,
3315 work->port->port_num,
3316 work->mad_recv_wc->wc,
3317 work->mad_recv_wc->recv_buf.grh,
3318 &ah_attr);
3319 if (ret)
3320 goto deref;
^^^^^^^^^^^
3321
3322 ret = cm_init_av_by_path(param->alternate_path, NULL, &alt_av);
3323 if (ret) {
3324 rdma_destroy_ah_attr(&ah_attr);
3325 return -EINVAL;
Should this be goto deref as well?
3326 }
3327
3328 spin_lock_irq(&cm_id_priv->lock);
3329 cm_init_av_for_lap(work->port, work->mad_recv_wc->wc,
3330 &ah_attr, &cm_id_priv->av);
3331 cm_move_av_from_path(&cm_id_priv->alt_av, &alt_av);
3332
3333 if (cm_id_priv->id.state != IB_CM_ESTABLISHED)
3334 goto unlock;
3335
3336 switch (cm_id_priv->id.lap_state) {
3337 case IB_CM_LAP_UNINIT:
3338 case IB_CM_LAP_IDLE:
3339 break;
3340 case IB_CM_MRA_LAP_SENT:
3341 atomic_long_inc(&work->port->counters[CM_RECV_DUPLICATES]
3342 [CM_LAP_COUNTER]);
3343 msg = cm_alloc_response_msg_no_ah(work->port, work->mad_recv_wc);
3344 if (IS_ERR(msg))
3345 goto unlock;
3346
3347 cm_format_mra((struct cm_mra_msg *) msg->mad, cm_id_priv,
3348 CM_MSG_RESPONSE_OTHER,
3349 cm_id_priv->service_timeout,
3350 cm_id_priv->private_data,
3351 cm_id_priv->private_data_len);
3352 spin_unlock_irq(&cm_id_priv->lock);
3353
3354 if (cm_create_response_msg_ah(work->port, work->mad_recv_wc, msg) ||
3355 ib_post_send_mad(msg, NULL))
3356 cm_free_response_msg(msg);
3357 goto deref;
3358 case IB_CM_LAP_RCVD:
3359 atomic_long_inc(&work->port->counters[CM_RECV_DUPLICATES]
3360 [CM_LAP_COUNTER]);
3361 goto unlock;
3362 default:
3363 goto unlock;
3364 }
3365
3366 cm_id_priv->id.lap_state = IB_CM_LAP_RCVD;
3367 cm_id_priv->tid = lap_msg->hdr.tid;
3368 cm_queue_work_unlock(cm_id_priv, work);
3369 return 0;
3370
3371 unlock: spin_unlock_irq(&cm_id_priv->lock);
3372 deref: cm_deref_id(cm_id_priv);
--> 3373 return -EINVAL;
3374 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [bug report] IB/cm: Improve the calling of cm_init_av_for_lap and cm_init_av_by_path
2022-01-18 9:16 [bug report] IB/cm: Improve the calling of cm_init_av_for_lap and cm_init_av_by_path Dan Carpenter
@ 2022-01-18 9:52 ` Leon Romanovsky
0 siblings, 0 replies; 2+ messages in thread
From: Leon Romanovsky @ 2022-01-18 9:52 UTC (permalink / raw)
To: Dan Carpenter; +Cc: markzhang, linux-rdma
On Tue, Jan 18, 2022 at 12:16:43PM +0300, Dan Carpenter wrote:
> Hello Mark Zhang,
>
> The patch 7345201c3963: "IB/cm: Improve the calling of
> cm_init_av_for_lap and cm_init_av_by_path" from Jun 2, 2021, leads to
> the following Smatch static checker warning:
>
> drivers/infiniband/core/cm.c:3373 cm_lap_handler() warn: inconsistent refcounting 'cm_id_priv->refcount.refs.counter':
> inc on: 3325
> dec on: 3373
>
> drivers/infiniband/core/cm.c
> 3278 static int cm_lap_handler(struct cm_work *work)
> 3279 {
> 3280 struct cm_id_private *cm_id_priv;
> 3281 struct cm_lap_msg *lap_msg;
> 3282 struct ib_cm_lap_event_param *param;
> 3283 struct ib_mad_send_buf *msg = NULL;
> 3284 struct rdma_ah_attr ah_attr;
> 3285 struct cm_av alt_av = {};
> 3286 int ret;
> 3287
> 3288 /* Currently Alternate path messages are not supported for
> 3289 * RoCE link layer.
> 3290 */
> 3291 if (rdma_protocol_roce(work->port->cm_dev->ib_device,
> 3292 work->port->port_num))
> 3293 return -EINVAL;
> 3294
> 3295 /* todo: verify LAP request and send reject APR if invalid. */
> 3296 lap_msg = (struct cm_lap_msg *)work->mad_recv_wc->recv_buf.mad;
> 3297 cm_id_priv = cm_acquire_id(
> 3298 cpu_to_be32(IBA_GET(CM_LAP_REMOTE_COMM_ID, lap_msg)),
> 3299 cpu_to_be32(IBA_GET(CM_LAP_LOCAL_COMM_ID, lap_msg)));
>
> cm_acquire_id() bumps the refcount.
>
> 3300 if (!cm_id_priv)
> 3301 return -EINVAL;
> 3302
> 3303 param = &work->cm_event.param.lap_rcvd;
> 3304 memset(&work->path[0], 0, sizeof(work->path[1]));
> 3305 cm_path_set_rec_type(work->port->cm_dev->ib_device,
> 3306 work->port->port_num, &work->path[0],
> 3307 IBA_GET_MEM_PTR(CM_LAP_ALTERNATE_LOCAL_PORT_GID,
> 3308 lap_msg));
> 3309 param->alternate_path = &work->path[0];
> 3310 cm_format_path_from_lap(cm_id_priv, param->alternate_path, lap_msg);
> 3311 work->cm_event.private_data =
> 3312 IBA_GET_MEM_PTR(CM_LAP_PRIVATE_DATA, lap_msg);
> 3313
> 3314 ret = ib_init_ah_attr_from_wc(work->port->cm_dev->ib_device,
> 3315 work->port->port_num,
> 3316 work->mad_recv_wc->wc,
> 3317 work->mad_recv_wc->recv_buf.grh,
> 3318 &ah_attr);
> 3319 if (ret)
> 3320 goto deref;
> ^^^^^^^^^^^
>
> 3321
> 3322 ret = cm_init_av_by_path(param->alternate_path, NULL, &alt_av);
> 3323 if (ret) {
> 3324 rdma_destroy_ah_attr(&ah_attr);
> 3325 return -EINVAL;
>
> Should this be goto deref as well?
Yes, it should.
Thanks
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-01-18 9:53 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-01-18 9:16 [bug report] IB/cm: Improve the calling of cm_init_av_for_lap and cm_init_av_by_path Dan Carpenter
2022-01-18 9:52 ` Leon Romanovsky
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).