On Mon, Jan 21, 2019 at 01:30:17PM +0200, Gal Pressman wrote: > On 21-Jan-19 04:15, Jason Gunthorpe wrote: > > On Sun, Jan 20, 2019 at 11:05:12AM +0200, Gal Pressman wrote: > >> On 18-Jan-19 23:22, Jason Gunthorpe wrote: > >>>>>> @@ -920,6 +926,9 @@ int ib_query_port(struct ib_device *device, > >>>>>> union ib_gid gid; > >>>>>> int err; > >>>>>> > >>>>>> + if (!device->ops.query_port) > >>>>>> + return -EOPNOTSUPP; > >>>>> > >>>>> Again, if query_port is not supported then the related sysfs file > >>>>> should not even be created. > >>>> > >>>> Needed for the cache setup as part of ib_register_device. > >>> > >>> Hum, what does the gid cache even do for !kverbs? uverbs uses it I > >>> suppose, but it won't work right at all if kverbs are not present.. > >>> > >>> That probably needs a similar fixing to sysfs, to safely disable the > >>> functionality... > >> > >> Why is the GID cache table dependent on kverbs? EFA for example uses it when > >> querying the GID sysfs. > > > > It used to be mostly used by kverbs only, but I guess we now have some > > other stuff with the various new lifetime things and what not. > > > >> Sure, it can bypass the cache and query the driver, but it's > >> probably better to use the same flow for both kverbs and non-kverbs. > > > > The gid cache code is some of the more terrifying code in the > > tree.. Are you going to review all patches for it to make sure it > > doesn't break your device? > > > > IMHO, just return the only GID in query device and be done with it - > > forget about sysfs. > > > >> I can return 0 instead of -EOPNOTSUPP in ib_query_port in order to pass the > >> device registration successfully. > >> Alternatively, one could argue that if phys_port_cnt > 0 query_port should be > >> implemented as well (and remove the if statement entirely). > > > > Maybe some of this stuff should be split from the 'gid cache', the > > pkey cache and subnet prefix cache are really unrelated and maybe only > > used by kverbs.. > > The query_port callback is used from netlink (link query) as well. > I think that consolidating the checks to ib_query_port is the most reasonable > thing to do in order to protect all different flows. Both returning 0 and > -EOPNOTSUPP are fine with me. I don't think query_port should fail.. ie in netlink if it fails then you can't do get_netdev which seems useful for something like usnic.. All the call sites would have to be audited otherwise. > We can also split the mandatory funcs to two: mandatory and mandatory for > kverbs. This way if a mandatory function (one that is needed for device > registration, such as query_device, query_port, port_immutable) is missing we > will fail the registration. If a kverb function is missing we'll mark the device > as non-kverbs provider. This way we can remove all the additional checks added > in this patch. Well, this is sort of what we are doing.. Checking the functions seems like too much babying of drivers though - we call the really mandatory ones during registration so just NULL deref for terminally broken drivers is not so bad. Jason
