Re: [PATCH net-next v3] net: ioctl: Use kernel memory on protocol ioctl callbacks

On Thu, May 25, 2023 at 11:34 AM David Ahern <[email protected]> wrote:
>
> On 5/25/23 9:05 AM, Willem de Bruijn wrote:
> >> +/* A wrapper around sock ioctls, which copies the data from userspace
> >> + * (depending on the protocol/ioctl), and copies back the result to userspace.
> >> + * The main motivation for this function is to pass kernel memory to the
> >> + * protocol ioctl callbacks, instead of userspace memory.
> >> + */
> >> +int sk_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
> >> +{
> >> +       int rc = 1;
> >> +
> >> +       if (ipmr_is_sk(sk))
> >> +               rc = ipmr_sk_ioctl(sk, cmd, arg);
> >> +       else if (ip6mr_is_sk(sk))
> >> +               rc = ip6mr_sk_ioctl(sk, cmd, arg);
> >> +       else if (phonet_is_sk(sk))
> >> +               rc = phonet_sk_ioctl(sk, cmd, arg);
> >
> > I don't understand what this buys us vs testing the sk_family,
> > sk_protocol and cmd here.
>
> To keep protocol specific code out of core files is the reason I
> suggested it.

I guess you object to demultiplexing based on per-family
protocol and ioctl cmd constants directly in this file?

That only requires including the smaller uapi headers.

But now net/core/sock.h now still has to add includes
linux/mroute.h, linux/mroute6.h and net/phonet/phonet.h.

Aside on phonet_is_sk, if we're keeping this: this should be
sk_is_phonet? Analogous to sk_is_tcp and such. And, it should suffice
to  demultiplex based on the protocol family, without testing the
type or protocol. The family is defined in protocol-independent header
linux/socket.h. The differences between
PN_PROTO_PHONET and PN_PROTO_PIPE should be handled inside the family
code. So I think it is cleaner just to open-coded as `if
(sk->sk_family == PF_PHONET)`