Messages in this thread |  | | From | Peng Tao <> | Date | Mon, 18 Nov 2013 14:18:04 +0800 | Subject | Re: [PATCH 05/40] staging/lustre: validate open handle cookies |
| |
On Mon, Nov 18, 2013 at 12:35 PM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Mon, Nov 18, 2013 at 10:36:26AM +0800, Peng Tao wrote: >> On Sun, Nov 17, 2013 at 3:50 AM, Greg Kroah-Hartman >> <gregkh@linuxfoundation.org> wrote: >> > On Sat, Nov 16, 2013 at 11:20:37AM +0000, Dilger, Andreas wrote: >> >> On 2013/11/14 9:13 PM, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org> >> >> wrote: >> >> >> >> >On Fri, Nov 15, 2013 at 12:13:07AM +0800, Peng Tao wrote: >> >> >> From: "John L. Hammond" <john.hammond@intel.com> >> >> >> >> >> >> Add a const void *h_owner member to struct portals_handle. Add a const >> >> >> void *owner parameter to class_handle2object() which must be matched >> >> >> by the h_owner member of the handle in addition to the cookie. >> >> > >> >> >Ick ick ick. >> >> > >> >> >NEVER use a void pointer if you can help it, and for a "handle", never. >> >> >This isn't other operating systems, sorry. We know what types our >> >> >pointers to structures are, use them, so that the compiler can catch our >> >> >problems, and don't try to cheat by using void *. >> >> >> >> The portals_handle is used as a generic type for objects referenced over >> >> the network, like a file handle. The "owner" parameter is just used as >> >> an extra check that the cookie passed from the client is actually a >> >> valid value for the code in which it is being used (e.g. metadata or >> >> data object). It isn't actually dereferenced by anything, it is just >> >> like a magic value. >> > >> > Then make it an explicit type, not a void *. >> > >> >> >> Adjust >> >> >> the callers of class_handle2object() accordingly, using NULL as the >> >> >> argument to the owner parameter, except in the case of >> >> >> mdt_handle2mfd() where we add an explicit mdt_export_data parameter >> >> >> which we use as the owner when searching for a MFD. When allocating a >> >> >> new MFD, pass a pointer to the mdt_export_data into mdt_mfd_new() and >> >> >> store it in h_owner. This allows the MDT to validate that the client >> >> >> has not sent the wrong open handle cookie, or sent the right cookie to >> >> >> the wrong MDT. >> >> > >> >> >This changelog entry doesn't even match up with the code below. ALl >> >> >callers of class_handle2object are passing NULL here, which makes this >> >> >patch pretty pointless, right? >> >> >> >> As Tao wrote, this is the patch summary that matches what was committed >> >> in our own tree and in this case mostly describes the changes made on the >> >> server. Keeping the same commits and comments in both trees makes it >> >> easier to keep the code in sync. >> > >> > Ok, but as it is, this patch does nothing to the client code, so how can >> > I accept it? A function that is only ever called with NULL as an option >> > is ripe for cleanup in my eyes. >> > >> How about adding a comment above the function to note that this extra >> argument is used by server code and please don't remove it? > > How about adding the server code to the kernel to keep problems like > this (which will continue to crop up, it's not just this one function, > right?) from happening in the future? > As explained in the other thread, the server code is not even ready for landing in upstream kernel. And it won't be for quite some time.
> In-kernel code does not depend on out-of-kernel code, it's that simple, > and has been a rule for kernel code for forever. Either deal with the > fact that you will have to keep the apis and functions working for your > out-of-tree code, or put all the code into the kernel. Don't force > in-kernel code to deal with out-of-tree code as there is NO way that > anyone other than the very few of you, can deal with that at all. > Fair enough. Andreas, how about we handling this kind of difference in external tree and letting in-tree client be clean of it? We already have HAVE_SERVER_SUPPORT macro in external tree. It is just a matter of adding more references.
Thanks, Tao
|  |