On Wed, Oct 11, 2023 at 11:48:43PM +0300, Petre Eftime wrote: > On Wed, Oct 11, 2023 at 8:46 PM Greg Kroah-Hartman > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote: > > > > On Wed, Oct 11, 2023 at 02:24:48PM +0200, Arnd Bergmann wrote: > > > On Tue, Oct 10, 2023, at 10:27, Greg Kroah-Hartman wrote: > > > > On Tue, Oct 10, 2023 at 10:08:43AM +0200, Alexander Graf wrote: > > > >> On 10.10.23 10:03, Greg Kroah-Hartman wrote: > > > >> > > > >> > > Out of these, the NSM communication protocol uses all except Semantic tags > > > >> > > and Floats. The CBOR library that this patch imports does not have special > > > >> > > handling for Semantic tags, which leaves only floats which are already > > > >> > > #ifdef'ed out. That means there is not much to trim. > > > >> > > > > > >> > > What you see here is what's needed to parse CBOR in kernel - if that's what > > > >> > > we want to do. I'm happy to rip it out again and make it a pure user space > > > >> > > problem to do CBOR :). > > > >> > Yes, why are we parsing this in the kernel? What could go wrong with > > > >> > adding yet-another-parser in privileged context? :) > > > >> > > > > >> > Why does this have to be in the kernel, the data sent/recieved is over > > > >> > virtio, so why does the kernel have to parse it? I couldn't figure that > > > >> > out from the driver, yet the driver seems to have a lot of hard-coded > > > >> > parsing logic in it to assume specific message formats? > > > >> > > > >> > > > >> The parsing doesn't have to be in kernel and it probably shouldn't be > > > >> either. V3 of the patch was punting all the parsing to user space, at which > > > >> point you and Arnd said I should give it a try to do the protocol parsing in > > > >> kernel space instead. That's why the parser is here. > > > > > > > > Arnd said that, not me :) > > > > > > > >> If we conclude that all this in-kernel parsing is not worth it, I'm very > > > >> happy to just go back to the the v3 ioctl interface and post v5 with hwrng > > > >> merged into misc, but remove all CBOR logic again :) > > > > > > > > I think the less parsers we have in the kernel, the safer we are for > > > > obvious reasons. Unless you have a parser for this in rust? :) > > > > > > > > I don't really know, having a generic interface is good, but at the > > > > expense of this api is probably not good. individual ioctls might be > > > > better if there are not going to be any other drivers for this type of > > > > thing? > > > > > > I was definitely expecting something simpler than what was possible > > > in the v4 patch. I had another look now, and it's clear that the > > > ioctl interface is still not great because the variable data structures > > > shine through for some of the calls, and even to get to this point, > > > a whole lot of complexity is required underneath. > > > > > > To get anything better, one would probably have to redesign the entire > > > interface stack (hypervisor, kernel and userland) to use regular > > > fixed data structures, and this seems unlikely to happen. > > > > Why not fix this and do it properly? What's preventing that from > > happening? We don't want to create an interface here that is broken, or > > insecure, or a pain to maintain, right? > > > > thanks, > > > > greg k-h > > I would think the proposal to have fixed structures would be a > downgrade in terms of maintainability, usability and security, not an > improvement. > > This current interface allows the hypervisor to extend the existing > functionality at any time, and the Linux kernel does not have to > change anything for that to work, the application does not have to be > recompiled to use the new kernel headers at any point. Adding new > APIs, adding new fields to API responses, or adding optional > parameters to the API is fully backwards compatible, would also not > require changes in userspace, as the CBOR data structures that are not > recognized can simply be skipped. This allows easy backwards > compatibility in most cases, and the userspace would be able to opt in > to new features only if it requires them, without forcing the upgrade > if it's not required. > > With fixed structures, then the driver would need to be more > explicitly versioned and would need to be able to handle multiple > versions of the API at the same time, which is both more complex, less > flexible and more prone to errors. Yes, but you are trading off the complexity of adding yet-another-protocol-parser to the kernel vs. making a strict user/kernel api, right? Which one is correct? Can you verify that this parser really is correct and doesn't have any overflows/security issues in it? I can't, maybe someone needs to write it in rust :) thanks, greg k-h
