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
