Messages in this thread | | | Date | Mon, 7 Jun 2021 09:32:10 -0300 | From | Jason Gunthorpe <> | Subject | Re: [PATCH rdma-next v1 02/15] RDMA/core: Replace the ib_port_data hw_stats pointers with a ib_port pointer |
| |
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
| |