Hi, Le jeudi 29 janvier 2015 à 11:09 -0700, Jason Gunthorpe a écrit : > On Wed, Jan 28, 2015 at 02:19:11PM +0100, Yann Droneaud wrote: > > > But the same program (either binary or source code) might fail on > > newer kernel where some bits in comp_mask gain a meaning not supported > > by the program. > > To clarify some of this: > > The intention of the comp_mask scheme was to define a growing > structure. > > The invariant is: A bigger structure allways has all smaller > structures (past versions) as a prefix. > > So the size alone is enough for user space to know what is going > on. userspace only processes structure members up to the size returned > by the kernel, and there is only one structure layout. > Unfortunately, the userspace don't get the size of the returned data: it's only a single "write()" syscall after all. So the kernel is left with the comp_mask in the response to express the returned size. > The purpose of the output comp_mask is to allow drivers to declare > they do not support the new structure members, and comp_mask bits > should only be used with new structure members do not have a natural > 'null' value. > It's not (yet) about drivers as the output's comp_mask (in the response), is set by uverbs layer. Do you think we have to enforce a 1 to 1 relation between the request comp_mask's bits and the response comp_mask's bits ? That would not fit with my understanding of "Extending Verbs API" presentation [1]: request's comp_mask describe the fields present in the request and response's comp_mask describe the fields present in the response. > Further, we need to distinguish cases where additional data is > mandatory or optional. > > query_device is clearly optional, the only purpose the input comp mask > serves is to reduce expensive work in the driver by indicating that > some result bits are not needed. That's not how I've understand the purpose of the request's comp_mask after reading the presentation: request's comp_mask describe the content of the request. Then that additional content can trigger the presence of additional fields in the response if needed. > It is perfectly OK for the kernel to > ignore the input comp mask, and OK for userspace to typically request > all bits. (indeed, I think this is a pretty silly optimization myself, > and the original patch that motivated this was restructured so it is > not needed) > It's not at all OK to ignore request's comp_mask: it has to be checked to find if there's a feature requested the kernel cannot fullfil, and any unknown bit is a possible feature request. So the kernel has to reject the request as a whole as it don't know how to process it. As we don't know yet how we would extend the extended QUERY_DEVICE uverbs, the kernel have to refuse any random value. http://blog.ffwll.ch/2013/11/botching-up-ioctls.html > Other verbs must be more strict, if one side does not understand the > comp mask then the verb must fail in some way. Presumably the valid > comp mask values are inferred in some way (eg query_device says the > extended function is supported) > > Everything should fit in one of those two general cases.. I believe every interface should return an error for any unknown value (every unused bits of a data structure), that is, there's only one case. > > > Thanks for the link to the presentation. > > If I recall the presentation is old and had some flaws that were > addressed before things made it into libibverbs.. > I have to have a look to this part of libibverbs: I'm not sure the extended QUERY_DEVICE is already implemented. > Thank you for looking at this, it is very important that this scheme > is used properly, and it is very easy to make mistakes. I haven't had > time to review any of these new patches myself. > I hope you would find some time to review my latest patchset so that we don't miss a corner case. It's starting to become urgent. Regards. -- Yann Droneaud OPTEYA -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
