[PATCH 1/1] MicroSemi Switchtec management interface driver

[Emil Velikov]

Hi Logan,

NOTE: Please take my comments with a healthy pinch of salt.

I'd imagine that core/more experienced developers have more thorough
feedback, so I'll mention a few things on the less common part -
robust/compat UABI.
Above all, please read through the in-tree documentation on the topic [1]

[1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/ioctl/botching-up-ioctls.txt?id=refs/tags/v4.10-rc6

On 31 January 2017 at 17:03, Logan Gunthorpe <logang at deltatee.com> wrote:

> --- /dev/null
> +++ b/include/uapi/linux/switchtec_ioctl.h


> +enum switchtec_ioctl_partition {

> +       SWITCHTEC_IOCTL_NUM_PARTITIONS,
> +};
I'm not sure what the overall consensus on the topic of 'enums vs
defines in UABI'. Fwiw personally I'm in favour of defines since enums
can lead to subtle issues if one is not extra careful. For example:
 - add new enum amidst the list - ABI break
 - add new enum at the end of the list [just before NUM_PARTITIONS] - ABI break.
 - opt for negative value for existing/new enum - ABI break
 - other

With a variable size in mind, carefully consider how you'll copy data
back and forth in the case of new kernel (supports partition[X) + old
userspace (partition [X-1]) and vice-versa. Feel free to look at the
drm ioctl handler and/or others through the kernel.

Hint - as-is we'll end up with buffer overflows/other issues.

> +
> +struct switchtec_ioctl_fw_info {
> +       __u32 flash_length;
> +
> +       struct {
> +               __u32 address;
> +               __u32 length;
> +               __u32 active;
Something to keep in mind, not sure how likely it is here:
If you embed structs (partition in this case), you will not be able to
extend it [the embedded one] it in the future without the risk of ABI
breakage.

> +       } partition[SWITCHTEC_IOCTL_NUM_PARTITIONS];
> +};
> +
Afaict this and most/all other structs [in this patch] are in
violation of Prerequisites#2 and/or #3.
Personally I find pahole quite useful - reference all the UABI structs
in a dummy userspace app, compile with gcc -g -O0 and observe the
output across 32 and 64bit builds. Members _must_ be at the same
offset, otherwise things will be broken with 64bit kernel on a 32bit
userspace. Something which people will eventually end up trying/using,
even if you don't plan to support it.

Please check that Basics (#2 and #5 in particular) are also covered.

A couple more generic suggestions, which I may have noticed while
skimming through:
 - please ensure that all the input is properly sanitised, before,
going into the actual implementation
Afaict having the separate step/separation helps clarity and reduces
chances of you/others getting it wrong down the line.
 - user provided storage must not be changed when the ioctl fails.

Additionally you want to provide a reference to open-source userspace
[alongside acknowledgement of the maintainers/co-developers about the
design] which makes use of said IOCTL(s). But please, do _not_ merge
userspace code before the kernel work has landed.

Hope that provided you with sufficient good points wrt IOCTL design.

Regards,
Emil