Re: [PATCH rdma-next v1 02/15] RDMA/core: Replace the ib_port_data hw_stats pointers with a ib_port pointer

[Jason Gunthorpe]

On Mon, Jun 07, 2021 at 12:23:23PM +0200, Greg KH wrote:
> >  static int add_port(struct ib_core_device *coredev, int port_num)
> >  {
> >  	struct ib_device *device = rdma_device_to_ibdev(&coredev->dev);
> > @@ -1171,6 +1177,8 @@ static int add_port(struct ib_core_device *coredev, int port_num)
> >  		setup_hw_stats(device, p, port_num);
> >  
> >  	list_add_tail(&p->kobj.entry, &coredev->port_list);
> > +	if (device->port_data && is_full_dev)
> > +		device->port_data[port_num].sysfs = p;
> 
> You are saving off a pointer to a reference counted structure without
> incrementing the reference count on it?  

This storage borrows another reference count, primarily because there
is no locking to read/write .sysfs. It is a fairly common idiom.

You can see it in the free path:

		port->ibdev->port_data[port->port_num].sysfs = NULL;
	kobject_put(&port->kobj);  // port == p above

Due to the lack of locks the whole external thing is arranged so that
the 3 users of .sysfs are sequenced properly around
setup_port()/destroy_port() using other external locks.

Adding more refs without also adding locking is just confusing what
the data protection model is. This is a borrowed ref and access is
only allowed when other locking is properly sequencing it with the ref
owner's manipulation of .sysfs.

Eg I would reject some code sequence like this:

	port->ibdev->port_data[port->port_num].sysfs = NULL;
	kobject_put(&port->kobj);  // one for .sysfs
	kobject_put(&port->kobj);  // one for our stack

As being pretty bogus.

Jason