On Wed, Jan 09, 2019 at 09:52:14AM +0200, Gal Pressman wrote:
> On 09-Jan-19 01:39, Jason Gunthorpe wrote:
> > On Tue, Jan 08, 2019 at 09:55:54PM +0000, Majd Dibbiny wrote:
> >>> @@ -537,11 +538,13 @@ static int setup_device(struct ib_device *device)
> >>> }
> >>>
> >>> memset(&device->attrs, 0, sizeof(device->attrs));
> >>> - ret = device->ops.query_device(device, &device->attrs, &uhw);
> >>> - if (ret) {
> >>> - dev_warn(&device->dev,
> >>> - "Couldn't query the device attributes\n");
> >>> - goto port_cleanup;
> >>> + if (device->ops.query_device) {
> >
> >> Why do we need these kind of checks now? In case device doesn’t
> >> implement all mandatory kverbs, then clients won’t add it.. uverbs
> >> has uverbs_cmd_mask in write path, and checks function pointer in
> >> the ioctl path.. Maybe I’m missing something..
> >
> > Yah, I agree, the checks should be in the uverbs layer, and presumably
> > they are all there already, it just needs careful checking.
>
> I've inspected each flow that comes down to using these callbacks, the checks
> are only added for flows that are not already protected by NULL checks (by the
> uverbs layer or in use by kverbs only which guarantees callback existence).
>
> Specifically, the check Majd pointed out in setup_device is called as part of
> ib_register_device which is not protected by the uverbs layer.
OK
> Other checks need careful inspection:
> ib_query_pkey is called from sysfs flow for example, which assumes the callback
> exists.
Don't create pkey sysfs files if they can't work
> ib_query_port has many call sites in drivers which assume the
> callback exist.
Presumably a non-kverbs driver will not call itself in a bad way..
> Create CQ/QP flows are checked for create_qp/cq existence by the
> uverbs layer, but in case of failure call destroy_qp/cq which are no
> longer guaranteed to exist.
The CQ and QP objects and all their uAPI methods are deleted if the
destroy methods are not available, so I don't think this is real.
Jason
