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
