On 18/02/2015 08:21, Roland Dreier wrote: >> The verb also checks the user-space provided response buffer size and only >> fills in capabilities that will fit in the buffer. In attempt to follow the >> spirit of presentation [2] by Tzahi Oved that was presented during >> OpenFabrics >> Alliance International Developer Workshop 2013, the comp_mask bits will only >> describe which fields are valid. > >> + if (ucore->outlen < resp.response_length) >> + return -ENOSPC; > > So is this really what we want? A future kernel that adds new fields > will cause previously working userspace to get an error?
No, the code is intended to only check for the legacy fields, comp_mask and response_length. At this point though, those are all the fields in the response struct, so I use sizeof. The next patch adds more fields to the response struct, and changes the value of response_length at this point to be the same as it was in this patch using offsetof: > --- a/drivers/infiniband/core/uverbs_cmd.c > +++ b/drivers/infiniband/core/uverbs_cmd.c > @@ -3318,7 +3318,7 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file > *file, > if (cmd.reserved) > return -EINVAL; > > - resp.response_length = sizeof(resp); > + resp.response_length = offsetof(typeof(resp), odp_caps); > > if (ucore->outlen < resp.response_length) > return -ENOSPC; ... > Is there anything wrong with truncating the response to only include > the fields that userspace asked for? Yann had strong objections to that implementation. The main issue is as I see it is that it can hide user-space bugs where user-space retrieves a partial field in the response struct. > Is it just that this is the > initial implementation of the query_device_ex verb, so the current > size is also the minimum size? Yes. > If so I think we need at least a > comment here, so that we remember to fix the ENOSPC code when adding > more fields in the kernel. Sure, I'll resend the patches with a comment. Regards, Haggai -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
