Breno Leitao wrote: > On Thu, Apr 13, 2023 at 10:24:31AM -0400, Willem de Bruijn wrote: > > > How to handle these contradictory behaviour ahead of time (at callee > > > time, where the buffers will be prepared)? > > > > Ah you found a counter-example to the simple pattern of put_user. > > > > The answer perhaps depends on how many such counter-examples you > > encounter in the list you gave. If this is the only one, exceptions > > in the wrapper are reasonable. Not if there are many. > > > Hello Williem, > > I spend sometime dealing with it, and the best way for me to figure out > how much work this is, was implementing a PoC. You can find a basic PoC > in the link below. It is not 100% complete (still need to convert 4 > simple ioctls), but, it deals with the most complicated cases. The > missing parts are straighforward if we are OK with this approach. > > https://github.com/leitao/linux/commits/ioctl_refactor > > Details > ======= > > 1) Change the ioctl callback to use kernel memory arguments. This > changes a lot of files but most of them are trivial. This is the new > ioctl callback: > > struct proto { > > int (*ioctl)(struct sock *sk, int cmd, > - unsigned long arg); > + int *karg); > > You can see the full changeset in the following commit (which is > the last in the tree above) > https://github.com/leitao/linux/commit/ad78da14601b078c4b6a9f63a86032467ab59bf7 > > 2) Create a wrapper (sock_skprot_ioctl()) that should be called instead > of sk->sk_prot->ioctl(). For every exception, calls a specific function > for the exception (basically ipmr_ioctl and ipmr_ioctl) (see more on 3) > > This is the commit https://github.com/leitao/linux/commit/511592e549c39ef0de19efa2eb4382cac5786227 > > 3) There are two exceptions, they are ip{6}mr_ioctl() and pn_ioctl(). > ip{6}mr is the hardest one, and I implemented the exception flow for it. > > You could find ipmr changes here: > https://github.com/leitao/linux/commit/659a76dc0547ab2170023f31e20115520ebe33d9 > > Is this what you had in mind? > > Thank you! Thanks for the series, Breno. Yes, this looks very much what I hoped for. The series shows two cases of ioctls: getters that return an int, and combined getter/setters that take a struct of a certain size and return the exact same. I would deduplicate the four ipmr/ip6mr cases that constitute the second type, by having a single helper for this type. sock_skprot_ioctl_struct, which takes an argument for the struct size to copy in/out. Did this series cover all proto ioctls, or is this still a subset just for demonstration purposes -- and might there still be other types lurking elsewhere? If this is all, this looks like a reasonable amount of code churn to me. Three small points * please keep the __user annotation. Use make C=2 when unsure to warn about mismatched annotation * minor: special case the ipmr (type 2) ioctls in sock_skprot_ioctl and treat the "return int" (type 1) ioctls as the default case. * introduce code in a patch together with its use-case, so no separate patches for sock_skprot_ioctl and sock_skprot_ioctl_ipmr. Either one patch, or two, for each type of conversion.
