Re: [Linaro-mm-sig] [PATCHv2 3/4] staging: android: ion: Add an ioctl for ABI checking
From: Laura Abbott
Date: Fri Sep 02 2016 - 18:14:17 EST
On 09/02/2016 02:33 PM, Arnd Bergmann wrote:
On Friday, September 2, 2016 1:33:44 PM CEST Laura Abbott wrote:
On 09/02/2016 02:02 AM, Arnd Bergmann wrote:
On Thursday, September 1, 2016 3:40:43 PM CEST Laura Abbott wrote:
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -22,6 +22,29 @@
#include "ion_priv.h"
#include "compat_ion.h"
+union ion_ioctl_arg {
+ struct ion_fd_data fd;
+ struct ion_allocation_data allocation;
+ struct ion_handle_data handle;
+ struct ion_custom_data custom;
+ struct ion_abi_version abi_version;
+};
Are you introducing this, or just clarifying the defintion of the
existing interface. For new interfaces, we should not have a union
as an ioctl argument. Instead each ioctl command should have one
specific structure (or better a scalar argument).
This was just a structure inside ion_ioctl. I pulled it out for
the validate function. It's not an actual argument to any ioctl from
userspace. ion_ioctl copies using _IOC_SIZE.
Ok, got it. This is fine from an interface point of view, just
a bit unusual in the way it's written.
+static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
+{
+ int ret = 0;
+
+ switch (cmd) {
+ case ION_IOC_ABI_VERSION:
+ ret = arg->abi_version.reserved != 0;
+ break;
+ default:
+ break;
+ }
+
+ return ret ? -EINVAL : 0;
+}
I agree with Greg, ioctl interfaces should normally not be versioned,
the usual way is to try a command and see if it fails or not.
The concern was trying ioctls that wouldn't actually fail or would
have some other unexpected side effect.
My conclusion from the other thread was that assuming we don't botch
up adding new ioctls in the future or make incompatible changes to
these in the future we shouldn't technically need it. I was still
trying to hedge my bets against the future but that might just be
making the problem worse?
We've had a number of cases where versioned ABIs just didn't work out.
The versions are either used to distinguish incompatible APIs, which
we should avoid to start with, or they are used for backwards-compatible
extensions that you should detect by checking whether an ioctl
succeeds. Relying on the API version number breaks if you get a partial
backport of features from a later version, and it's unclear what a
user space tool should expect when the kernel reports a newer ABI
than it knows.
I think the wireless extensions and KVM are examples of versioned
APIs that turned out to make things more complicated than they
would have been otherwise.
Okay it sounds like the answer is to strive to never run into a case
where versioned ioctls are necessary. Shouldn't be too hard, right? ;)
+/**
+ * struct ion_abi_version
+ *
+ * @version - current ABI version
+ */
+
+#define ION_ABI_VERSION KERNEL_VERSION(0, 1, 0)
+
+struct ion_abi_version {
+ __u32 abi_version;
+ __u32 reserved;
+};
+
This interface doesn't really need a "reserved" field, you could
as well use a __u32 by itself. If you ever need a second field,
just add a new command number.
The botching-ioctls.txt document suggested everything should be aligned
to 64-bits. Was I interpreting that too literally?
I didn't even know that file existed ;-)
I'm pretty sure the paragraph refers to the problem of x86 of having
a structure like
struct ioctl_arg {
__u64 first;
__u32 second;
};
which is 12 bytes long on x86, but 16 bytes long including implied
padding on all 64-bit architectures and most (maybe all) 32-bit ones
other than x86.
Right, that's the problem it's trying to avoid.
If there is no 64-bit member in the struct, there is no need for padding
at the end.
That's what I thought as well. I think I'll submit a patch to the docs
clarifying a few things.
Arnd
Thanks,
Laura