On 21-Jan-19 18:57, Jason Gunthorpe wrote: > 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. That sounds reasonable as well. I'll send v6 without the NULL checks.
