[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <643ef2643f3ce_352b2f2945d@willemb.c.googlers.com.notmuch>
Date: Tue, 18 Apr 2023 15:41:24 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Breno Leitao <leitao@...ian.org>,
Willem de Bruijn <willemdebruijn.kernel@...il.com>,
kuba@...nel.org
Cc: Jens Axboe <axboe@...nel.dk>, David Ahern <dsahern@...nel.org>,
Willem de Bruijn <willemb@...gle.com>,
io-uring@...r.kernel.org, netdev@...r.kernel.org, kuba@...nel.org,
asml.silence@...il.com, leit@...com, edumazet@...gle.com,
pabeni@...hat.com, davem@...emloft.net, dccp@...r.kernel.org,
mptcp@...ts.linux.dev, linux-kernel@...r.kernel.org,
matthieu.baerts@...sares.net, marcelo.leitner@...il.com
Subject: Re: [PATCH 0/5] add initial io_uring_cmd support for sockets
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.
Powered by blists - more mailing lists