lkml.org 
[lkml]   [2021]   [Jun]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2021-06-07 14:33    [W:2.104 / U:0.204 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site