[bug report] RDMA/devices: Re-organize device.c locking

[bug report] RDMA/devices: Re-organize device.c locking

[Dan Carpenter]

Hello Jason Gunthorpe,

The patch 921eab1143aa: "RDMA/devices: Re-organize device.c locking"
from Feb 6, 2019, leads to the following static checker warning:

	drivers/infiniband/core/device.c:712 add_client_context()
	warn: missing error code 'ret'

drivers/infiniband/core/device.c
    689 static int add_client_context(struct ib_device *device,
    690 			      struct ib_client *client)
    691 {
    692 	int ret = 0;
    693 
    694 	if (!device->kverbs_provider && !client->no_kverbs_req)
    695 		return 0;
    696 
    697 	down_write(&device->client_data_rwsem);
    698 	/*
    699 	 * So long as the client is registered hold both the client and device
    700 	 * unregistration locks.
    701 	 */
    702 	if (!refcount_inc_not_zero(&client->uses))
    703 		goto out_unlock;
    704 	refcount_inc(&device->refcount);
    705 
    706 	/*
    707 	 * Another caller to add_client_context got here first and has already
    708 	 * completely initialized context.
    709 	 */
    710 	if (xa_get_mark(&device->client_data, client->client_id,
    711 		    CLIENT_DATA_REGISTERED))
--> 712 		goto out;

Hard to tell if ret should be zero or negative.

    713 
    714 	ret = xa_err(xa_store(&device->client_data, client->client_id, NULL,
    715 			      GFP_KERNEL));
    716 	if (ret)
    717 		goto out;
    718 	downgrade_write(&device->client_data_rwsem);
    719 	if (client->add) {
    720 		if (client->add(device)) {
    721 			/*
    722 			 * If a client fails to add then the error code is
    723 			 * ignored, but we won't call any more ops on this
    724 			 * client.
    725 			 */
    726 			xa_erase(&device->client_data, client->client_id);
    727 			up_read(&device->client_data_rwsem);
    728 			ib_device_put(device);
    729 			ib_client_put(client);
    730 			return 0;
    731 		}
    732 	}
    733 
    734 	/* Readers shall not see a client until add has been completed */
    735 	xa_set_mark(&device->client_data, client->client_id,
    736 		    CLIENT_DATA_REGISTERED);
    737 	up_read(&device->client_data_rwsem);
    738 	return 0;
    739 
    740 out:
    741 	ib_device_put(device);
    742 	ib_client_put(client);
    743 out_unlock:
    744 	up_write(&device->client_data_rwsem);
    745 	return ret;
    746 }

regards,
dan carpenter

Re: [bug report] RDMA/devices: Re-organize device.c locking

[Jason Gunthorpe]

On Tue, Aug 10, 2021 at 12:18:42PM +0300, Dan Carpenter wrote:
> Hello Jason Gunthorpe,
> 
> The patch 921eab1143aa: "RDMA/devices: Re-organize device.c locking"
> from Feb 6, 2019, leads to the following static checker warning:
> 
> 	drivers/infiniband/core/device.c:712 add_client_context()
> 	warn: missing error code 'ret'
> 
> drivers/infiniband/core/device.c
>     689 static int add_client_context(struct ib_device *device,
>     690 			      struct ib_client *client)
>     691 {
>     692 	int ret = 0;
>     693 
>     694 	if (!device->kverbs_provider && !client->no_kverbs_req)
>     695 		return 0;
>     696 
>     697 	down_write(&device->client_data_rwsem);
>     698 	/*
>     699 	 * So long as the client is registered hold both the client and device
>     700 	 * unregistration locks.
>     701 	 */
>     702 	if (!refcount_inc_not_zero(&client->uses))
>     703 		goto out_unlock;
>     704 	refcount_inc(&device->refcount);
>     705 
>     706 	/*
>     707 	 * Another caller to add_client_context got here first and has already
>     708 	 * completely initialized context.
>     709 	 */
>     710 	if (xa_get_mark(&device->client_data, client->client_id,
>     711 		    CLIENT_DATA_REGISTERED))
> --> 712 		goto out;
> 
> Hard to tell if ret should be zero or negative.

It should be 0, this collision is success, the xarray has the correct
data, it was just put there by another thread.

Jason