Re: [PATCH 3/4] IB/uverbs: ex_query_device: check request's comp_mask

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

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