Re: [PATCH v1 1/5] IB/uverbs: ex_query_device: answer must not depend on request's comp_mask

[Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>]

Hi,

Le jeudi 29 janvier 2015 à 11:28 -0700, Jason Gunthorpe a écrit :
> On Thu, Jan 29, 2015 at 06:59:58PM +0100, Yann Droneaud wrote:
> > As specified in "Extending Verbs API" presentation [1] by Tzahi Oved
> > during OFA International Developer Workshop 2013, the request's
> > comp_mask should describe the request data: it's describe the
> > availability of extended fields in the request.
> > Conversely, the response's comp_mask should describe the presence
> > of extended fields in the response.
> 
> Roland: I agree with Yann, these patches need to go in, or the ODP
> patches reverted.
> 
> >  #ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING
> > -	if (cmd.comp_mask & IB_USER_VERBS_EX_QUERY_DEVICE_ODP) {
> 
> Absolutely, a query output should never depend on the input comp_mask.
> 

We should keep this in mind then.

> > -		resp.odp_caps.general_caps = attr.odp_caps.general_caps;
> > -		resp.odp_caps.per_transport_caps.rc_odp_caps =
> > -			attr.odp_caps.per_transport_caps.rc_odp_caps;
> > -		resp.odp_caps.per_transport_caps.uc_odp_caps =
> > -			attr.odp_caps.per_transport_caps.uc_odp_caps;
> > -		resp.odp_caps.per_transport_caps.ud_odp_caps =
> > -			attr.odp_caps.per_transport_caps.ud_odp_caps;
> > -		resp.comp_mask |= IB_USER_VERBS_EX_QUERY_DEVICE_ODP;
> > -	}
> > +	resp.odp_caps.general_caps = attr.odp_caps.general_caps;
> > +	resp.odp_caps.per_transport_caps.rc_odp_caps =
> > +		attr.odp_caps.per_transport_caps.rc_odp_caps;
> > +	resp.odp_caps.per_transport_caps.uc_odp_caps =
> > +		attr.odp_caps.per_transport_caps.uc_odp_caps;
> > +	resp.odp_caps.per_transport_caps.ud_odp_caps =
> > +		attr.odp_caps.per_transport_caps.ud_odp_caps;
> >  #endif
> > +	resp.comp_mask |= IB_USER_VERBS_EX_QUERY_DEVICE_ODP;
> 
> Not sure about this - if 0 is a valid null answer for all the _caps
> then it is fine, and the comp_mask bit should just be removed as the
> size alone should be enough.
> 

That's difficult to say. But I hope 0 are valids answers in this case.

Anyway, the response's comp_mask is needed, as it's the sole way for 
the userspace to know the size of the response. See below.

> This looks wrong however:
> 
> >  	err = ib_copy_to_udata(ucore, &resp, sizeof(resp));
> >  	if (err)
> >           return err;
> >        return 0;
> 
> Why isn't this returning the filled structure size to userspace?  This
> would seem to be very urgently wrong to me? Am I missing something?
> 
> Patch 3 probably should gain:
> 
> - return 0;
> + return resp_len;
> 

The write() syscall must return the size buffer passed to it, or less,
but in such case it would ask for trouble as userspace would be allowed
to write() the remaining bytes.
Returning a size bigger than the one passed to write() is not acceptable
and would break any expectation.

> This patch looks like an improvement to me:
> 
> Reviewed-By: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> 

Thanks a lot.

Regards.

-- 
Yann Droneaud
OPTEYA


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html